Skip to content

Add mount_setattr #1002

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

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

Choose a reason for hiding this comment

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

It seems to me like the current struct-based approach was done as an implementation detail at the syscall level to allow extensibility in the future. I wouldn't see anything wrong with a simple implementation of this API using only (an admittedly large number of) function parameters: dirfd, pathname, flags, set, clear, propagation, userns.

We could even imagine specialized cases similar to what is done for other mount APIs (like how the mount syscall is exposed also as mount_remount, or there are many typesafe fsconfig variants).

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me like the current struct-based approach was done as an implementation detail at the syscall level to allow extensibility in the future. I wouldn't see anything wrong with a simple implementation of this API using only (an admittedly large number of) function parameters: dirfd, pathname, flags, set, clear, propagation, userns.

Well this would mean rustix needs a semver bump or a new function if the kernel adds a new field.

We could even imagine specialized cases similar to what is done for other mount APIs (like how the mount syscall is exposed also as mount_remount, or there are many typesafe fsconfig variants).

There is one difference, the mount functions you listed have an multiplexed interface that got demultiplexed by rustix. mount_setattr isn't a multiplexed syscall (in that form) so splitting would mean you need multiple syscalls instead of one.

Summarizing my thought, we should first decided whether we want extensibility in the future (w/o breaking) or not.

Copy link

Choose a reason for hiding this comment

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

I see it as a matter of pragmatism: this would be an easy way to resolve this issue in the short term...

Should the API ever change, then there would be a lot of options at that point, including adding a complex API that takes a struct...

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me like the current struct-based approach was done as an implementation detail at the syscall level to allow extensibility in the future.

An advantage of using a struct is that it is easier to omit default fields with dedicated builder functions. For example, with the draft in #1002 (comment), an ID-mapped mount could be created with:

use MountSetAttrFlags as Flags;

mount_setattr(
    dir_fd,
    c"", 
    Flags::RECURSIVE | Flags::EMPTY_PATH,
    MountAttr::new_with_idmap(userns),
)?;

Copy link
Contributor

Choose a reason for hiding this comment

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

  • openat2 has a future extensibility struct that rustix decided to inline into the function signature.
  • clone3 has a future extensibility struct but is impossible to be implemented by rustix.
  • mount_setattr, sched_setattr, landlock_create_ruleset, statmount/listmount, perf_event_open have a future extensibility struct and are not yet implemented by rustix.

So we are not dealing with a single syscall but also with the design future syscalls in rustix might follow.

}
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)`
///
/// # 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::*;