[PATCH WIP 0/2] rust: debugfs: Support attaching data to DebugFS directories

[PATCH WIP 0/2] rust: debugfs: Support attaching data to DebugFS directories

[Matthew Maurer]

We can wait to properly review and land this until after we've decided
on the final interface for the first part, but since dakr@kernel.org was
poking at how this might work in his review of the previous patchset, I
wanted to upload this sketch as context.

The general concept here is that you select an owning directory and
attach data to it while converting its lifetime to be invariant (e.g.
can't be shortened) so that you know that the DebugFS contents will be
cleaned up before the data. You can then implement things underneath
that directory using the attached data.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
Matthew Maurer (2):
      rust: debugfs: Add interface to build debugfs off pinned objects
      rust: debugfs: Extend sample to use attached data

 rust/kernel/debugfs.rs       | 206 ++++++++++++++++++++++++++++++++++++++++---
 samples/rust/rust_debugfs.rs | 110 ++++++++++++++++++++++-
 2 files changed, 302 insertions(+), 14 deletions(-)
---
base-commit: 33035b665157558254b3c21c3f049fd728e72368
change-id: 20250501-debugfs-rust-attach-e164b67c9c16
prerequisite-change-id: 20250428-debugfs-rust-3cd5c97eb7d1:v4
prerequisite-patch-id: 7ac67017c11249cd04fc4beca6cfdb5b83aa89de
prerequisite-patch-id: 2e8256a6ef25afc95279e740f43d17ec169a65f2
prerequisite-patch-id: 0abd76fd2d71ba300d8a5ce5b3b61751feec88fa
prerequisite-patch-id: 5919cceb97187adfc71e9c1899f85d5af092bde6

Best regards,
-- 
Matthew Maurer <mmaurer@google.com>

[PATCH WIP 1/2] rust: debugfs: Add interface to build debugfs off pinned objects

[Matthew Maurer]

Previously, we could only expose `'static` references to DebugFS because
we don't know how long the file could live. This provides a way to
package data alongside an owning directory to guarantee that it will
live long enough to be accessed by the DebugFS files, while still
allowing a dynamic lifecycle.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index cfdff63c10517f1aebf757c965289a49fed6ae85..f232598b510e036bd25e6846f153572ebfb459f3 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -6,10 +6,14 @@
 //!
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
+use crate::prelude::PinInit;
 use crate::str::CStr;
 use core::fmt;
 use core::fmt::Display;
-use core::marker::PhantomData;
+use core::marker::{PhantomData, PhantomPinned};
+use core::ops::Deref;
+use core::pin::Pin;
+use pin_init::pin_data;
 
 /// Owning handle to a DebugFS directory.
 ///
@@ -158,6 +162,8 @@ pub fn fmt_file<'b, T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
         f: &'static F,
     ) -> File<'b> {
         // SAFETY: As `data` lives for the static lifetime, it outlives the file
+        // As we return `File<'b>`, and we have a borrow to a directory with lifetime 'b, we have a
+        // lower bound of `'b` on the directory.
         unsafe { self.fmt_file_raw(name, data, f) }
     }
 
@@ -169,11 +175,10 @@ pub fn fmt_file<'b, T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
     /// This means that before `data` may become invalid, either:
     /// * The refcount must go to zero.
     /// * The file must be rendered inaccessible, e.g. via `debugfs_remove`.
-    unsafe fn display_file_raw<'b, T: Display + Sized>(
-        &'b self,
-        name: &CStr,
-        data: *const T,
-    ) -> File<'b> {
+    /// * If `self` may take longer than `'a` to be destroyed, the caller is responsible for
+    ///   shortening the lifetime of the return value to a lower bound for the lifetime of that
+    ///   object.
+    unsafe fn display_file_raw<T: Display + Sized>(&self, name: &CStr, data: *const T) -> File<'a> {
         // 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.
@@ -215,13 +220,16 @@ unsafe fn display_file_raw<'b, T: Display + Sized>(
     ///
     /// # Safety
     ///
