Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mount_setattr #1002

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add mount_setattr #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/backend/libc/mount/syscalls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,31 @@ pub(crate) fn fsconfig_reconfigure(fs_fd: BorrowedFd<'_>) -> io::Result<()> {
))
}
}

#[cfg(linux_kernel)]
#[cfg(feature = "mount")]
pub(crate) fn mount_setattr(
dir_fd: BorrowedFd<'_>,
path: &CStr,
flags: crate::fs::AtFlags,
mount_attr: &crate::mount::MountAttr<'_>,
) -> io::Result<()> {
syscall! {
fn mount_setattr(
dir_fd: c::c_int,
fs_name: *const c::c_char,
flags: c::c_uint,
mount_attr: *const crate::mount::MountAttr<'_>,
size: usize
) via SYS_mount_setattr -> c::c_int
}
unsafe {
ret(mount_setattr(
borrowed_fd(dir_fd),
c_str(path),
flags.bits(),
mount_attr as *const _,
core::mem::size_of::<crate::mount::MountAttr<'_>>(),
))
}
}
14 changes: 13 additions & 1 deletion src/backend/libc/mount/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::backend::c;
use crate::fd::BorrowedFd;
use bitflags::bitflags;

#[cfg(linux_kernel)]
Expand Down Expand Up @@ -150,9 +151,10 @@ pub(crate) enum FsConfigCmd {
#[cfg(feature = "mount")]
#[cfg(linux_kernel)]
bitflags! {
/// `MOUNT_ATTR_*` constants for use with [`fsmount`].
/// `MOUNT_ATTR_*` constants for use with [`fsmount`, `mount_setattr`].
///
/// [`fsmount`]: crate::mount::fsmount
/// [`mount_setattr`]: crate::mount::mount_setattr
#[repr(transparent)]
#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
pub struct MountAttrFlags: c::c_uint {
Expand Down Expand Up @@ -338,3 +340,13 @@ bitflags! {

#[cfg(linux_kernel)]
pub(crate) struct MountFlagsArg(pub(crate) c::c_ulong);

#[repr(C)]
#[derive(Debug, Copy, Clone)]
#[allow(missing_docs)]
pub struct MountAttr<'a> {
pub attr_set: MountAttrFlags,
pub attr_clr: MountAttrFlags,
pub propagation: MountPropagationFlags,
pub userns_fd: BorrowedFd<'a>,
}
20 changes: 20 additions & 0 deletions src/backend/linux_raw/mount/syscalls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,23 @@ pub(crate) fn fsconfig_reconfigure(fs_fd: BorrowedFd<'_>) -> io::Result<()> {
))
}
}

#[cfg(feature = "mount")]
#[inline]
pub(crate) fn mount_setattr(
dir_fd: BorrowedFd<'_>,
path: &CStr,
flags: crate::fs::AtFlags,
mount_attr: &crate::mount::MountAttr<'_>,
) -> io::Result<()> {
unsafe {
ret(syscall_readonly!(
__NR_mount_setattr,
dir_fd,
path,
flags,
mount_attr as *const crate::mount::MountAttr<'_>,
crate::backend::conv::size_of::<crate::mount::MountAttr<'_>, _>()
))
}
}
14 changes: 13 additions & 1 deletion src/backend/linux_raw/mount/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::backend::c;
use crate::fd::BorrowedFd;
use bitflags::bitflags;

bitflags! {
Expand Down Expand Up @@ -147,9 +148,10 @@ pub(crate) enum FsConfigCmd {

#[cfg(feature = "mount")]
bitflags! {
/// `MOUNT_ATTR_*` constants for use with [`fsmount`].
/// `MOUNT_ATTR_*` constants for use with [`fsmount`, `mount_setattr`].
///
/// [`fsmount`]: crate::mount::fsmount
/// [`mount_setattr`]: crate::mount::mount_setattr
#[repr(transparent)]
#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
pub struct MountAttrFlags: c::c_uint {
Expand Down Expand Up @@ -330,3 +332,13 @@ bitflags! {

#[repr(transparent)]
pub(crate) struct MountFlagsArg(pub(crate) c::c_uint);

#[repr(C)]
#[derive(Debug, Copy, Clone)]
#[allow(missing_docs)]
pub struct MountAttr<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct should be non_exhaustive.

pub attr_set: MountAttrFlags,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

pub attr_clr: MountAttrFlags,
pub propagation: MountPropagationFlags,
pub userns_fd: BorrowedFd<'a>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }

    // ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,
}

Copy link
Contributor

@ayosec ayosec Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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):

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

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,
        )
    }
}

}
24 changes: 24 additions & 0 deletions src/mount/misc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//! Miscellaneous mount APIs

use crate::backend::mount::types::MountAttr;
use crate::fd::BorrowedFd;
use crate::fs::AtFlags;
use crate::{backend, io, path};

/// `mount_setattr(dir_fd, path, flags, mount_attr)`
sunfishcode marked this conversation as resolved.
Show resolved Hide resolved
///
/// # References
/// - [Linux]
///
/// [Linux]: https://man7.org/linux/man-pages/man2/mount_setattr.2.html
#[inline]
pub fn mount_setattr<Path: path::Arg>(
dir_fd: BorrowedFd<'_>,
path: Path,
flags: AtFlags,
mount_attr: &MountAttr<'_>,
) -> io::Result<()> {
path.into_with_c_str(|path| {
backend::mount::syscalls::mount_setattr(dir_fd, path, flags, mount_attr)
})
}
2 changes: 2 additions & 0 deletions src/mount/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@

#[cfg(feature = "mount")]
mod fsopen;
mod misc;
mod mount_unmount;
mod types;

#[cfg(feature = "mount")]
pub use fsopen::*;
pub use misc::*;
pub use mount_unmount::*;
pub use types::*;
Loading