[PATCH v6 3/5] rust: debugfs: Support arbitrary owned backing for File

Matthew Maurer posted 5 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v6 3/5] rust: debugfs: Support arbitrary owned backing for File
Posted by Matthew Maurer 3 months, 3 weeks ago
This allows `File`s to be backed by `Deref<Target=T>` rather than just
`&'static T`. This means that dynamically allocated objects can be
attached to `File`s without needing to take extra steps to create a
pinned reference that's guaranteed to live long enough.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 6a89557d8cf49327d2984d15741ffb6640defd70..cd83f21cf2818f406575941ebbc6c426575643e4 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -5,12 +5,13 @@
 //!
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
-#[cfg(CONFIG_DEBUG_FS)]
+use crate::alloc::KBox;
 use crate::prelude::GFP_KERNEL;
 use crate::str::CStr;
 #[cfg(CONFIG_DEBUG_FS)]
 use crate::sync::Arc;
 use core::fmt::Display;
+use core::ops::Deref;
 
 #[cfg(CONFIG_DEBUG_FS)]
 mod display_file;
@@ -61,40 +62,59 @@ fn create(_name: &CStr, _parent: Option<&Dir>) -> Self {
     }
 
     #[cfg(CONFIG_DEBUG_FS)]
-    fn create_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
+    fn create_file<D: Deref<Target = T> + 'static + Send + Sync, T: Display>(
+        &self,
+        name: &CStr,
+        data: D,
+    ) -> File {
+        let mut file = File {
+            _entry: entry::Entry::empty(),
+            _data: None,
+        };
+        let Some(data) = KBox::new(data, GFP_KERNEL).ok() else {
+            return file;
+        };
+
         let Some(parent) = &self.0 else {
-            return File {
-                _entry: entry::Entry::empty(),
-            };
+            return file;
         };
+
         // 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.
+        //   we have an owning `D` in the `File`, and we tear down the file during `Drop`.
         let ptr = unsafe {
             bindings::debugfs_create_file_full(
                 name.as_char_ptr(),
                 0o444,
                 parent.as_ptr(),
-                data as *const _ as *mut _,
+                data.deref() as *const _ as *mut _,
                 core::ptr::null(),
                 &<T as display_file::DisplayFile>::VTABLE,
             )
         };
 
+        file._data = Some(data);
+
         // SAFETY: `debugfs_create_file_full` either returns an error code or a legal
         // dentry pointer, so `Entry::new` is safe to call here.
-        let entry = unsafe { entry::Entry::new(ptr, Some(parent.clone())) };
+        file._entry = unsafe { entry::Entry::new(ptr, Some(parent.clone())) };
 
-        File { _entry: entry }
+        file
     }
 
     #[cfg(not(CONFIG_DEBUG_FS))]
-    fn create_file<T: Display + Sized>(&self, _name: &CStr, _data: &'static T) -> File {
-        File {}
+    fn create_file<D: Deref<Target = T> + 'static + Send + Sync, T: Display>(
+        &self,
+        _name: &CStr,
+        data: D,
+    ) -> File {
+        File {
+            _data: KBox::new(data, GFP_KERNEL).ok().map(|x| x as _),
+        }
     }
 
     /// Create a DebugFS subdirectory.
@@ -125,7 +145,11 @@ pub fn subdir(&self, name: &CStr) -> Self {
     /// dir.display_file(c_str!("foo"), &200);
     /// // "my_debugfs_dir/foo" now contains the number 200.
     /// ```
-    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
+    pub fn display_file<D: Deref<Target = T> + 'static + Send + Sync, T: Display>(
+        &self,
+        name: &CStr,
+        data: D,
+    ) -> File {
         self.create_file(name, data)
     }
 
