Add mount_setattr

[yujincheng08]

Preliminary implementation of mount_setattr.

A few notes:

1. I leave the size non-exported. Should we export this?
2. I intentionally place mount_setattr in misc.rs because I am going to add statmount and listmount syscall once 6.8 releases and libc adds the syscall number.
3. CI test requires android: add missing syscall constants rust-lang/libc#3558

Fix #975

[sunfishcode]

According to Linux's documentation, attr_set is a __u64, while MountAttrFlags is currently a c_uint. Could you add a test testing that the layout matches, following one of the "layouts" tests in the tree?

[sunfishcode]

I leave the size non-exported. Should we export this?

If you don't have a need for it at this time, then we don't need to. If someone later has a need for it, we can add another function that exposes it.

[ayosec]

I would like to help to finish this pull-request, in case @yujincheng08 can't continue it.

[ayosec]

userns_fd field is expected as a __u64, but BorrowedFd is 4 bytes. Maybe it is safer to declare a private struct with the exact same fields of the C API, and translate the MountAttr fields to it before calling mount_setattr.

#[repr(C)]
pub(crate) struct mount_attr {
    pub attr_set: u64,
    pub attr_clr: u64,
    pub propagation: u64,
    pub userns_fd: u64,
}

---

Also, userns_fd is needed only when attr_set includes MOUNT_ATTR_IDMAP. The field could be declared as an Option<BorrowedFd<'a>>, and send it as -EBADFD to the syscall if it is None.

[rusty-snake]

Maybe it is safer to declare a private struct with the exact same fields of the C API, and translate the MountAttr fields to it before calling mount_setattr.

FWIW: https://codeberg.org/crabjail/lnix/src/commit/092defd573bab18f32c466dd7f774a03236babb6/src/mount/mountfd.rs#L453-L488

[ayosec]

I think that the way mount_attr is defined in lnix is better than exposing the fields directly.

The usage would be something like this:

mount_setattr(
    dir_fd,
    c"", 
    MountSetattrFlags::RECURSIVE | MountSetattrFlags::EMPTY_PATH,
    MountAttr::new().set_flags(MountAttrFlags::RDONLY),
)?;

@sunfishcode Do you want to take the same approach from lnix?

I can implement the changes if needed.

[sunfishcode]

How is userns_fd exposed in the lnix API?

In general, I'm in favor of encapsulating these fields like this, provided we can do so without restricting useful functionality.

[ayosec]

How is userns_fd exposed in the lnix API?

It seems that it does not support setting values to userns_fd (there is a TODO: ID-mapped mounts, and userns_fd only appears in the struct definition).

mount_setattr() will not close the file descriptor. I guess that MountAttr should take the ownership of the file descriptor, so it can guarantee that the descriptor is valid when mount_setattr is called, and closed when MountAttr is not needed anymore.

Maybe something like this:

pub struct MountAttr {
    mount_attr: sys::mount_attr,
    userns_fd: Option<OwnedFd>, 
}

impl MountAttr {

    pub fn set_userns_fd(&mut self, userns_fd: OwnedFd) -> &mut Self {
        self.mount_attr.userns_fd = userns_fd.as_raw_fd() as u64;
        self.userns_fd = Some(userns_fd);
        self
    }

    // ...

[rusty-snake]

I guess that MountAttr should take the ownership of the file descriptor, so it can guarantee that the descriptor is valid when mount_setattr is called, and closed when MountAttr is not needed anymore.

A BorrowedFd would work too.

While BorrowedFd/OwnedFd are ffi safe, this does not help us here because we need a u64.

You should first find answers to the following questions.

• Is a lifetime parameter on MountAttr ok?
• Where on the raw-opinionated scalar should rustix' implementation be.
  • userns_fd is ignored unless set_flags contains MOUNT_ATTR_IDMAP.
• Do you want a zero-copy repr(C) struct or a copy repr(Rust) struct?

If a lifetime is ok, you could use a phantom.

#[repr(C)]
struct mount_attr<'fd> {
    attr_set: u64, // MountAttrFlags is repr(transparent) but not u64.
    attr_clr: u64,
    propagation: u64,
    userns_fd: u64,
    _userns_fd_phantom: PhantomData<BorrowedFd<'fd>>,
}

