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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Jul 2023 18:34:09 -0400
From:   Trevor Gross <tmgross@...ch.edu>
To:     Ariel Miculas <amiculas@...co.com>
Cc:     rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, tycho@...ho.pizza,
        brauner@...nel.org, viro@...iv.linux.org.uk, ojeda@...nel.org,
        alex.gaynor@...il.com, wedsonaf@...il.com
Subject: Re: [RFC PATCH v2 03/10] rust: kernel: add an abstraction over
 vfsmount to allow cloning a new private mount

On Wed, Jul 26, 2023 at 12:54 PM Ariel Miculas <amiculas@...co.com> 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
>
>