@@ -147,4 +171,7 @@ pub fn new(name: &CStr) -> Self {
 pub struct File {
     #[cfg(CONFIG_DEBUG_FS)]
     _entry: entry::Entry,
+    // The data needs to be kept in a `Box` to prevent it from moving when the file does, as
+    // this might invalidate the pointer that's been passed to debugfs.
+    _data: Option<KBox<dyn Send + Sync>>,
 }

-- 
2.50.0.rc2.696.g1fc2a0284f-goog
Re: [PATCH v6 3/5] rust: debugfs: Support arbitrary owned backing for File
Posted by Alice Ryhl 3 months, 3 weeks ago
On Wed, Jun 18, 2025 at 02:28:15AM +0000, Matthew Maurer wrote:
> This allows `File`s to be backed by `Deref<Target=T>` rather than just
> `&'static T`. This means that dynamically allocated objects can be
> attached to `File`s without needing to take extra steps to create a
> pinned reference that's guaranteed to live long enough.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  rust/kernel/debugfs.rs | 51 ++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index 6a89557d8cf49327d2984d15741ffb6640defd70..cd83f21cf2818f406575941ebbc6c426575643e4 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -5,12 +5,13 @@
>  //!
>  //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
>  
> -#[cfg(CONFIG_DEBUG_FS)]
> +use crate::alloc::KBox;
>  use crate::prelude::GFP_KERNEL;
>  use crate::str::CStr;
>  #[cfg(CONFIG_DEBUG_FS)]
>  use crate::sync::Arc;
>  use core::fmt::Display;
> +use core::ops::Deref;
>  
>  #[cfg(CONFIG_DEBUG_FS)]
>  mod display_file;
> @@ -61,40 +62,59 @@ fn create(_name: &CStr, _parent: Option<&Dir>) -> Self {
>      }
>  
>      #[cfg(CONFIG_DEBUG_FS)]
> -    fn create_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
> +    fn create_file<D: Deref<Target = T> + 'static + Send + Sync, T: Display>(
> +        &self,
> +        name: &CStr,
> +        data: D,
> +    ) -> File {
> +        let mut file = File {
> +            _entry: entry::Entry::empty(),
> +            _data: None,
> +        };
> +        let Some(data) = KBox::new(data, GFP_KERNEL).ok() else {
> +            return file;
> +        };

We may want to consider using the ForeignOwnable trait here instead. The
trait is implemented by anything that can be converted to/from a void
pointer, so you can:

* When creating the file, convert it to a void pointer that you store in
  File and pass to debugfs_create_file_full.
* When displaying the file, create a borrowed version of the void
  pointer and display that.
* When freeing the File, convert the void pointer back into an owned
  value and drop it.

For cases where a box really is necessary, the user can create a box and
pass it themselves. But if the user already has a pointer type (e.g. and
Arc<T> or &'static T) then they can pass that pointer directly and the
pointer is stored as a void pointer without the Box indirection.

Alice
Re: [PATCH v6 3/5] rust: debugfs: Support arbitrary owned backing for File
Posted by Matthew Maurer 3 months, 3 weeks ago
> We may want to consider using the ForeignOwnable trait here instead. The

I was considering trying to switch over to `StableDeref`-like trait
[1] in a follow-up patchset. The core property I need is that moving
the `D` cannot result in the pointer it would `deref` to changing.

The problem with `ForeignOwnable` is that it forbids the user from
passing in a `Box<dyn Foo>`, because that doesn't fit in a `void*` A
`StableDeref` version would not have this issue. I agree that
`ForeignOwnable` would be a strict upgrade to what I have now, since a
user can still pass in a `Box<Box<dyn Foo>>` and have it work with
`ForeignOwnable`, and if we ever added `StableDeref`, then
`ForeignOwnable` would have a blanket impl for it.

I'll send a new version using `ForeignOwnable`, and we can consider
the `StableDeref` version in the future.

[1]: https://docs.rs/gimli/latest/gimli/trait.StableDeref.html


> trait is implemented by anything that can be converted to/from a void
> pointer, so you can:
>
> * When creating the file, convert it to a void pointer that you store in
>   File and pass to debugfs_create_file_full.
> * When displaying the file, create a borrowed version of the void
>   pointer and display that.
> * When freeing the File, convert the void pointer back into an owned
>   value and drop it.
>
> For cases where a box really is necessary, the user can create a box and
> pass it themselves. But if the user already has a pointer type (e.g. and
> Arc<T> or &'static T) then they can pass that pointer directly and the
> pointer is stored as a void pointer without the Box indirection.
>
> Alice
Re: [PATCH v6 3/5] rust: debugfs: Support arbitrary owned backing for File
Posted by Danilo Krummrich 3 months, 3 weeks ago
On Wed, Jun 18, 2025 at 08:00:51AM -0700, Matthew Maurer wrote:
> > We may want to consider using the ForeignOwnable trait here instead. The
> 
> I was considering trying to switch over to `StableDeref`-like trait
> [1] in a follow-up patchset. The core property I need is that moving
> the `D` cannot result in the pointer it would `deref` to changing.
> 
> The problem with `ForeignOwnable` is that it forbids the user from
> passing in a `Box<dyn Foo>`, because that doesn't fit in a `void*` A
> `StableDeref` version would not have this issue. I agree that
> `ForeignOwnable` would be a strict upgrade to what I have now, since a
> user can still pass in a `Box<Box<dyn Foo>>` and have it work with
> `ForeignOwnable`, and if we ever added `StableDeref`, then
> `ForeignOwnable` would have a blanket impl for it.
> 
> I'll send a new version using `ForeignOwnable`, and we can consider
> the `StableDeref` version in the future.

Yes, please do that for now. It's rather common case that drivers want to expose
reference counted data, i.e. an Arc, through debugfs and having to go through
the indirection with an additional Box isn't quite nice.

> [1]: https://docs.rs/gimli/latest/gimli/trait.StableDeref.html
> 
> 
> > trait is implemented by anything that can be converted to/from a void
> > pointer, so you can:
> >
> > * When creating the file, convert it to a void pointer that you store in
> >   File and pass to debugfs_create_file_full.
> > * When displaying the file, create a borrowed version of the void
> >   pointer and display that.
> > * When freeing the File, convert the void pointer back into an owned
> >   value and drop it.
> >
> > For cases where a box really is necessary, the user can create a box and
> > pass it themselves. But if the user already has a pointer type (e.g. and
> > Arc<T> or &'static T) then they can pass that pointer directly and the
> > pointer is stored as a void pointer without the Box indirection.
> >
> > Alice