-    /// `data` must outlive the resulting file's accessibility
-    unsafe fn fmt_file_raw<'b, T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
-        &'b self,
+    /// * `data` must outlive the resulting file's accessibility
+    /// * If `self` may take longer than `'a` to be destroyed, the caller is responsible for
+    ///   shortening the lifetime of the return value to a lower bound for the lifetime of that
+    ///   object.
+    unsafe fn fmt_file_raw<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
         name: &CStr,
         data: &T,
         f: &'static F,
-    ) -> File<'b> {
+    ) -> File<'a> {
         #[cfg(CONFIG_DEBUG_FS)]
         let data_adapted = FormatAdapter::new(data, f);
         #[cfg(not(CONFIG_DEBUG_FS))]
@@ -303,6 +311,184 @@ pub fn remove(self) {
     }
 }
 
+/// A DebugFS directory combined with a backing store for data to implement it.
+#[pin_data]
+pub struct Values<T> {
+    dir: Dir<'static, false>,
+    // The order here is load-bearing - `dir` must be dropped before `backing`, as files under
+    // `dir` may point into `backing`.
+    #[pin]
+    backing: T,
+    // Since the files present under our directory may point into `backing`, we are `!Unpin`.
+    #[pin]
+    _pin: PhantomPinned,
+}
+
+impl<T> Deref for Values<T> {
+    type Target = T;
+    fn deref(&self) -> &T {
+        &self.backing
+    }
+}
+
+impl<T> Values<T> {
+    /// Attach backing data to a DebugFS directory.
+    ///
+    /// Attached data will be available inside [`Values::build`] to use when defining files in
+    /// the provided directory. When this object is dropped, it will remove the provided directory
+    /// before destroying the attached data.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::{c_str, new_mutex};
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let dir = Dir::new(c_str!("foo"));
+    /// let debugfs = KBox::pin_init(Values::attach(new_mutex!(0), dir), GFP_KERNEL)?;
+    /// // Files can be put inside `debugfs` which reference the constructed mutex.
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn attach(backing: impl PinInit<T>, dir: Dir<'static, false>) -> impl PinInit<Self> {
+        pin_init::pin_init! { Self {
+            dir: dir,
+            backing <- backing,
+            _pin: PhantomPinned,
+        }}
+    }
+
+    /// Runs a function which can create files from the backing data.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::{c_str, new_mutex};
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let dir = Dir::new(c_str!("foo"));
+    /// let debugfs = KBox::pin_init(Values::attach(new_mutex!(0), dir), GFP_KERNEL)?;
+    /// debugfs.as_ref().build(|mutex, dir| {
+    ///   // Can access `BoundDir` methods on `dir` here, with lifetime unified to the
+    ///   // lifetime of `mutex`
+    /// });
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn build<U, F: for<'b> FnOnce(&'b T, BoundDir<'b>) -> U>(self: Pin<&Self>, f: F) -> U {
+        // SAFETY: The `BoundDir` here is only being provided as a universally quantified argument
+        // to a function, so in the body, it will only be available in an existentially quantified
+        // context. This means that the function is legal to execute agains the true lifetime of
+        // the directory, even if we don't know exactly what that is.
+        f(&self.backing, unsafe { BoundDir::new(&self.dir) })
+    }
+}
+
+/// A `Dir`, bound to the lifetime for which it will exist. Unlike `&'a Dir`, this has an invariant
+/// lifetime - it cannot be shortened or lengthened.
+///
+/// # Invariants
+///
+/// * `BoundDir`'s lifetime must match the actual lifetime the directory lives for. In practice,
+///   this means that a `BoundDir` may only be used in an existentially quantified context.
+/// * `dir` will never be promoted to an owning reference (e.g. via calling `Dir::owning`)
+#[repr(transparent)]
+pub struct BoundDir<'a> {
+    dir: Dir<'a, true>,
+    _invariant: PhantomData<fn(&'a ()) -> &'a ()>,
+}
+
+impl<'a> BoundDir<'a> {
+    /// Create a new bound directory.
+    ///
+    /// # Safety
+    ///
+    /// `'a` is being used in a context where it is existentially quantified.
+    unsafe fn new(dir: &'a Dir<'_, false>) -> Self {
+        Self {
+            // We can create a non-owning `Dir` with our restricted lifetime because we do not
+            // allow this `dir` to be upgraded to an owning reference, and the owning reference
+            // provided necessarily outlives us.
+            dir: Dir {
+                dir: dir.dir,
+                _phantom: PhantomData,
+            },
+            _invariant: PhantomData,
+        }
+    }
+
+    /// Create a DebugFS subdirectory.
+    ///
+    /// This will produce another [`BoundDir`], scoped to the lifetime of the parent [`BoundDir`].
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let parent = Dir::new(c_str!("parent"));
+    /// let values = KBox::pin_init(Values::attach(0, parent), GFP_KERNEL)?;
+    /// values.as_ref().build(|value, builder| {
+    ///   builder.subdir(c_str!("child"));
+    /// });
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn subdir(&self, name: &CStr) -> Self {
+        Self {
+            dir: Dir::create(name, Some(&self.dir)),
+            _invariant: PhantomData,
+        }
+    }
+
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking `f`
+    /// on the provided reference.
+    ///
+    /// `f` must be a function item or a non-capturing closure, or this will fail to compile.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::{c_str, new_mutex};
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let parent = Dir::new(c_str!("parent"));
+    /// let values = KBox::pin_init(Values::attach(new_mutex!(0), parent), GFP_KERNEL)?;
+    /// values.as_ref().build(|value, builder| {
+    ///   builder.fmt_file(c_str!("child"), value, &|value, f| {
+    ///     writeln!(f, "Reading a mutex: {}", *value.lock())
+    ///   });
+    /// });
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn fmt_file<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
+        name: &CStr,
+        data: &'a T,
+        f: &'static F,
+    ) -> File<'a> {
+        // SAFETY: As `data` lives for the same lifetime as our `BoundDir` lifetime, which is
+        // instantiated as the actual lifetime of the directory, it lives long enough.
+        // We don't need to shorten the file handle because `BoundDir`'s lifetime parameter is both
+        // a lower *and* upper bound.
+        unsafe { self.dir.fmt_file_raw(name, data, f) }
+    }
+
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
+    /// [`Display::fmt`] on the provided reference with a trailing newline.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::{c_str, new_mutex};
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let parent = Dir::new(c_str!("parent"));
+    /// let values = KBox::pin_init(Values::attach((1, 2), parent), GFP_KERNEL)?;
+    /// values.as_ref().build(|value, builder| {
+    ///   builder.display_file(c_str!("child_0"), &value.0);
+    ///   builder.display_file(c_str!("child_1"), &value.1);
+    /// });
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn display_file<T: Display>(&self, name: &CStr, data: &'a T) -> File<'a> {
+        self.fmt_file(name, data, &|data, f| writeln!(f, "{}", data))
+    }
+}
+
 #[cfg(CONFIG_DEBUG_FS)]
 mod helpers {
     use crate::seq_file::SeqFile;

-- 
2.49.0.906.g1f30a19c02-goog

[PATCH WIP 2/2] rust: debugfs: Extend sample to use attached data

[Matthew Maurer]

Demonstrates how to attach data/handles needed for implementing DebugFS
file to a directory.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 samples/rust/rust_debugfs.rs | 110 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 4 deletions(-)

diff --git a/samples/rust/rust_debugfs.rs b/samples/rust/rust_debugfs.rs
index 2b1119b7281fd15109b542e6853d4206c2c80afc..6da8dd2c91e8c206b2314eabb97d6d31843efeb5 100644
--- a/samples/rust/rust_debugfs.rs
+++ b/samples/rust/rust_debugfs.rs
@@ -4,10 +4,14 @@
 
 //! Sample DebugFS exporting module
 
+use core::fmt;
+use core::fmt::{Display, Formatter};
 use core::sync::atomic::{AtomicU32, Ordering};
 use kernel::c_str;
-use kernel::debugfs::Dir;
+use kernel::debugfs::{BoundDir, Dir, Values};
+use kernel::new_mutex;
 use kernel::prelude::*;
+use kernel::sync::{Arc, Mutex};
 
 module! {
     type: RustDebugFs,
@@ -20,7 +24,89 @@
 struct RustDebugFs {
     // As we only hold this for drop effect (to remove the directory) we have a leading underscore
     // to indicate to the compiler that we don't expect to use this field directly.
-    _debugfs: Dir<'static>,
+    _debugfs: Pin<KBox<Values<Backing>>>,
+}
+
+struct Composite {
+    major: u32,
+    minor: u32,
+}
+
+struct Record {
+    name: &'static CStr,
+    size: usize,
+    stride: usize,
+}
+
+struct Backing {
+    simple: u32,
+    composite: Composite,
+    custom: u32,
+    many: KVec<Record>,
+    atomic: AtomicU32,
+    locked: Arc<Mutex<u32>>,
+}
+
+impl Display for Composite {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        write!(f, "{}.{}", self.major, self.minor)
+    }
+}
+
+impl Backing {
+    fn new() -> Result<Self> {
+        let mut many = KVec::new();
+        many.push(
+            Record {
+                name: c_str!("foo"),
+                size: 1,
+                stride: 2,
+            },
+            GFP_KERNEL,
+        )?;
+        many.push(
+            Record {
+                name: c_str!("bar"),
+                size: 3,
+                stride: 4,
+            },
+            GFP_KERNEL,
+        )?;
+        Ok(Self {
+            simple: 10,
+            composite: Composite { major: 1, minor: 2 },
+            custom: 37,
+            many,
+            atomic: AtomicU32::new(7),
+            locked: Arc::pin_init(new_mutex!(0), GFP_KERNEL)?,
+        })
+    }
+
+    fn build<'a>(&'a self, root: BoundDir<'a>) {
+        // Just prints out the number in the field
+        root.display_file(c_str!("simple"), &self.simple);
+        // Uses the custom display implementation to print major.minor
+        root.display_file(c_str!("composite"), &self.composite);
+        // Uses the custom hook print the number in 0-padded hex with some decorations.
+        root.fmt_file(c_str!("custom"), &self.custom, &|custom, f| {
+            writeln!(f, "Foo! {:#010x} Bar!", custom)
+        });
+        // Creates a directory for every record in the list, named the name of the item, with files
+        // for each attribute.
+        for record in self.many.iter() {
+            let dir = root.subdir(record.name);
+            dir.display_file(c_str!("size"), &record.size);
+            dir.display_file(c_str!("stride"), &record.stride);
+        }
+        // Access the attached atomic via `.load()`
+        root.fmt_file(c_str!("atomic"), &self.atomic, &|atomic, f| {
+            writeln!(f, "{}", atomic.load(Ordering::Relaxed))
+        });
+        // Access the attached mutex-guarded data via `.lock()`
+        root.fmt_file(c_str!("locked"), &self.locked, &|locked, f| {
+            writeln!(f, "{}", *locked.lock())
+        });
+    }
 }
 
 static EXAMPLE: AtomicU32 = AtomicU32::new(8);
