Received-SPF: pass (google.com: domain of linux-kernel+bounces-178735-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Wedson Almeida Filho <wedsonaf@gmail.com>
To: Alexander Viro <viro@zeniv.linux.org.uk>,
	Christian Brauner <brauner@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Dave Chinner <david@fromorbit.com>
Cc: Kent Overstreet <kent.overstreet@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-fsdevel@vger.kernel.org,
	rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Wedson Almeida Filho <walmeida@microsoft.com>
Subject: [RFC PATCH v2 22/30] rust: fs: add per-superblock data
Date: Tue, 14 May 2024 10:17:03 -0300
Message-Id: <20240514131711.379322-23-wedsonaf@gmail.com>
In-Reply-To: <20240514131711.379322-1-wedsonaf@gmail.com>
References: <20240514131711.379322-1-wedsonaf@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

From: Wedson Almeida Filho <walmeida@microsoft.com>

Allow Rust file systems to associate [typed] data to super blocks when
they're created. Since we only have a pointer-sized field in which to
store the state, it must implement the `ForeignOwnable` trait.

Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
 rust/kernel/fs.rs         | 46 ++++++++++++++++++++++++++++---------
 rust/kernel/fs/sb.rs      | 48 +++++++++++++++++++++++++++++++++++----
 samples/rust/rust_rofs.rs |  3 ++-
 3 files changed, 81 insertions(+), 16 deletions(-)

diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs
index 51de73008857..387e87e3edaf 100644
--- a/rust/kernel/fs.rs
+++ b/rust/kernel/fs.rs
@@ -7,7 +7,7 @@
 //! C headers: [`include/linux/fs.h`](srctree/include/linux/fs.h)
 
 use crate::error::{code::*, from_result, to_result, Error, Result};
-use crate::types::Opaque;
+use crate::types::{ForeignOwnable, Opaque};
 use crate::{bindings, init::PinInit, str::CStr, try_pin_init, ThisModule};
 use core::{ffi, marker::PhantomData, mem::ManuallyDrop, pin::Pin, ptr};
 use dentry::DEntry;
@@ -31,6 +31,9 @@
 
 /// A file system type.
 pub trait FileSystem {
+    /// Data associated with each file system instance (super-block).
+    type Data: ForeignOwnable + Send + Sync;
+
     /// The name of the file system type.
     const NAME: &'static CStr;
 
@@ -40,8 +43,8 @@ pub trait FileSystem {
     #[doc(hidden)]
     const IS_UNSPECIFIED: bool = false;
 
-    /// Initialises the new superblock.
-    fn fill_super(sb: &mut SuperBlock<Self>) -> Result;
+    /// Initialises the new superblock and returns the data to attach to it.
+    fn fill_super(sb: &mut SuperBlock<Self, sb::New>) -> Result<Self::Data>;
 
     /// Initialises and returns the root inode of the given superblock.
     ///
@@ -94,9 +97,10 @@ pub struct Stat {
 pub struct UnspecifiedFS;
 
 impl FileSystem for UnspecifiedFS {
+    type Data = ();
     const NAME: &'static CStr = crate::c_str!("unspecified");
     const IS_UNSPECIFIED: bool = true;
-    fn fill_super(_: &mut SuperBlock<Self>) -> Result {
+    fn fill_super(_: &mut SuperBlock<Self, sb::New>) -> Result {
         Err(ENOTSUPP)
     }
 
@@ -134,7 +138,7 @@ pub fn new<T: FileSystem + ?Sized>(module: &'static ThisModule) -> impl PinInit<
                 fs.owner = module.0;
                 fs.name = T::NAME.as_char_ptr();
                 fs.init_fs_context = Some(Self::init_fs_context_callback::<T>);
-                fs.kill_sb = Some(Self::kill_sb_callback);
+                fs.kill_sb = Some(Self::kill_sb_callback::<T>);
                 fs.fs_flags = 0;
 
                 // SAFETY: Pointers stored in `fs` are static so will live for as long as the
@@ -155,10 +159,22 @@ pub fn new<T: FileSystem + ?Sized>(module: &'static ThisModule) -> impl PinInit<
         })
     }
 
-    unsafe extern "C" fn kill_sb_callback(sb_ptr: *mut bindings::super_block) {
+    unsafe extern "C" fn kill_sb_callback<T: FileSystem + ?Sized>(
+        sb_ptr: *mut bindings::super_block,
+    ) {
         // SAFETY: In `get_tree_callback` we always call `get_tree_nodev`, so `kill_anon_super` is
         // the appropriate function to call for cleanup.
         unsafe { bindings::kill_anon_super(sb_ptr) };
+
+        // SAFETY: The C API contract guarantees that `sb_ptr` is valid for read.
+        let ptr = unsafe { (*sb_ptr).s_fs_info };
+        if !ptr.is_null() {
+            // SAFETY: The only place where `s_fs_info` is assigned is `NewSuperBlock::init`, where
+            // it's initialised with the result of an `into_foreign` call. We checked above that
+            // `ptr` is non-null because it would be null if we never reached the point where we
+            // init the field.
+            unsafe { T::Data::from_foreign(ptr) };
+        }
     }
 }
 
@@ -205,12 +221,19 @@ impl<T: FileSystem + ?Sized> Tables<T> {
             sb.s_xattr = &Tables::<T>::XATTR_HANDLERS[0];
             sb.s_flags |= bindings::SB_RDONLY;
 
-            T::fill_super(new_sb)?;
+            let data = T::fill_super(new_sb)?;
+
+            // N.B.: Even on failure, `kill_sb` is called and frees the data.
+            sb.s_fs_info = data.into_foreign().cast_mut();
 
-            let root = T::init_root(new_sb)?;
+            // SAFETY: The callback contract guarantees that `sb_ptr` is a unique pointer to a
+            // newly-created (and initialised above) superblock. And we have just initialised
+            // `s_fs_info`.
+            let sb = unsafe { SuperBlock::from_raw(sb_ptr) };
+            let root = T::init_root(sb)?;
 
             // Reject root inode if it belongs to a different superblock.
-            if !ptr::eq(root.super_block(), new_sb) {
+            if !ptr::eq(root.super_block(), sb) {
                 return Err(EINVAL);
             }
 
@@ -346,7 +369,7 @@ fn init(module: &'static ThisModule) -> impl PinInit<Self, Error> {
 ///
 /// ```
 /// # mod module_fs_sample {
-/// use kernel::fs::{dentry, inode::INode, sb::SuperBlock, self};
+/// use kernel::fs::{dentry, inode::INode, sb, sb::SuperBlock, self};
 /// use kernel::prelude::*;
 ///
 /// kernel::module_fs! {
@@ -359,8 +382,9 @@ fn init(module: &'static ThisModule) -> impl PinInit<Self, Error> {
 ///
 /// struct MyFs;
 /// impl fs::FileSystem for MyFs {
+///     type Data = ();
 ///     const NAME: &'static CStr = kernel::c_str!("myfs");
-///     fn fill_super(_: &mut SuperBlock<Self>) -> Result {
+///     fn fill_super(_: &mut SuperBlock<Self, sb::New>) -> Result {
 ///         todo!()
 ///     }
 ///     fn init_root(_sb: &SuperBlock<Self>) -> Result<dentry::Root<Self>> {
diff --git a/rust/kernel/fs/sb.rs b/rust/kernel/fs/sb.rs
index fa10f3db5593..7c0c52e6da0a 100644
--- a/rust/kernel/fs/sb.rs
+++ b/rust/kernel/fs/sb.rs
@@ -10,19 +10,37 @@
 use super::FileSystem;
 use crate::bindings;
 use crate::error::{code::*, Result};
-use crate::types::{ARef, Either, Opaque};
+use crate::types::{ARef, Either, ForeignOwnable, Opaque};
 use core::{marker::PhantomData, ptr};
 
+/// A typestate for [`SuperBlock`] that indicates that it's a new one, so not fully initialized
+/// yet.
+pub struct New;
+
+/// A typestate for [`SuperBlock`] that indicates that it's ready to be used.
+pub struct Ready;
+
+// SAFETY: Instances of `SuperBlock<T, Ready>` are only created after initialising the data.
+unsafe impl DataInited for Ready {}
+
+/// Indicates that a superblock in this typestate has data initialized.
+///
+/// # Safety
+///
+/// Implementers must ensure that `s_fs_info` is properly initialised in this state.
+#[doc(hidden)]
+pub unsafe trait DataInited {}
+
 /// A file system super block.
 ///
 /// Wraps the kernel's `struct super_block`.
 #[repr(transparent)]
-pub struct SuperBlock<T: FileSystem + ?Sized>(
+pub struct SuperBlock<T: FileSystem + ?Sized, S = Ready>(
     pub(crate) Opaque<bindings::super_block>,
-    PhantomData<T>,
+    PhantomData<(S, T)>,
 );
 
-impl<T: FileSystem + ?Sized> SuperBlock<T> {
+impl<T: FileSystem + ?Sized, S> SuperBlock<T, S> {
     /// Creates a new superblock reference from the given raw pointer.
     ///
     /// # Safety
@@ -31,6 +49,7 @@ impl<T: FileSystem + ?Sized> SuperBlock<T> {
     ///
     /// * `ptr` is valid and remains so for the lifetime of the returned object.
     /// * `ptr` has the correct file system type, or `T` is [`super::UnspecifiedFS`].
+    /// * `ptr` in the right typestate.
     pub(crate) unsafe fn from_raw<'a>(ptr: *mut bindings::super_block) -> &'a Self {
         // SAFETY: The safety requirements guarantee that the cast below is ok.
         unsafe { &*ptr.cast::<Self>() }
@@ -44,6 +63,7 @@ pub(crate) unsafe fn from_raw<'a>(ptr: *mut bindings::super_block) -> &'a Self {
     ///
     /// * `ptr` is valid and remains so for the lifetime of the returned object.
     /// * `ptr` has the correct file system type, or `T` is [`super::UnspecifiedFS`].
+    /// * `ptr` in the right typestate.
     /// * `ptr` is the only active pointer to the superblock.
     pub(crate) unsafe fn from_raw_mut<'a>(ptr: *mut bindings::super_block) -> &'a mut Self {
         // SAFETY: The safety requirements guarantee that the cast below is ok.
@@ -55,7 +75,9 @@ pub fn rdonly(&self) -> bool {
         // SAFETY: `s_flags` only changes during init, so it is safe to read it.
         unsafe { (*self.0.get()).s_flags & bindings::SB_RDONLY != 0 }
     }
+}
 
+impl<T: FileSystem + ?Sized> SuperBlock<T, New> {
     /// Sets the magic number of the superblock.
     pub fn set_magic(&mut self, magic: usize) -> &mut Self {
         // SAFETY: This is a new superblock that is being initialised, so it's ok to write to its
@@ -63,8 +85,26 @@ pub fn set_magic(&mut self, magic: usize) -> &mut Self {
         unsafe { (*self.0.get()).s_magic = magic as core::ffi::c_ulong };
         self
     }
+}
+
+impl<T: FileSystem + ?Sized, S: DataInited> SuperBlock<T, S> {
+    /// Returns the data associated with the superblock.
+    pub fn data(&self) -> <T::Data as ForeignOwnable>::Borrowed<'_> {
+        if T::IS_UNSPECIFIED {
+            crate::build_error!("super block data type is unspecified");
+        }
+
+        // SAFETY: This method is only available if the typestate implements `DataInited`, whose
+        // safety requirements include `s_fs_info` being properly initialised.
+        let ptr = unsafe { (*self.0.get()).s_fs_info };
+        unsafe { T::Data::borrow(ptr) }
+    }
 
     /// Tries to get an existing inode or create a new one if it doesn't exist yet.
+    ///
+    /// This method is not callable from a superblock where data isn't inited yet because it would
+    /// allow one to get access to the uninited data via `inode::New::init()` ->
+    /// `INode::super_block()` -> `SuperBlock::data()`.
     pub fn get_or_create_inode(&self, ino: Ino) -> Result<Either<ARef<INode<T>>, inode::New<T>>> {
         // SAFETY: All superblock-related state needed by `iget_locked` is initialised by C code
         // before calling `fill_super_callback`, or by `fill_super_callback` itself before calling
diff --git a/samples/rust/rust_rofs.rs b/samples/rust/rust_rofs.rs
index 7a09e2db878d..7027ca067f8f 100644
--- a/samples/rust/rust_rofs.rs
+++ b/samples/rust/rust_rofs.rs
@@ -98,9 +98,10 @@ fn iget(sb: &sb::SuperBlock<Self>, e: &'static Entry) -> Result<ARef<INode<Self>
 }
 
 impl fs::FileSystem for RoFs {
+    type Data = ();
     const NAME: &'static CStr = c_str!("rust_rofs");
 
-    fn fill_super(sb: &mut sb::SuperBlock<Self>) -> Result {
+    fn fill_super(sb: &mut sb::SuperBlock<Self, sb::New>) -> Result {
         sb.set_magic(0x52555354);
         Ok(())
     }
-- 
2.34.1