Otherwise, you can also take ownership.

#[repr(C)]
struct mount_attr<'fd> {
    attr_set: u64, // MountAttrFlags is repr(transparent) but not u64.
    attr_clr: u64,
    propagation: u64,
    userns_fd: u64,
    _userns_fd: Option<OwnedFd>,
}

[ayosec]

A BorrowedFd would work too.

I didn't consider it because it requires a change in the lifetime, but now I think it makes more sense to use BorrowedFd instead of an OwnedFd.

I think that the main issue with BorrowedFd is how to call the functions to set the values (in lnix: set_flags, clear_flags, and propagation). Those functions receive and return a &mut self. However, userns_fd must use the lifetime of its parameter, so it returns a new type.

For example, we could define MountAttr like this:

#[repr(C)]
#[allow(non_camel_case_types)]
#[derive(Default, Debug)]
struct mount_attr {
    attr_set: u64,
    attr_clr: u64,
    propagation: u64,
    userns_fd: u64,
}

#[derive(Debug)]
pub struct MountAttr<'a> {
    attr: mount_attr,
    userns_fd: PhantomData<BorrowedFd<'a>>,
}

Then, the default constructor would return a 'static lifetime:

impl MountAttr<'static> {
    pub fn new() -> Self {
        MountAttr {
            attr: mount_attr::default(),
            userns_fd: PhantomData,
        }
    }
}

But for userns_fd it must return the lifetime of the BorrowedFd, so it can't return Self. The implementation could be something like this:

impl<'a> MountAttr<'a> {

    pub fn userns_fd<'b>(self, userns_fd: BorrowedFd<'b>) -> MountAttr<'b> {
        MountAttr {
            attr: mount_attr {
                userns_fd: userns_fd.as_raw_fd() as u64,
                ..self.attr
            },
            userns_fd: PhantomData,
        }
    }

This is important because it is inconsistent with the two ways to build an instance of MountAttr:

// This two blocks are identical:

foo(MountAttr::new().set(MountAttrFlags::MOUNT_ATTR_FOO));


let mut attr = MountAttr::new();
attr.set(MountAttrFlags::MOUNT_ATTR_FOO);
foo(&attr);


// But these blocks are different:

foo(MountAttr::new()
    .set(MountAttrFlags::MOUNT_ATTR_IDMAP)
    .userns_fd(fd.as_fd()));


let mut attr = MountAttr::new();
attr.set(MountAttrFlags::MOUNT_ATTR_IDMAP); // Updates `attr`.
attr.userns_fd(fd.as_fd());                 // Returns a new instance.
foo(&attr);                                 // Call `foo()` with no `userns_fd`.

Maybe, a way to avoid the possible confussion is to take the ownership of MountAttr to build its value, and always return an owned instance (instead of a reference).

impl<'a> MountAttr<'a> {
    pub fn set(mut self, flags: MountAttrFlags) -> Self {
        self.attr.attr_set = flags.bits() as u64;
        self
    }

    pub fn clear(mut self, flags: MountAttrFlags) -> Self {
        self.attr.attr_clr = flags.bits() as u64;
        self
    }

    pub fn propagation(mut self, propagation: MountPropagationFlags) -> Self {
        self.attr.propagation = propagation.bits() as u64;
        self
    }

    pub fn userns_fd<'b>(self, userns_fd: BorrowedFd<'b>) -> MountAttr<'b> {
        MountAttr {
            attr: mount_attr {
                userns_fd: userns_fd.as_raw_fd() as u64,
                ..self.attr
            },
            userns_fd: PhantomData,
        }
    }
}

