[PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display

Matthew Maurer posted 4 patches 9 months, 1 week ago
There is a newer version of this series
[PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
Posted by Matthew Maurer 9 months, 1 week ago
Allows creation of files for references that live forever and lack
metadata through the `Display` implementation.

The reference must live forever because we do not have a maximum
lifetime for the file we are creating.

The `Display` implementation is used because `seq_printf` needs to route
through `%pA`, which in turn routes through Arguments. A more generic
API is provided later in the series, implemented in terms of this one.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 rust/kernel/debugfs.rs | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index b533ab21aaa775d4e3f33caf89e2d67ef85592f8..87de94da3b27c2a399bb377afd47280f65208d41 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -7,6 +7,7 @@
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
 use crate::str::CStr;
+use core::fmt::Display;
 
 /// Handle to a DebugFS directory.
 // INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`
@@ -121,6 +122,47 @@ fn as_ptr(&self) -> *mut bindings::dentry {
     pub fn keep(self) {
         core::mem::forget(self)
     }
+
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
+    /// [`Display::fmt`] on the provided reference.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
+    /// dir.display_file(c_str!("foo"), &200).keep();
+    /// // "my_debugfs_dir/foo" now contains the number 200.
+    /// ```
+    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> Self {
+        // SAFETY:
+        // * `name` is a NUL-terminated C string, living across the call, by CStr invariant
+        // * `parent` is a live dentry since we have a reference to it
+        // * `vtable` is all stock `seq_file` implementations except for `open`.
+        //   `open`'s only requirement beyond what is provided to all open functions is that the
+        //   inode's data pointer must point to a `T` that will outlive it, which we know because
+        //   we have a static reference.
+        // * debugfs_create_file_full either returns an error code or a legal dentry pointer, so
+        //   `Self::from_ptr` is safe to call here.
+        #[cfg(CONFIG_DEBUG_FS)]
+        unsafe {
+            Self::from_ptr(bindings::debugfs_create_file_full(
+                name.as_char_ptr(),
+                0o444,
+                self.as_ptr(),
+                data as *const _ as *mut _,
+                core::ptr::null(),
+                &<T as DisplayFile>::VTABLE,
+            ))
+        }
+        #[cfg(not(CONFIG_DEBUG_FS))]
+        {
+            // Mark parameters used
+            let (_, _) = (name, data);
+            Self()
+        }
+    }
 }
 
 impl Drop for Dir {
@@ -133,3 +175,63 @@ fn drop(&mut self) {
         }
     }
 }
+
+#[cfg(CONFIG_DEBUG_FS)]
+mod helpers {
+    use crate::seq_file::SeqFile;
+    use crate::seq_print;
+    use core::fmt::Display;
+
+    /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`
+    ///
+    /// # Safety
+    /// * inode's private pointer must point to a value of type T which will outlive the inode and
+    ///   will not be mutated during this call
+    /// * file must point to a live, not-yet-initialized file object
+    pub(crate) unsafe extern "C" fn display_open<T: Display>(
+        inode: *mut bindings::inode,
+        file: *mut bindings::file,
+    ) -> i32 {
+        // SAFETY:
+        // * file is acceptable by caller precondition
+        // * print_act will be called on a seq_file with private data set to the third argument,
+        //   so we meet its safety requirements
+        // * The data pointer passed in the third argument is a valid T pointer that outlives this
+        //   call by caller preconditions
+        unsafe { bindings::single_open(file, Some(display_act::<T>), (*inode).i_private) }
+    }
+
+    /// Prints private data stashed in a seq_file to that seq file
+    /// # Safety
+    /// `seq` must point to a live `seq_file` whose private data is a live pointer to a `T` which is
+    /// not being mutated.
+    pub(crate) unsafe extern "C" fn display_act<T: Display>(
+        seq: *mut bindings::seq_file,
+        _: *mut core::ffi::c_void,
+    ) -> i32 {
+        // SAFETY: By caller precondition, this pointer is live, points to a value of type T, and is
+        // not being mutated.
+        let data = unsafe { &*((*seq).private as *mut T) };
+        // SAFETY: By caller precondition, seq_file points to a live seq_file, so we can lift it
+        let seq_file = unsafe { SeqFile::from_raw(seq) };
+        seq_print!(seq_file, "{}", data);
+        0
+    }
+
+    // Work around lack of generic const items
+    pub(crate) trait DisplayFile: Display + Sized {
+        const VTABLE: bindings::file_operations = bindings::file_operations {
+            read: Some(bindings::seq_read),
+            llseek: Some(bindings::seq_lseek),
+            release: Some(bindings::single_release),
+            open: Some(display_open::<Self> as _),
+            // SAFETY: file_operations supports zeroes in all fields
+            ..unsafe { core::mem::zeroed() }
+        };
+    }
+
+    impl<T: Display + Sized> DisplayFile for T {}
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+use helpers::*;

-- 
2.49.0.906.g1f30a19c02-goog
Re: [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
Posted by Danilo Krummrich 9 months, 1 week ago
On Wed, Apr 30, 2025 at 11:31:57PM +0000, Matthew Maurer wrote:
> Allows creation of files for references that live forever and lack
> metadata through the `Display` implementation.
> 
> The reference must live forever because we do not have a maximum
> lifetime for the file we are creating.
> 
> The `Display` implementation is used because `seq_printf` needs to route
> through `%pA`, which in turn routes through Arguments. A more generic
> API is provided later in the series, implemented in terms of this one.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  rust/kernel/debugfs.rs | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index b533ab21aaa775d4e3f33caf89e2d67ef85592f8..87de94da3b27c2a399bb377afd47280f65208d41 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -7,6 +7,7 @@
>  //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
>  
>  use crate::str::CStr;
> +use core::fmt::Display;
>  
>  /// Handle to a DebugFS directory.
>  // INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`
> @@ -121,6 +122,47 @@ fn as_ptr(&self) -> *mut bindings::dentry {
>      pub fn keep(self) {
>          core::mem::forget(self)
>      }
> +
> +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> +    /// [`Display::fmt`] on the provided reference.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
> +    /// dir.display_file(c_str!("foo"), &200).keep();
> +    /// // "my_debugfs_dir/foo" now contains the number 200.
> +    /// ```
> +    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> Self {
> +        // SAFETY:
> +        // * `name` is a NUL-terminated C string, living across the call, by CStr invariant
> +        // * `parent` is a live dentry since we have a reference to it
> +        // * `vtable` is all stock `seq_file` implementations except for `open`.
> +        //   `open`'s only requirement beyond what is provided to all open functions is that the
> +        //   inode's data pointer must point to a `T` that will outlive it, which we know because
> +        //   we have a static reference.
> +        // * debugfs_create_file_full either returns an error code or a legal dentry pointer, so
> +        //   `Self::from_ptr` is safe to call here.
> +        #[cfg(CONFIG_DEBUG_FS)]
> +        unsafe {
> +            Self::from_ptr(bindings::debugfs_create_file_full(
> +                name.as_char_ptr(),
> +                0o444,
> +                self.as_ptr(),
> +                data as *const _ as *mut _,
> +                core::ptr::null(),
> +                &<T as DisplayFile>::VTABLE,
> +            ))
> +        }
> +        #[cfg(not(CONFIG_DEBUG_FS))]
> +        {
> +            // Mark parameters used
> +            let (_, _) = (name, data);
> +            Self()
> +        }
> +    }

Analogous to SubDir, this should be a new type, such that we can't leak the root
directory. Also, methods like subdir() don't really make sense for a file, no?

Besides that, don't we also need a separate type for a file to be able to attach
non-static data anyways? I.e. something like:

	#[cfg(CONFIG_DEBUG_FS)]
	struct File<T> {
	   dentry: *mut bindings::dentry,
	   data: T,
	}

	#[cfg(not(CONFIG_DEBUG_FS))]
	struct File<T> {
	   _p: PhantomData<T>,
	}

I'm not exactly sure how v1 did this; I haven't had time to look at v1 before v2
was posted. I seems like v1 relied on a separate structure storing the data,
which also held a reference to the corresponding dentry or something along those
lines?
Re: [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
Posted by Matthew Maurer 9 months, 1 week ago
On Thu, May 1, 2025 at 3:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Apr 30, 2025 at 11:31:57PM +0000, Matthew Maurer wrote:
> > Allows creation of files for references that live forever and lack
> > metadata through the `Display` implementation.
> >
> > The reference must live forever because we do not have a maximum
> > lifetime for the file we are creating.
> >
> > The `Display` implementation is used because `seq_printf` needs to route
> > through `%pA`, which in turn routes through Arguments. A more generic
> > API is provided later in the series, implemented in terms of this one.
> >
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > ---
> >  rust/kernel/debugfs.rs | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> >
> > diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> > index b533ab21aaa775d4e3f33caf89e2d67ef85592f8..87de94da3b27c2a399bb377afd47280f65208d41 100644
> > --- a/rust/kernel/debugfs.rs
> > +++ b/rust/kernel/debugfs.rs
> > @@ -7,6 +7,7 @@
> >  //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
> >
> >  use crate::str::CStr;
> > +use core::fmt::Display;
> >
> >  /// Handle to a DebugFS directory.
> >  // INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`
> > @@ -121,6 +122,47 @@ fn as_ptr(&self) -> *mut bindings::dentry {
> >      pub fn keep(self) {
> >          core::mem::forget(self)
> >      }
> > +
> > +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> > +    /// [`Display::fmt`] on the provided reference.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
> > +    /// dir.display_file(c_str!("foo"), &200).keep();
> > +    /// // "my_debugfs_dir/foo" now contains the number 200.
> > +    /// ```
> > +    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> Self {
> > +        // SAFETY:
> > +        // * `name` is a NUL-terminated C string, living across the call, by CStr invariant
> > +        // * `parent` is a live dentry since we have a reference to it
> > +        // * `vtable` is all stock `seq_file` implementations except for `open`.
> > +        //   `open`'s only requirement beyond what is provided to all open functions is that the
> > +        //   inode's data pointer must point to a `T` that will outlive it, which we know because
> > +        //   we have a static reference.
> > +        // * debugfs_create_file_full either returns an error code or a legal dentry pointer, so
> > +        //   `Self::from_ptr` is safe to call here.
> > +        #[cfg(CONFIG_DEBUG_FS)]
> > +        unsafe {
> > +            Self::from_ptr(bindings::debugfs_create_file_full(
> > +                name.as_char_ptr(),
> > +                0o444,
> > +                self.as_ptr(),
> > +                data as *const _ as *mut _,
> > +                core::ptr::null(),
> > +                &<T as DisplayFile>::VTABLE,
> > +            ))
> > +        }
> > +        #[cfg(not(CONFIG_DEBUG_FS))]
> > +        {
> > +            // Mark parameters used
> > +            let (_, _) = (name, data);
> > +            Self()
> > +        }
> > +    }
>
> Analogous to SubDir, this should be a new type, such that we can't leak the root
> directory. Also, methods like subdir() don't really make sense for a file, no?
I agree, v3 will have a `File` type which:
1. Doesn't auto-discard the file, but provides a method to discard it.
2. Doesn't deref to `Dir`, so you can't call subdir.
>
> Besides that, don't we also need a separate type for a file to be able to attach
> non-static data anyways? I.e. something like:
>
>         #[cfg(CONFIG_DEBUG_FS)]
>         struct File<T> {
>            dentry: *mut bindings::dentry,
>            data: T,
>         }
>
>         #[cfg(not(CONFIG_DEBUG_FS))]
>         struct File<T> {
>            _p: PhantomData<T>,
>         }
>
> I'm not exactly sure how v1 did this; I haven't had time to look at v1 before v2
> was posted. I seems like v1 relied on a separate structure storing the data,
> which also held a reference to the corresponding dentry or something along those
> lines?
In v1, this was done via
```
#[pin_data]
struct Values<T> {
  dir: /* ignore this type */,
  #[pin]
  backing: T
}
```
Then, there was an interface that let the user provide a building
function which had to have a fully polymorphic lifetime, which would
be passed a backing reference that it was allowed to attach to
subdirectory files. Since the dir would be cleaned up before the
backing went away, we could know that it successfully outlived it.
It'll probably look a little different when I send the follow-up
series on top of this one.

Attaching to the root directory rather than each individual file made
sense to me because this meant that if you had
```
struct Foo {
  prop_a: u32,
  prop_b: u32
}
```
it would not be as tricky to attach `prop_a` to one file and `prop_b`
to another, because the directory would own `Foo`. This'll probably be
clearer when I send up a dependent series on top of v3 later today.
Re: [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
Posted by Danilo Krummrich 9 months, 1 week ago
On Thu, May 01, 2025 at 09:09:27AM -0700, Matthew Maurer wrote:
> On Thu, May 1, 2025 at 3:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > On Wed, Apr 30, 2025 at 11:31:57PM +0000, Matthew Maurer wrote:
> > Besides that, don't we also need a separate type for a file to be able to attach
> > non-static data anyways? I.e. something like:
> >
> >         #[cfg(CONFIG_DEBUG_FS)]
> >         struct File<T> {
> >            dentry: *mut bindings::dentry,
> >            data: T,
> >         }
> >
> >         #[cfg(not(CONFIG_DEBUG_FS))]
> >         struct File<T> {
> >            _p: PhantomData<T>,
> >         }
> >
> > I'm not exactly sure how v1 did this; I haven't had time to look at v1 before v2
> > was posted. I seems like v1 relied on a separate structure storing the data,
> > which also held a reference to the corresponding dentry or something along those
> > lines?
> In v1, this was done via
> ```
> #[pin_data]
> struct Values<T> {
>   dir: /* ignore this type */,
>   #[pin]
>   backing: T
> }
> ```
> Then, there was an interface that let the user provide a building
> function which had to have a fully polymorphic lifetime, which would
> be passed a backing reference that it was allowed to attach to
> subdirectory files. Since the dir would be cleaned up before the
> backing went away, we could know that it successfully outlived it.
> It'll probably look a little different when I send the follow-up
> series on top of this one.
> 
> Attaching to the root directory rather than each individual file made
> sense to me because this meant that if you had
> ```
> struct Foo {
>   prop_a: u32,
>   prop_b: u32
> }
> ```
> it would not be as tricky to attach `prop_a` to one file and `prop_b`
> to another, because the directory would own `Foo`.

Thanks for the explanation! I need to think a bit more about this approach.

> This'll probably be
> clearer when I send up a dependent series on top of v3 later today.

At least from my side, no need to rush. :) I'll have quite limited bandwidth for
the next ~10 days anyways.