Message-ID: <CAGSQo005hmZCJNBUJE+oJ9NH7e9wCALaGYLGN-DL_Du7+9K0YQ@mail.gmail.com>
Date: Thu, 1 May 2025 09:09:27 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Sami Tolvanen <samitolvanen@...gle.com>, Timur Tabi <ttabi@...dia.com>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display

On Thu, May 1, 2025 at 3:37 AM Danilo Krummrich <dakr@...nel.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@...gle.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.

Powered by blists - more mailing lists