| Message ID | 20230726164535.230515-4-amiculas@cisco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust PuzleFS filesystem driver | expand |
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 > >
Thanks, I've applied your suggestions on https://github.com/ariel-miculas/linux/tree/puzzlefs_rfc
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) }; + } +}
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