| Message ID | 20231018122518.128049-2-wedsonaf@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust abstractions for VFS | expand |
On 18.10.23 14:25, Wedson Almeida Filho wrote: > From: Wedson Almeida Filho <walmeida@microsoft.com> > > Allow basic registration and unregistration of Rust file system types. > Unregistration happens automatically when a registration variable is > dropped (e.g., when it goes out of scope). > > File systems registered this way are visible in `/proc/filesystems` but > cannot be mounted yet because `init_fs_context` fails. > > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/error.rs | 2 - > rust/kernel/fs.rs | 80 +++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 4 files changed, 82 insertions(+), 2 deletions(-) > create mode 100644 rust/kernel/fs.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 3b620ae07021..9c23037b33d0 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -8,6 +8,7 @@ > > #include <kunit/test.h> > #include <linux/errname.h> > +#include <linux/fs.h> > #include <linux/slab.h> > #include <linux/refcount.h> > #include <linux/wait.h> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs > index 05fcab6abfe6..e6d7ce46be55 100644 > --- a/rust/kernel/error.rs > +++ b/rust/kernel/error.rs > @@ -320,8 +320,6 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> { > /// }) > /// } > /// ``` > -// TODO: Remove `dead_code` marker once an in-kernel client is available. > -#[allow(dead_code)] > pub(crate) fn from_result<T, F>(f: F) -> T > where > T: From<i16>, > diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs > new file mode 100644 > index 000000000000..f3fb09db41ba > --- /dev/null > +++ b/rust/kernel/fs.rs > @@ -0,0 +1,80 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Kernel file systems. > +//! > +//! This module allows Rust code to register new kernel file systems. > +//! > +//! C headers: [`include/linux/fs.h`](../../include/linux/fs.h) > + > +use crate::error::{code::*, from_result, to_result, Error}; > +use crate::types::Opaque; > +use crate::{bindings, init::PinInit, str::CStr, try_pin_init, ThisModule}; > +use core::{marker::PhantomPinned, pin::Pin}; > +use macros::{pin_data, pinned_drop}; > + > +/// A file system type. > +pub trait FileSystem { > + /// The name of the file system type. > + const NAME: &'static CStr; > +} > + > +/// A registration of a file system. > +#[pin_data(PinnedDrop)] > +pub struct Registration { > + #[pin] > + fs: Opaque<bindings::file_system_type>, > + #[pin] > + _pin: PhantomPinned, Note that since commit 0b4e3b6f6b79 ("rust: types: make `Opaque` be `!Unpin`") you do not need an extra pinned `PhantomPinned` in your struct (if you already have a pinned `Opaque`), since `Opaque` already is `!Unpin`. > +} > + > +// SAFETY: `Registration` doesn't provide any `&self` methods, so it is safe to pass references > +// to it around. > +unsafe impl Sync for Registration {} > + > +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed > +// from any thread, so `Registration` is `Send`. > +unsafe impl Send for Registration {} > + > +impl Registration { > + /// Creates the initialiser of a new file system registration. > + pub fn new<T: FileSystem + ?Sized>(module: &'static ThisModule) -> impl PinInit<Self, Error> { I am a bit curious why you specify `?Sized` here, is it common for types that implement `FileSystem` to not be `Sized`? Or do you want to use `dyn FileSystem`? > + try_pin_init!(Self { > + _pin: PhantomPinned, > + fs <- Opaque::try_ffi_init(|fs_ptr: *mut bindings::file_system_type| { > + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write. > + unsafe { fs_ptr.write(bindings::file_system_type::default()) }; > + > + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write, and it has > + // just been initialised above, so it's also valid for read. > + let fs = unsafe { &mut *fs_ptr }; > + fs.owner = module.0; > + fs.name = T::NAME.as_char_ptr(); > + fs.init_fs_context = Some(Self::init_fs_context_callback); > + fs.kill_sb = Some(Self::kill_sb_callback); > + fs.fs_flags = 0; > + > + // SAFETY: Pointers stored in `fs` are static so will live for as long as the > + // registration is active (it is undone in `drop`). > + to_result(unsafe { bindings::register_filesystem(fs_ptr) }) > + }), > + }) > + } > + > + unsafe extern "C" fn init_fs_context_callback( > + _fc_ptr: *mut bindings::fs_context, > + ) -> core::ffi::c_int { > + from_result(|| Err(ENOTSUPP)) > + } > + > + unsafe extern "C" fn kill_sb_callback(_sb_ptr: *mut bindings::super_block) {} > +} > + > +#[pinned_drop] > +impl PinnedDrop for Registration { > + fn drop(self: Pin<&mut Self>) { > + // SAFETY: If an instance of `Self` has been successfully created, a call to > + // `register_filesystem` has necessarily succeeded. So it's ok to call > + // `unregister_filesystem` on the previously registered fs. I would simply add an invariant on `Registration` that `self.fs` is registered, then you do not need such a lengthy explanation here.
On Wed, 18 Oct 2023 at 12:38, Benno Lossin <benno.lossin@proton.me> wrote: > On 18.10.23 14:25, Wedson Almeida Filho wrote: > > +/// A registration of a file system. > > +#[pin_data(PinnedDrop)] > > +pub struct Registration { > > + #[pin] > > + fs: Opaque<bindings::file_system_type>, > > + #[pin] > > + _pin: PhantomPinned, > > Note that since commit 0b4e3b6f6b79 ("rust: types: make `Opaque` be > `!Unpin`") you do not need an extra pinned `PhantomPinned` in your struct > (if you already have a pinned `Opaque`), since `Opaque` already is > `!Unpin`. Will remove in v2. > > +impl Registration { > > + /// Creates the initialiser of a new file system registration. > > + pub fn new<T: FileSystem + ?Sized>(module: &'static ThisModule) -> impl PinInit<Self, Error> { > > I am a bit curious why you specify `?Sized` here, is it common > for types that implement `FileSystem` to not be `Sized`? > > Or do you want to use `dyn FileSystem`? No reason beyond `Sized` being a restriction I don't need. For something I was doing early on in binder, I ended up having to change a bunch of generic type decls to allow !Sized, so here I'm doing it preemptively as I don't lose anything. > > + try_pin_init!(Self { > > + _pin: PhantomPinned, > > + fs <- Opaque::try_ffi_init(|fs_ptr: *mut bindings::file_system_type| { > > + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write. > > + unsafe { fs_ptr.write(bindings::file_system_type::default()) }; > > + > > + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write, and it has > > + // just been initialised above, so it's also valid for read. > > + let fs = unsafe { &mut *fs_ptr }; > > + fs.owner = module.0; > > + fs.name = T::NAME.as_char_ptr(); > > + fs.init_fs_context = Some(Self::init_fs_context_callback); > > + fs.kill_sb = Some(Self::kill_sb_callback); > > + fs.fs_flags = 0; > > + > > + // SAFETY: Pointers stored in `fs` are static so will live for as long as the > > + // registration is active (it is undone in `drop`). > > + to_result(unsafe { bindings::register_filesystem(fs_ptr) }) > > + }), > > + }) > > + } > > + > > + unsafe extern "C" fn init_fs_context_callback( > > + _fc_ptr: *mut bindings::fs_context, > > + ) -> core::ffi::c_int { > > + from_result(|| Err(ENOTSUPP)) > > + } > > + > > + unsafe extern "C" fn kill_sb_callback(_sb_ptr: *mut bindings::super_block) {} > > +} > > + > > +#[pinned_drop] > > +impl PinnedDrop for Registration { > > + fn drop(self: Pin<&mut Self>) { > > + // SAFETY: If an instance of `Self` has been successfully created, a call to > > + // `register_filesystem` has necessarily succeeded. So it's ok to call > > + // `unregister_filesystem` on the previously registered fs. > > I would simply add an invariant on `Registration` that `self.fs` is > registered, then you do not need such a lengthy explanation here. Since this is the only place I need this explanation, I prefer to leave it here because it's exactly where I need it. Thanks, -Wedson
On 10.01.24 19:32, Wedson Almeida Filho wrote: >>> +#[pinned_drop] >>> +impl PinnedDrop for Registration { >>> + fn drop(self: Pin<&mut Self>) { >>> + // SAFETY: If an instance of `Self` has been successfully created, a call to >>> + // `register_filesystem` has necessarily succeeded. So it's ok to call >>> + // `unregister_filesystem` on the previously registered fs. >> >> I would simply add an invariant on `Registration` that `self.fs` is >> registered, then you do not need such a lengthy explanation here. > > Since this is the only place I need this explanation, I prefer to > leave it here because it's exactly where I need it. I get why you want this, but consider this: someone adds a another `new` function, but forgets to call `register_filesystem`. They have no indication except for this comment in the `Drop` impl, that they are doing something wrong. I took a look at the implement ion of `unregister_filesystem` and found that you can pass an unregistered filesystem, in that case the function just returns an error. I think the only safety requirement of `unregister_filesystem` is that if the supplied pointer is a registered filesystem, the pointee is valid.
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 3b620ae07021..9c23037b33d0 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -8,6 +8,7 @@ #include <kunit/test.h> #include <linux/errname.h> +#include <linux/fs.h> #include <linux/slab.h> #include <linux/refcount.h> #include <linux/wait.h> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 05fcab6abfe6..e6d7ce46be55 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -320,8 +320,6 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> { /// }) /// } /// ``` -// TODO: Remove `dead_code` marker once an in-kernel client is available. -#[allow(dead_code)] pub(crate) fn from_result<T, F>(f: F) -> T where T: From<i16>, diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs new file mode 100644 index 000000000000..f3fb09db41ba --- /dev/null +++ b/rust/kernel/fs.rs @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Kernel file systems. +//! +//! This module allows Rust code to register new kernel file systems. +//! +//! C headers: [`include/linux/fs.h`](../../include/linux/fs.h) + +use crate::error::{code::*, from_result, to_result, Error}; +use crate::types::Opaque; +use crate::{bindings, init::PinInit, str::CStr, try_pin_init, ThisModule}; +use core::{marker::PhantomPinned, pin::Pin}; +use macros::{pin_data, pinned_drop}; + +/// A file system type. +pub trait FileSystem { + /// The name of the file system type. + const NAME: &'static CStr; +} + +/// A registration of a file system. +#[pin_data(PinnedDrop)] +pub struct Registration { + #[pin] + fs: Opaque<bindings::file_system_type>, + #[pin] + _pin: PhantomPinned, +} + +// SAFETY: `Registration` doesn't provide any `&self` methods, so it is safe to pass references +// to it around. +unsafe impl Sync for Registration {} + +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed +// from any thread, so `Registration` is `Send`. +unsafe impl Send for Registration {} + +impl Registration { + /// Creates the initialiser of a new file system registration. + pub fn new<T: FileSystem + ?Sized>(module: &'static ThisModule) -> impl PinInit<Self, Error> { + try_pin_init!(Self { + _pin: PhantomPinned, + fs <- Opaque::try_ffi_init(|fs_ptr: *mut bindings::file_system_type| { + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write. + unsafe { fs_ptr.write(bindings::file_system_type::default()) }; + + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write, and it has + // just been initialised above, so it's also valid for read. + let fs = unsafe { &mut *fs_ptr }; + fs.owner = module.0; + fs.name = T::NAME.as_char_ptr(); + fs.init_fs_context = Some(Self::init_fs_context_callback); + fs.kill_sb = Some(Self::kill_sb_callback); + fs.fs_flags = 0; + + // SAFETY: Pointers stored in `fs` are static so will live for as long as the + // registration is active (it is undone in `drop`). + to_result(unsafe { bindings::register_filesystem(fs_ptr) }) + }), + }) + } + + unsafe extern "C" fn init_fs_context_callback( + _fc_ptr: *mut bindings::fs_context, + ) -> core::ffi::c_int { + from_result(|| Err(ENOTSUPP)) + } + + unsafe extern "C" fn kill_sb_callback(_sb_ptr: *mut bindings::super_block) {} +} + +#[pinned_drop] +impl PinnedDrop for Registration { + fn drop(self: Pin<&mut Self>) { + // SAFETY: If an instance of `Self` has been successfully created, a call to + // `register_filesystem` has necessarily succeeded. So it's ok to call + // `unregister_filesystem` on the previously registered fs. + unsafe { bindings::unregister_filesystem(self.fs.get()) }; + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 187d58f906a5..00059b80c240 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -34,6 +34,7 @@ mod allocator; mod build_assert; pub mod error; +pub mod fs; pub mod init; pub mod ioctl; #[cfg(CONFIG_KUNIT)]