@@ -28,11 +114,11 @@ struct RustDebugFs {
 impl kernel::Module for RustDebugFs {
     fn init(_this: &'static ThisModule) -> Result<Self> {
         // Create a debugfs directory in the root of the filesystem called "sample_debugfs".
-        let debugfs = Dir::new(c_str!("sample_debugfs"));
+        let root = Dir::new(c_str!("sample_debugfs"));
 
         {
             // Create a subdirectory, so "sample_debugfs/subdir" now exists.
-            let sub = debugfs.subdir(c_str!("subdir"));
+            let sub = root.subdir(c_str!("subdir"));
 
             // Create a single file in the subdirectory called "example" that will read from the
             // `EXAMPLE` atomic variable.
@@ -47,6 +133,22 @@ fn init(_this: &'static ThisModule) -> Result<Self> {
         EXAMPLE.store(10, Ordering::Relaxed);
         // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 10\n" when read.
 
+        // We can also attach data scoped to our root directory
+        let backing = Backing::new()?;
+        // Grab a refcount pointing to the locked data
+        let locked = backing.locked.clone();
+        let debugfs = KBox::pin_init(Values::attach(backing, root), GFP_KERNEL)?;
+
+        // Once it's attached, we can invoke `Backing::build` to create the relevant files:
+        debugfs.as_ref().build(Backing::build);
+
+        // We can still access read-only references the contents of the attached values. If the
+        // values allow interior mutability, like atomics, this lets us still change them:
+        debugfs.as_ref().atomic.store(8, Ordering::Relaxed);
+
+        // If we attached refcounted data, we can use an external handle to access it
+        *locked.lock() = 42;
+
         // Save the root debugfs directory we created to our module object. It will be
         // automatically cleaned up when our module is unloaded because dropping the module object
         // will drop the `Dir` handle. The base directory, the subdirectory, and the file will all

-- 
2.49.0.906.g1f30a19c02-goog

Re: [PATCH WIP 1/2] rust: debugfs: Add interface to build debugfs off pinned objects

[Danilo Krummrich]

On Sat, May 03, 2025 at 12:43:59AM +0000, Matthew Maurer wrote:
> +/// A DebugFS directory combined with a backing store for data to implement it.
> +#[pin_data]
> +pub struct Values<T> {
> +    dir: Dir<'static, false>,
> +    // The order here is load-bearing - `dir` must be dropped before `backing`, as files under
> +    // `dir` may point into `backing`.
> +    #[pin]
> +    backing: T,
> +    // Since the files present under our directory may point into `backing`, we are `!Unpin`.
> +    #[pin]
> +    _pin: PhantomPinned,
> +}

This only ever allows attaching data to the root directory, correct? What if I
want to remove (or replace) a file or a subdir? Then I'd be left with the data
for this specific file (or subdir) until the root is finally removed.

It would also require Option<V> (where V is the type of a field in T), if I
don't have an instance of V yet, when the root directory is created.

I think we should store the data per file, rather than per root directory.

Re: [PATCH WIP 1/2] rust: debugfs: Add interface to build debugfs off pinned objects

[Matthew Maurer]

On Sun, May 4, 2025 at 9:58 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sat, May 03, 2025 at 12:43:59AM +0000, Matthew Maurer wrote:
> > +/// A DebugFS directory combined with a backing store for data to implement it.
> > +#[pin_data]
> > +pub struct Values<T> {
> > +    dir: Dir<'static, false>,
> > +    // The order here is load-bearing - `dir` must be dropped before `backing`, as files under
> > +    // `dir` may point into `backing`.
> > +    #[pin]
> > +    backing: T,
> > +    // Since the files present under our directory may point into `backing`, we are `!Unpin`.
> > +    #[pin]
> > +    _pin: PhantomPinned,
> > +}
>
> This only ever allows attaching data to the root directory, correct? What if I
> want to remove (or replace) a file or a subdir? Then I'd be left with the data
> for this specific file (or subdir) until the root is finally removed.

The intended way to deal with this is that your debugfs root has
structures or handles that let you compute what all your debugfs files
should see, not necessarily fully populated if you want something
dynamic. For example, one of your entries could be
`Arc<Mutex<Vec<Record>>>`, and this could get updated elsewhere
without thinking about DebugFS - you just need to know the shape of
all the handles DebugFS will need.

It'd be pretty easy for me to relax this to allow attachments to
subtrees. I'd just need to do `Values<'a, T>` and `Dir<'a,  false>`.
It'd also make the argument for the safety of the builder interface
slightly more complicated (things that statically outlive the lifetime
of the dir would also be usable in the builder, which is safe, but
trickier to argue for).

You wouldn't be able to do nested attachments, but you could do
attachments starting with any subtree. So for example, you could do:
```
let root = Dir::new(c_str!("foo"));
let foo = root.subdir(c_str!("bar")).owning();
let bar = root.subdir(c_str!("baz")).owning();
let foo_values = Values::attach(foo_data, foo);
let bar_values = Values::attach(bar_data, foo);
```

The tricky business here (and why I didn't do it in my initial draft)
is that because `foo_values` and `bar_values` are restricted to live
no longer than `root` (since they will be invalidated if `root` is
dropped), it is hard to store these objects in a module context or
similar object, which would make it tricky to use. Attaching to a root
directory on the other hand is not tricky, because their lifecycle
isn't dependent on some other object.

A legal way of using this kind of scoped interface (but not
immediately obvious to me how to design the safe interface to let
users build it) might look like

```
struct MyDebugFsInfo<T> {
  // RawValues is a fictitious type that would be like `Values`, but
with the actual lifetime parameter erased.
  subdirs: Vec<RawValues<T>>,
  // Order matters, root must be released last because earlier values
are borrows
  root: Dir<'static, false>,
}

impl<T> MyDebugFsInfo<T> {
  fn new_subdir(&mut self, name: &CStr, value: T) {
    let subdir = self.root.subdir(name);
    // SAFETY: We don't allow the root to be destroyed before our structure, so
    let val = unsafe { RawValues::attach(value, self.root.subdir(name)) };
    self.subdirs.push(val)
  }
  fn get<'a>(&'a self, idx: usize) -> &Values<'a, T> {
     let x = self.subdirs[idx];
     // SAFETY: We know all our subdirs are children of the root we
own, so if we have a read-only reference to ourselves, that's an upper
bound on how long the value struct can live
     unsafe { RawValues::ascribe(x) }
  }
}
```

If I wanted to do better than this in terms of ergonomics, I think I'd
need some kind of refcounted wrapper where holding a subdir prevents
the parent dir from being cleaned up, which I could add, but would be
a Rust abstraction layer on top of the C one.


>
> It would also require Option<V> (where V is the type of a field in T), if I
> don't have an instance of V yet, when the root directory is created.

`Option<V>` won't actually save you - once you create a `Values<T>`,
you are intentionally unable to mutate `T` anymore, because it may be
read at any point in time after that by someone reading a debugfs
file, so you can't do a potentially racing mutation. If you wanted a
value to be attached to a debugfs, but didn't have one, you'd need to
do `Mutex<Option<V>>`.

>
> I think we should store the data per file, rather than per root directory.

This doesn't match what at least seems to be a common usage of
debugfs, where a debugfs directory is used to represent an object with
each file in the directory representing one of the properties on the
object.

This also means that we need to store N handles in a Rust driver (one
per live file) compared to 1 handle in a C driver.

===

I do think that the normal idiomatic way of doing this in Rust would
be to attach refcounted handles (e.g. similar to `ForeignOwnable`) to
inodes, and when the inode is destroyed, have that refcount be
deleted, but we don't currently have any `release` implementation for
a DebugFS inode afaict, so everything has to be externally scoped via
this attached pinned data setup I've been producing.

Re: [PATCH WIP 1/2] rust: debugfs: Add interface to build debugfs off pinned objects

[Danilo Krummrich]

On Mon, May 05, 2025 at 10:32:39AM -0700, Matthew Maurer wrote:
> On Sun, May 4, 2025 at 9:58 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sat, May 03, 2025 at 12:43:59AM +0000, Matthew Maurer wrote:
> > > +/// A DebugFS directory combined with a backing store for data to implement it.
> > > +#[pin_data]
> > > +pub struct Values<T> {
> > > +    dir: Dir<'static, false>,
> > > +    // The order here is load-bearing - `dir` must be dropped before `backing`, as files under
> > > +    // `dir` may point into `backing`.
> > > +    #[pin]
> > > +    backing: T,
> > > +    // Since the files present under our directory may point into `backing`, we are `!Unpin`.
> > > +    #[pin]
> > > +    _pin: PhantomPinned,
> > > +}
> >
> > This only ever allows attaching data to the root directory, correct? What if I
> > want to remove (or replace) a file or a subdir? Then I'd be left with the data
> > for this specific file (or subdir) until the root is finally removed.
> 
> The intended way to deal with this is that your debugfs root has
> structures or handles that let you compute what all your debugfs files
> should see, not necessarily fully populated if you want something
> dynamic. For example, one of your entries could be
> `Arc<Mutex<Vec<Record>>>`, and this could get updated elsewhere
> without thinking about DebugFS - you just need to know the shape of
> all the handles DebugFS will need.

Well sure, but what if we can't initialize them (yet) at the time we create the
debugfs root?

Also, what if the data that should be exposed through a file in debugfs is not
supposed to live for the full lifetime of the debugfs root (which might be
forever)?

> It'd be pretty easy for me to relax this to allow attachments to
> subtrees. I'd just need to do `Values<'a, T>` and `Dir<'a,  false>`.
> It'd also make the argument for the safety of the builder interface
> slightly more complicated (things that statically outlive the lifetime
> of the dir would also be usable in the builder, which is safe, but
> trickier to argue for).
> 
> You wouldn't be able to do nested attachments, but you could do
> attachments starting with any subtree. So for example, you could do:
> ```
> let root = Dir::new(c_str!("foo"));
> let foo = root.subdir(c_str!("bar")).owning();
> let bar = root.subdir(c_str!("baz")).owning();
> let foo_values = Values::attach(foo_data, foo);
> let bar_values = Values::attach(bar_data, foo);
> ```
> 
> The tricky business here (and why I didn't do it in my initial draft)
> is that because `foo_values` and `bar_values` are restricted to live
> no longer than `root` (since they will be invalidated if `root` is
> dropped), it is hard to store these objects in a module context or
> similar object, which would make it tricky to use. Attaching to a root
> directory on the other hand is not tricky, because their lifecycle
> isn't dependent on some other object.
> 
> A legal way of using this kind of scoped interface (but not
> immediately obvious to me how to design the safe interface to let
> users build it) might look like
> 
> ```
> struct MyDebugFsInfo<T> {
>   // RawValues is a fictitious type that would be like `Values`, but
> with the actual lifetime parameter erased.
>   subdirs: Vec<RawValues<T>>,
>   // Order matters, root must be released last because earlier values
> are borrows
>   root: Dir<'static, false>,
> }
> 
> impl<T> MyDebugFsInfo<T> {
>   fn new_subdir(&mut self, name: &CStr, value: T) {
>     let subdir = self.root.subdir(name);
>     // SAFETY: We don't allow the root to be destroyed before our structure, so
>     let val = unsafe { RawValues::attach(value, self.root.subdir(name)) };
>     self.subdirs.push(val)
>   }
>   fn get<'a>(&'a self, idx: usize) -> &Values<'a, T> {
>      let x = self.subdirs[idx];
>      // SAFETY: We know all our subdirs are children of the root we
> own, so if we have a read-only reference to ourselves, that's an upper
> bound on how long the value struct can live
>      unsafe { RawValues::ascribe(x) }
>   }
> }
> ```

Yes, I'm aware of this possibility and its complexity. That's why I think per
file would suit better.

> If I wanted to do better than this in terms of ergonomics, I think I'd
> need some kind of refcounted wrapper where holding a subdir prevents
> the parent dir from being cleaned up, which I could add, but would be
> a Rust abstraction layer on top of the C one.

I don't think something like that is desirable.

> >
> > It would also require Option<V> (where V is the type of a field in T), if I
> > don't have an instance of V yet, when the root directory is created.
> 
> `Option<V>` won't actually save you - once you create a `Values<T>`,
> you are intentionally unable to mutate `T` anymore, because it may be
> read at any point in time after that by someone reading a debugfs
> file, so you can't do a potentially racing mutation. If you wanted a
> value to be attached to a debugfs, but didn't have one, you'd need to
> do `Mutex<Option<V>>`.

Sure, or some other kind of synchronization. I left that aside, since it's not
relevant for the point I wanted to make. Which is that with the per root data
approach we'd need to work with Option types when the data isn't yet known when
the root directory is created and if we want to be able to free the data before
the root directory is removed (which might be never).

> >
> > I think we should store the data per file, rather than per root directory.
> 
> This doesn't match what at least seems to be a common usage of
> debugfs, where a debugfs directory is used to represent an object with
> each file in the directory representing one of the properties on the
> object.

Why not treat File as container, i.e. make the property of an object a File<T>
with T being the property?

> This also means that we need to store N handles in a Rust driver (one
> per live file) compared to 1 handle in a C driver.

I can't follow what you mean here. Files in a C driver do need a handle to the
data they expose too, no?

> 
> ===
> 
> I do think that the normal idiomatic way of doing this in Rust would
> be to attach refcounted handles (e.g. similar to `ForeignOwnable`) to
> inodes, and when the inode is destroyed, have that refcount be
> deleted, but we don't currently have any `release` implementation for
> a DebugFS inode afaict, so everything has to be externally scoped via
> this attached pinned data setup I've been producing.

Why do you think that this would be "the normal idimatic way" of doing this?

Does this imply that you think tying the lifetime of the object exposing a value
to the value itself wouldn't be idiomatic for Rust?