diff mbox series

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

Message ID 20230726164535.230515-4-amiculas@cisco.com
State New
Headers
Series Rust PuzleFS filesystem driver |

Commit Message

Ariel Miculas July 26, 2023, 4:45 p.m. UTC 
  Signed-off-by: Ariel Miculas <amiculas@cisco.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/lib.rs              |  1 +
 rust/kernel/mount.rs            | 71 +++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+)
 create mode 100644 rust/kernel/mount.rs

Comments

Trevor Gross July 26, 2023, 10:34 p.m. UTC | #1 
On Wed, Jul 26, 2023 at 12:54 PM Ariel Miculas <amiculas@cisco.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
>
>
Ariel Miculas July 27, 2023, 1:10 p.m. UTC | #2 
Thanks, I've applied your suggestions on
https://github.com/ariel-miculas/linux/tree/puzzlefs_rfc
diff mbox series

Patch

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index e5fc3b1d408d..205ef50dfd4c 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -18,6 +18,7 @@ 
 #include <linux/uio.h>
 #include <linux/uaccess.h>
 #include <linux/delay.h>
+#include <linux/namei.h>
 
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 08f67833afcf..7dc07bdb5d55 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -46,6 +46,7 @@ 
 pub mod ioctl;
 pub mod iov_iter;
 pub mod mm;
+pub mod mount;
 pub mod pages;
 pub mod prelude;
 pub mod print;
diff --git a/rust/kernel/mount.rs b/rust/kernel/mount.rs
new file mode 100644
index 000000000000..d08830b27571
--- /dev/null
+++ b/rust/kernel/mount.rs
@@ -0,0 +1,71 @@ 
+//! Mount interface
+//!
+//! C headers: [`include/linux/mount.h`](../../../../include/linux/mount.h)
+
+use kernel::bindings;
+use kernel::error::from_err_ptr;
+use kernel::pr_err;
+use kernel::prelude::*;
+use kernel::str::CStr;
+use kernel::types::Opaque;
+
+/// Wraps the kernel's `struct path`.
+#[repr(transparent)]
+pub struct Path(pub(crate) Opaque<bindings::path>);
+
+/// Wraps the kernel's `struct vfsmount`.
+#[repr(transparent)]
+#[derive(Debug)]
+pub struct Vfsmount {
+    vfsmount: *mut bindings::vfsmount,
+}
+
+// SAFETY: No one besides us has the raw pointer, so we can safely transfer Vfsmount to another thread
+unsafe impl Send for Vfsmount {}
+// SAFETY: It's OK to access `Vfsmount` through references from other threads because we're not
+// accessing any properties from the underlying raw pointer
+unsafe impl Sync for Vfsmount {}
+
+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(),
+            )
+        };
+        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;
+        }
+        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) };
+    }
+}