Re: [RFC PATCH v2 03/10] rust: kernel: add an abstraction over vfsmount to allow cloning a new private mount

[Trevor Gross]

On Wed, Jul 26, 2023 at 12:54 PM Ariel Miculas <amiculas@xxxxxxxxx> wrote:
> [...]
> +++ b/rust/kernel/mount.rs
> @@ -0,0 +1,71 @@
> [...]
> +impl Vfsmount {
> +    /// Create a new private mount clone based on a path name
> +    pub fn new_private_mount(path_name: &CStr) -> Result<Self> {
> +        let path: Path = Path(Opaque::uninit());
> +        // SAFETY: path_name is a &CStr, so it's a valid string pointer; path is an uninitialized
> +        // struct stored on the stack and it's ok because kern_path expects an out parameter
> +        let err = unsafe {
> +            bindings::kern_path(
> +                path_name.as_ptr() as *const i8,
> +                bindings::LOOKUP_FOLLOW | bindings::LOOKUP_DIRECTORY,
> +                path.0.get(),
> +            )
> +        };

Nit: I believe that `.cast::<i8>()` is preferred over `as *const T`
because it makes it harder to
accidentally change constness or indirection level. This isn't always
followed in kernel, but
good practice.

> +        if err != 0 {
> +            pr_err!("failed to resolve '{}': {}\n", path_name, err);
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: path is a struct stored on the stack and it is  initialized because the call to
> +        // kern_path succeeded
> +        let vfsmount = unsafe { from_err_ptr(bindings::clone_private_mount(path.0.get()))? };
> +
> +        // Don't inherit atime flags
> +        // SAFETY: we called from_err_ptr so it's safe to dereference this pointer
> +        unsafe {
> +            (*vfsmount).mnt_flags &=
> +                !(bindings::MNT_NOATIME | bindings::MNT_NODIRATIME | bindings::MNT_RELATIME) as i32;
> +        }

Nit: it looks a bit cleaner to OR the flags outside of the unsafe
block, which is also nice because then
the unsafe block only contains the unsafe ops (could do this above
too). Also - I think maybe style
might encourage `// CAST:` where `as` is used?

    let new_flags =
        !(bindings::MNT_NOATIME | bindings::MNT_NODIRATIME |
bindings::MNT_RELATIME) as i32;

    // SAFETY: we called from_err_ptr so it's safe to dereference this pointer
    unsafe { (*vfsmount).mnt_flags &= new_flags }

> +        Ok(Self { vfsmount })
> +    }
> +
> +    /// Returns a raw pointer to vfsmount
> +    pub fn get(&self) -> *mut bindings::vfsmount {
> +        self.vfsmount
> +    }
> +}
> +
> +impl Drop for Vfsmount {
> +    fn drop(&mut self) {
> +        // SAFETY new_private_mount makes sure to return a valid pointer
> +        unsafe { bindings::kern_unmount(self.vfsmount) };
> +    }
> +}
> --
> 2.41.0
>
>