I made a simple proof of concept with this idea.

[rusty-snake]

Where on the raw-opinionated scalar should rustix' implementation be.

• mount_setattr(MountAttr)
• mount_setattr_idmap(MountAttr, Fd)

[ayosec]

• mount_setattr(MountAttr)
• mount_setattr_idmap(MountAttr, Fd)

If we add specific functions for common cases, we could add specific constructors for MountAttr.

The rustix::mount module has multiple mount_* functions for common use-cases of mount(2), like mount_bind or mount_change, so it makes sense to support specific cases for mount_setattr.

I checked some projects using mount_setattr(2):

• When an ID-mapped mount is created, the most common usage of mount_setattr is with just attr_set = MOUNT_ATTR_IDMAP:

  • In composefs, lcfs-mount.c:lcfs_mount_erofs.
  • In containerd, mount_idmapped_linux.go:IDMapMount.
  • In lxc, conf.c:lxc_idmapped_mounts_parent.
  • In runc, mount_linux.go:mountFd.
  • In systemd, mount-util.c:remount_idmap_fd.
  • In util-linux, hook_idmap.c:hook_mount_post.

• attr_clr seems to be very uncommon.

  The only use-case I was able to find is to change the access-time setting, since it is a requirement of the syscall. From mount_setattr(2):

  Note that, since the access-time values are an enumeration rather than bit values, a caller wanting to transition to a different access-time setting cannot simply specify the access-time setting in attr_set, but must also include MOUNT_ATTR__ATIME in the attr_clr field. The kernel will verify that MOUNT_ATTR__ATIME isn't partially set in attr_clr [...]

• Both attr_set and propagation are used either alone or together.

I also checked other types in rustix that contain file descriptors:

• rustix::process::PrctlMmMap has a pub exe_fd: u32 field.

  The struct is used in rustix::process::configure_virtual_memory_map, which is marked as unsafe.

• rustix::io_uring::io_uring_params has a pub wq_fd: u32 field, which is used when io_uring_setup is called with IORING_SETUP_ATTACH_WQ.

  Unlike the previous function, rustix::io_uring::io_uring_setup is not marked as unsafe.

• rustix::event::PollFd contains a BorrowedFd field, which is private.

  This struct seems to be close to what is needed for MountAttr.

To keep consistency, we could define MountAttr similar to PollFd: keep all fields private, and provide constructors from common cases. For example:

pub struct MountAttr<'fd> {
    attr: c::mount_attr,
    userns_fd: PhantomData<BorrowedFd<'fd>>,
}

impl<'fd> MountAttr<'fd> {
    pub fn new<Fd: AsFd>(
        set: MountAttrFlags,
        clear: MountAttrFlags,
        propagation: MountAttrPropagation,
        userns_fd: Option<&'fd Fd>,
    ) -> Self {
        // If there is no `userns_fd`, use `fs::CWD` instead of `0`, so the
        // kernel will reply with `EBADF` if `IDMAP` is set.
        let userns_fd = userns_fd.map(|fd| fd.as_fd()).unwrap_or(fs::CWD);

        // ...
    }

    pub fn new_with_idmap<Fd: AsFd>(fd: &'fd Fd) -> Self {
        new(
            MountAttrFlags::IDMAP,
            MountAttrFlags::empty(),
            MountAttrPropagation::empty(),
            Some(fd),
        )
    }

    // ...
}

impl MountAttr<'static> {
    pub fn new_with_set(set: MountAttrFlags) -> Self {
        new(
            set,
            MountAttrFlags::empty(),
            MountAttrPropagation::empty(),
            None,
        )
    }

    pub fn new_with_propagation(propagation: MountAttrPropagation) -> Self {
        new(
            MountAttrFlags::empty(),
            MountAttrFlags::empty(),
            propagation,
            None,
        )
    }
}

[rusty-snake]

This struct should be non_exhaustive.