[PATCH v3 0/4] rust: DebugFS Bindings

[PATCH v3 0/4] rust: DebugFS Bindings

[Matthew Maurer]

This series provides safe DebugFS bindings for Rust, with a sample
driver using them.

This series currently only supports referencing `'static` lifetime
objects, because we need to know the objects outlive the DebugFS files.
A subsequent series may be able to relax this.

* Directories created in the root of debugfs are assumed to be owning,
  and will clean up after themselves when dropped.
* Subdirectories are assumed non-owning, remaining alive until their
  containing directory is cleaned up. They can be promoted to owning
  variants if needed.
* Files are non-owning, but can be manually removed by handle if needed.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
Changes in v3:
- Split `Dir` into `Dir`/`SubDir`/`File` to improve API.
- Add "." to end of all comments.
- Convert INVARIANT to # Invariants on types.
- Add backticks everywhere I found variables/types in my comments.
- Promoted invariant comment to doc comment.
- Extended sample commenting to make it clearer what is happening.
- Link to v2: https://lore.kernel.org/r/20250430-debugfs-rust-v2-0-2e8d3985812b@google.com

Changes in v2:
- Drop support for builder / pinned bindings in initial series
- Remove `ARef` usage to abstract the dentry nature of handles
- Remove error handling to discourage users from caring whether DebugFS
  is enabled.
- Support CONFIG_DEBUG_FS=n while leaving the API available
- Fixed mistaken decimal/octal mixup
- Doc/comment cleanup
- Link to v1: https://lore.kernel.org/r/20250429-debugfs-rust-v1-0-6b6e7cb7929f@google.com

---
Matthew Maurer (4):
      rust: debugfs: Bind DebugFS directory creation
      rust: debugfs: Bind file creation for long-lived Display
      rust: debugfs: Support format hooks
      rust: samples: Add debugfs sample

 MAINTAINERS                     |   2 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/debugfs.rs          | 405 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 samples/rust/Kconfig            |  11 ++
 samples/rust/Makefile           |   1 +
 samples/rust/rust_debugfs.rs    |  54 ++++++
 7 files changed, 475 insertions(+)
---
base-commit: b6a7783d306baf3150ac54cd5124f6e85dd375b0
change-id: 20250428-debugfs-rust-3cd5c97eb7d1

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

[PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Matthew Maurer]

Support creating DebugFS directories and subdirectories. Similar to the
original DebugFS API, errors are hidden.

By default, when a root directory handle leaves scope, it will be cleaned
up.

Subdirectories will by default persist until their root directory is
cleaned up, but can be converted into a root directory if a scoped
lifecycle is desired.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/debugfs.rs          | 155 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 4 files changed, 158 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 906881b6c5cb6ff743e13b251873b89138c69a1c..a3b835e427b083a4ddd690d9e7739851f0af47ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7271,6 +7271,7 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	include/linux/sysfs.h
 F:	lib/kobj*
+F:	rust/kernel/debugfs.rs
 F:	rust/kernel/device.rs
 F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8a2add69e5d66d1c2ebed9d2c950380e61c48842..787f928467faabd02a7f3cf041378fac856c4f89 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -13,6 +13,7 @@
 #include <linux/blkdev.h>
 #include <linux/cpumask.h>
 #include <linux/cred.h>
+#include <linux/debugfs.h>
 #include <linux/device/faux.h>
 #include <linux/dma-mapping.h>
 #include <linux/errname.h>
diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..b589c2d9a8d169bd66e98d2894261784e427230e
--- /dev/null
+++ b/rust/kernel/debugfs.rs
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! DebugFS Abstraction
+//!
+//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
+
+use crate::str::CStr;
+use core::mem::ManuallyDrop;
+use core::ops::Deref;
+
+/// Owning handle to a DebugFS directory.
+///
+/// This directory will be cleaned up when it goes out of scope.
+///
+/// # Invariants
+///
+/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
+#[repr(transparent)]
+pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
+
+// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
+// between threads.
+unsafe impl Send for Dir {}
+
+// SAFETY: All the native functions we re-export use interior locking, and the contents of the
+// struct are opaque to Rust.
+unsafe impl Sync for Dir {}
+
+impl Dir {
+    /// Create a new directory in DebugFS at the root.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// {
+    ///    let parent = Dir::new(c_str!("parent"));
+    ///    // The path "parent" exists in DebugFS here.
+    /// }
+    /// // It does not exist here.
+    /// ```
+    pub fn new(name: &CStr) -> Self {
+        Self::create(name, None)
+    }
+
+    /// Create a DebugFS subdirectory.
+    ///
+    /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
+    /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// {
+    ///    let parent = Dir::new(c_str!("parent"));
+    ///    // The path "parent" exists in DebugFS here.
+    ///    {
+    ///        let child = parent.subdir(c_str!("child"));
+    ///        // The path "parent/child" exists in DebugFS here.
+    ///    }
+    ///    // The path "parent/child" still exists.
+    ///    {
+    ///        let child2 = Dir::from(parent.subdir(c_str!("child2")));
+    ///        // The path "parent/child2" exists in DebugFS here.
+    ///    }
+    ///    // The path "parent/child2" is gone.
+    /// }
+    /// // None of the paths exist here.
+    /// ```
+    pub fn subdir(&self, name: &CStr) -> SubDir {
+        SubDir::new(Self::create(name, Some(self)))
+    }
+
+    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
+    #[cfg(CONFIG_DEBUG_FS)]
+    fn create(name: &CStr, parent: Option<&Self>) -> Self {
+        let parent_ptr = match parent {
+            Some(parent) => parent.as_ptr(),
+            None => core::ptr::null_mut(),
+        };
+        // SAFETY:
+        // * `name` argument points to a NUL-terminated string that lives across the call, by
+        //   invariants of `&CStr`.
+        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
+        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
+        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
+        //   so we can call `Self::from_ptr`.
+        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
+    }
+
+    #[cfg(not(CONFIG_DEBUG_FS))]
+    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
+        Self()
+    }
+
+    /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
+    ///
+    /// # Safety
+    ///
+    /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
+    /// live DebugFS directory.
+    #[cfg(CONFIG_DEBUG_FS)]
+    unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
+        Self(ptr)
+    }
+
+    /// Returns the pointer representation of the DebugFS directory.
+    ///
+    /// Due to the type invariant, the value returned from this function will always be an error
+    /// code, NUL, or a live DebugFS directory.
+    // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
+    #[cfg(CONFIG_DEBUG_FS)]
+    fn as_ptr(&self) -> *mut bindings::dentry {
+        self.0
+    }
+}
+
+impl Drop for Dir {
+    fn drop(&mut self) {
+        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
+        // `as_ptr` guarantees that the pointer is of this form.
+        #[cfg(CONFIG_DEBUG_FS)]
+        unsafe {
+            bindings::debugfs_remove(self.as_ptr())
+        }
+    }
+}
+
+/// Handle to a DebugFS directory that will stay alive after leaving scope.
+#[repr(transparent)]
+pub struct SubDir(ManuallyDrop<Dir>);
+
+impl Deref for SubDir {
+    type Target = Dir;
+    fn deref(&self) -> &Dir {
+        &self.0
+    }
+}
+
+impl From<SubDir> for Dir {
+    fn from(subdir: SubDir) -> Self {
+        ManuallyDrop::into_inner(subdir.0)
+    }
+}
+
+impl SubDir {
+    fn new(dir: Dir) -> Self {
+        Self(ManuallyDrop::new(dir))
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c3762e80b314316b4b0cee3bfd9442f8f0510b91..86f6055b828d5f711578293d8916a517f2436977 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -45,6 +45,7 @@
 #[doc(hidden)]
 pub mod build_assert;
 pub mod cred;
+pub mod debugfs;
 pub mod device;
 pub mod device_id;
 pub mod devres;

-- 
2.49.0.906.g1f30a19c02-goog

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Danilo Krummrich]

On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> +/// Owning handle to a DebugFS directory.
> +///
> +/// This directory will be cleaned up when it goes out of scope.
> +///
> +/// # Invariants
> +///
> +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> +#[repr(transparent)]
> +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);

Should probably use Opaque instead of a raw pointer.

> +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred

[`Dir`]

> +// between threads.
> +unsafe impl Send for Dir {}
> +
> +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> +// struct are opaque to Rust.
> +unsafe impl Sync for Dir {}
> +
> +impl Dir {
> +    /// Create a new directory in DebugFS at the root.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// {
> +    ///    let parent = Dir::new(c_str!("parent"));
> +    ///    // The path "parent" exists in DebugFS here.
> +    /// }
> +    /// // It does not exist here.

This ready like an explanation for scopes; I think we should drop those comments
and the scope.

> +    /// ```
> +    pub fn new(name: &CStr) -> Self {
> +        Self::create(name, None)
> +    }
> +
> +    /// Create a DebugFS subdirectory.
> +    ///
> +    /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> +    /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// {
> +    ///    let parent = Dir::new(c_str!("parent"));
> +    ///    // The path "parent" exists in DebugFS here.
> +    ///    {
> +    ///        let child = parent.subdir(c_str!("child"));
> +    ///        // The path "parent/child" exists in DebugFS here.
> +    ///    }
> +    ///    // The path "parent/child" still exists.
> +    ///    {
> +    ///        let child2 = Dir::from(parent.subdir(c_str!("child2")));
> +    ///        // The path "parent/child2" exists in DebugFS here.
> +    ///    }
> +    ///    // The path "parent/child2" is gone.
> +    /// }
> +    /// // None of the paths exist here.

I think the fact that you need all those comments here proves that it's not
really intuitive. Please see me comment on SubDir below.

> +    /// ```
> +    pub fn subdir(&self, name: &CStr) -> SubDir {
> +        SubDir::new(Self::create(name, Some(self)))
> +    }
> +
> +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    fn create(name: &CStr, parent: Option<&Self>) -> Self {
> +        let parent_ptr = match parent {
> +            Some(parent) => parent.as_ptr(),
> +            None => core::ptr::null_mut(),
> +        };
> +        // SAFETY:
> +        // * `name` argument points to a NUL-terminated string that lives across the call, by
> +        //   invariants of `&CStr`.
> +        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> +        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> +        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> +        //   so we can call `Self::from_ptr`.
> +        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }

Please split up in two calls, such that we don't have two unsafe function calls
in a single unsafe block.

> +    }
> +
> +    #[cfg(not(CONFIG_DEBUG_FS))]
> +    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> +        Self()
> +    }
> +
> +    /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
> +    /// live DebugFS directory.
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
> +        Self(ptr)
> +    }
> +
> +    /// Returns the pointer representation of the DebugFS directory.
> +    ///
> +    /// Due to the type invariant, the value returned from this function will always be an error
> +    /// code, NUL, or a live DebugFS directory.

Maybe put this in a '# Guarantees' section.

> +    // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.

I think you mean ERR_PTR(ENODEV).

> +    #[cfg(CONFIG_DEBUG_FS)]
> +    fn as_ptr(&self) -> *mut bindings::dentry {
> +        self.0
> +    }
> +}
> +
> +impl Drop for Dir {
> +    fn drop(&mut self) {
> +        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> +        // `as_ptr` guarantees that the pointer is of this form.
> +        #[cfg(CONFIG_DEBUG_FS)]
> +        unsafe {
> +            bindings::debugfs_remove(self.as_ptr())
> +        }
> +    }
> +}
> +
> +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> +#[repr(transparent)]
> +pub struct SubDir(ManuallyDrop<Dir>);

I think it's not very intuitive if the default is that a SubDir still exists
after it has been dropped. I think your first approach being explicit about this
with keep() consuming the SubDir was much better; please keep this approach.

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman]

On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > +/// Owning handle to a DebugFS directory.
> > +///
> > +/// This directory will be cleaned up when it goes out of scope.
> > +///
> > +/// # Invariants
> > +///
> > +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> > +#[repr(transparent)]
> > +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
> 
> Should probably use Opaque instead of a raw pointer.
> 
> > +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
> 
> [`Dir`]
> 
> > +// between threads.
> > +unsafe impl Send for Dir {}
> > +
> > +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> > +// struct are opaque to Rust.
> > +unsafe impl Sync for Dir {}
> > +
> > +impl Dir {
> > +    /// Create a new directory in DebugFS at the root.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// {
> > +    ///    let parent = Dir::new(c_str!("parent"));
> > +    ///    // The path "parent" exists in DebugFS here.
> > +    /// }
> > +    /// // It does not exist here.
> 
> This ready like an explanation for scopes; I think we should drop those comments
> and the scope.
> 
> > +    /// ```
> > +    pub fn new(name: &CStr) -> Self {
> > +        Self::create(name, None)
> > +    }
> > +
> > +    /// Create a DebugFS subdirectory.
> > +    ///
> > +    /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> > +    /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// {
> > +    ///    let parent = Dir::new(c_str!("parent"));
> > +    ///    // The path "parent" exists in DebugFS here.
> > +    ///    {
> > +    ///        let child = parent.subdir(c_str!("child"));
> > +    ///        // The path "parent/child" exists in DebugFS here.
> > +    ///    }
> > +    ///    // The path "parent/child" still exists.
> > +    ///    {
> > +    ///        let child2 = Dir::from(parent.subdir(c_str!("child2")));
> > +    ///        // The path "parent/child2" exists in DebugFS here.
> > +    ///    }
> > +    ///    // The path "parent/child2" is gone.
> > +    /// }
> > +    /// // None of the paths exist here.
> 
> I think the fact that you need all those comments here proves that it's not
> really intuitive. Please see me comment on SubDir below.
> 
> > +    /// ```
> > +    pub fn subdir(&self, name: &CStr) -> SubDir {
> > +        SubDir::new(Self::create(name, Some(self)))
> > +    }
> > +
> > +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    fn create(name: &CStr, parent: Option<&Self>) -> Self {
> > +        let parent_ptr = match parent {
> > +            Some(parent) => parent.as_ptr(),
> > +            None => core::ptr::null_mut(),
> > +        };
> > +        // SAFETY:
> > +        // * `name` argument points to a NUL-terminated string that lives across the call, by
> > +        //   invariants of `&CStr`.
> > +        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> > +        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> > +        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > +        //   so we can call `Self::from_ptr`.
> > +        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
> 
> Please split up in two calls, such that we don't have two unsafe function calls
> in a single unsafe block.
> 
> > +    }
> > +
> > +    #[cfg(not(CONFIG_DEBUG_FS))]
> > +    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> > +        Self()
> > +    }
> > +
> > +    /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
> > +    /// live DebugFS directory.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
> > +        Self(ptr)
> > +    }
> > +
> > +    /// Returns the pointer representation of the DebugFS directory.
> > +    ///
> > +    /// Due to the type invariant, the value returned from this function will always be an error
> > +    /// code, NUL, or a live DebugFS directory.
> 
> Maybe put this in a '# Guarantees' section.
> 
> > +    // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
> 
> I think you mean ERR_PTR(ENODEV).
> 
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    fn as_ptr(&self) -> *mut bindings::dentry {
> > +        self.0
> > +    }
> > +}
> > +
> > +impl Drop for Dir {
> > +    fn drop(&mut self) {
> > +        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> > +        // `as_ptr` guarantees that the pointer is of this form.
> > +        #[cfg(CONFIG_DEBUG_FS)]
> > +        unsafe {
> > +            bindings::debugfs_remove(self.as_ptr())
> > +        }
> > +    }
> > +}
> > +
> > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > +#[repr(transparent)]
> > +pub struct SubDir(ManuallyDrop<Dir>);
> 
> I think it's not very intuitive if the default is that a SubDir still exists
> after it has been dropped. I think your first approach being explicit about this
> with keep() consuming the SubDir was much better; please keep this approach.

Wait, let's step back.  Why do we care about the difference between a
"subdir" and a "dir"?  They both are the same thing, and how do you
describe a subdir of a subdir?  :)

Why the "split" here, that just adds additional mental energy to both
someone using the api, as well as someone having to review someone using
it.  A directory is a directory, no matter where in debugfs it lives, so
let's just keep it as simple as possible please.

thanks,

greg k-h

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Danilo Krummrich]

On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote:
> On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > > +#[repr(transparent)]
> > > +pub struct SubDir(ManuallyDrop<Dir>);
> > 
> > I think it's not very intuitive if the default is that a SubDir still exists
> > after it has been dropped. I think your first approach being explicit about this
> > with keep() consuming the SubDir was much better; please keep this approach.
> 
> Wait, let's step back.  Why do we care about the difference between a
> "subdir" and a "dir"?  They both are the same thing, and how do you
> describe a subdir of a subdir?  :)

We care about the difference, because Dir originally had keep() which drops the
Dir instance without actually removing it. For subdirs this is fine, since
they'll be cleaned up when the parent is removed.

However, we don't want users to be able to call keep() on the directory that has
been created first, since if that's done we loose our root anchor to ever free
the tree, which almost always would be a bug.

Please also see [1] and subsequent replies.

A subdir of a subdir would still be a SubDir.

[1] https://lore.kernel.org/lkml/aBNKEewhCP8jRIZL@pollux/

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman]

On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote:
> On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > > > +#[repr(transparent)]
> > > > +pub struct SubDir(ManuallyDrop<Dir>);
> > > 
> > > I think it's not very intuitive if the default is that a SubDir still exists
> > > after it has been dropped. I think your first approach being explicit about this
> > > with keep() consuming the SubDir was much better; please keep this approach.
> > 
> > Wait, let's step back.  Why do we care about the difference between a
> > "subdir" and a "dir"?  They both are the same thing, and how do you
> > describe a subdir of a subdir?  :)
> 
> We care about the difference, because Dir originally had keep() which drops the
> Dir instance without actually removing it. For subdirs this is fine, since
> they'll be cleaned up when the parent is removed.

But does that mean a subdir can not be cleaned up without dropping the
parent first?  For many subsystems, they make a "root" debugfs
directory, and then add/remove subdirs all the time within that.

> However, we don't want users to be able to call keep() on the directory that has
> been created first, since if that's done we loose our root anchor to ever free
> the tree, which almost always would be a bug.

Then do a call to debugfs_lookup_and_remove() which is what I really
recommend doing for any C user anyway.  That way no dentry is ever
"stored" anywhere.

Anyway, if Dir always has an implicit keep() call in it, then I guess
this is ok.  Let's see how this shakes out with some real-world users.
We can always change it over time if it gets unwieldy.

thanks,

greg k-h

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Danilo Krummrich]

On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote:
> On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote:
> > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > > > > +#[repr(transparent)]
> > > > > +pub struct SubDir(ManuallyDrop<Dir>);
> > > > 
> > > > I think it's not very intuitive if the default is that a SubDir still exists
> > > > after it has been dropped. I think your first approach being explicit about this
> > > > with keep() consuming the SubDir was much better; please keep this approach.
> > > 
> > > Wait, let's step back.  Why do we care about the difference between a
> > > "subdir" and a "dir"?  They both are the same thing, and how do you
> > > describe a subdir of a subdir?  :)
> > 
> > We care about the difference, because Dir originally had keep() which drops the
> > Dir instance without actually removing it. For subdirs this is fine, since
> > they'll be cleaned up when the parent is removed.
> 
> But does that mean a subdir can not be cleaned up without dropping the
> parent first?  For many subsystems, they make a "root" debugfs
> directory, and then add/remove subdirs all the time within that.

In the following I will call the first top level directory created by a module /
driver "root".

The logic I propose is that "root" is of type Dir, which means there is no
keep() and if dropped the whole tree under root is removed.

A subdir created under a Dir is of type SubDir and has the keep() method and if
called consumes the type instance and subsequently can only ever be removed by
dropping root.

Alternatively a SubDir can be converted into a Dir, and hence don't has keep()
anymore and if dropped will be removed.

So, the result is that we still can add / remove subdirs as we want.

The advantage is that we don't have keep() for root, which would be a dedicated
API for driver / modules to create bugs. If a driver / module would call keep()
on the root, it would not only mean that we leak the root directory, but also
all subdirs and files that we called keep() on.

This becomes even more problematic if we start attaching data to files. Think of
an Arc that is attached to a file, which keeps driver data alive just because we
leaked the root.

> > However, we don't want users to be able to call keep() on the directory that has
> > been created first, since if that's done we loose our root anchor to ever free
> > the tree, which almost always would be a bug.
> 
> Then do a call to debugfs_lookup_and_remove() which is what I really
> recommend doing for any C user anyway.  That way no dentry is ever
> "stored" anywhere.
> 
> Anyway, if Dir always has an implicit keep() call in it, then I guess
> this is ok.  Let's see how this shakes out with some real-world users.
> We can always change it over time if it gets unwieldy.

I really advise against it, Rust allows us to model such subtile differences
properly (and easily) with the type system to avoid bugs. Let's take advantage
of that.

Using debugfs_lookup_and_remove() wouldn't change anything, since we want to
attach the lifetime of a directory to a corresponding object.

(Otherwise we're back to where we are with C, i.e. the user has to remember to
call the correct thing at the correct time, rather than letting the type system
take care instead.)

So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from
Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer
vs. CString.

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Danilo Krummrich]

On Fri, May 02, 2025 at 09:33:21AM +0200, Danilo Krummrich wrote:
> On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote:
> > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > > > > > +#[repr(transparent)]
> > > > > > +pub struct SubDir(ManuallyDrop<Dir>);
> > > > > 
> > > > > I think it's not very intuitive if the default is that a SubDir still exists
> > > > > after it has been dropped. I think your first approach being explicit about this
> > > > > with keep() consuming the SubDir was much better; please keep this approach.
> > > > 
> > > > Wait, let's step back.  Why do we care about the difference between a
> > > > "subdir" and a "dir"?  They both are the same thing, and how do you
> > > > describe a subdir of a subdir?  :)
> > > 
> > > We care about the difference, because Dir originally had keep() which drops the
> > > Dir instance without actually removing it. For subdirs this is fine, since
> > > they'll be cleaned up when the parent is removed.
> > 
> > But does that mean a subdir can not be cleaned up without dropping the
> > parent first?  For many subsystems, they make a "root" debugfs
> > directory, and then add/remove subdirs all the time within that.
> 
> In the following I will call the first top level directory created by a module /
> driver "root".
> 
> The logic I propose is that "root" is of type Dir, which means there is no
> keep() and if dropped the whole tree under root is removed.
> 
> A subdir created under a Dir is of type SubDir and has the keep() method and if
> called consumes the type instance and subsequently can only ever be removed by
> dropping root.
> 
> Alternatively a SubDir can be converted into a Dir, and hence don't has keep()
> anymore and if dropped will be removed.
> 
> So, the result is that we still can add / remove subdirs as we want.
> 
> The advantage is that we don't have keep() for root, which would be a dedicated
> API for driver / modules to create bugs. If a driver / module would call keep()
> on the root, it would not only mean that we leak the root directory, but also
> all subdirs and files that we called keep() on.
> 
> This becomes even more problematic if we start attaching data to files. Think of
> an Arc that is attached to a file, which keeps driver data alive just because we
> leaked the root.

Forgot to mention, this Arc could contain vtables into the (driver) module after
the module has been removed already, which could be called into if reading
from / writing to a corresponding (leaked) debugfs file.

I really think Dir::keep() is an invitation for potentially horrible bugs.

If we really don't want SubDir, then let's not have keep() at all.

> > > However, we don't want users to be able to call keep() on the directory that has
> > > been created first, since if that's done we loose our root anchor to ever free
> > > the tree, which almost always would be a bug.
> > 
> > Then do a call to debugfs_lookup_and_remove() which is what I really
> > recommend doing for any C user anyway.  That way no dentry is ever
> > "stored" anywhere.
> > 
> > Anyway, if Dir always has an implicit keep() call in it, then I guess
> > this is ok.  Let's see how this shakes out with some real-world users.
> > We can always change it over time if it gets unwieldy.
> 
> I really advise against it, Rust allows us to model such subtile differences
> properly (and easily) with the type system to avoid bugs. Let's take advantage
> of that.
> 
> Using debugfs_lookup_and_remove() wouldn't change anything, since we want to
> attach the lifetime of a directory to a corresponding object.
> 
> (Otherwise we're back to where we are with C, i.e. the user has to remember to
> call the correct thing at the correct time, rather than letting the type system
> take care instead.)
> 
> So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from
> Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer
> vs. CString.

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman]

On Fri, May 02, 2025 at 09:33:15AM +0200, Danilo Krummrich wrote:
> On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote:
> > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > > > > > +#[repr(transparent)]
> > > > > > +pub struct SubDir(ManuallyDrop<Dir>);
> > > > > 
> > > > > I think it's not very intuitive if the default is that a SubDir still exists
> > > > > after it has been dropped. I think your first approach being explicit about this
> > > > > with keep() consuming the SubDir was much better; please keep this approach.
> > > > 
> > > > Wait, let's step back.  Why do we care about the difference between a
> > > > "subdir" and a "dir"?  They both are the same thing, and how do you
> > > > describe a subdir of a subdir?  :)
> > > 
> > > We care about the difference, because Dir originally had keep() which drops the
> > > Dir instance without actually removing it. For subdirs this is fine, since
> > > they'll be cleaned up when the parent is removed.
> > 
> > But does that mean a subdir can not be cleaned up without dropping the
> > parent first?  For many subsystems, they make a "root" debugfs
> > directory, and then add/remove subdirs all the time within that.
> 
> In the following I will call the first top level directory created by a module /
> driver "root".
> 
> The logic I propose is that "root" is of type Dir, which means there is no
> keep() and if dropped the whole tree under root is removed.
> 
> A subdir created under a Dir is of type SubDir and has the keep() method and if
> called consumes the type instance and subsequently can only ever be removed by
> dropping root.
> 
> Alternatively a SubDir can be converted into a Dir, and hence don't has keep()
> anymore and if dropped will be removed.
> 
> So, the result is that we still can add / remove subdirs as we want.
> 
> The advantage is that we don't have keep() for root, which would be a dedicated
> API for driver / modules to create bugs. If a driver / module would call keep()
> on the root, it would not only mean that we leak the root directory, but also
> all subdirs and files that we called keep() on.
> 
> This becomes even more problematic if we start attaching data to files. Think of
> an Arc that is attached to a file, which keeps driver data alive just because we
> leaked the root.

Ok, fair enough, let's try it this way, without keep()

> > > However, we don't want users to be able to call keep() on the directory that has
> > > been created first, since if that's done we loose our root anchor to ever free
> > > the tree, which almost always would be a bug.
> > 
> > Then do a call to debugfs_lookup_and_remove() which is what I really
> > recommend doing for any C user anyway.  That way no dentry is ever
> > "stored" anywhere.
> > 
> > Anyway, if Dir always has an implicit keep() call in it, then I guess
> > this is ok.  Let's see how this shakes out with some real-world users.
> > We can always change it over time if it gets unwieldy.
> 
> I really advise against it, Rust allows us to model such subtile differences
> properly (and easily) with the type system to avoid bugs. Let's take advantage
> of that.
> 
> Using debugfs_lookup_and_remove() wouldn't change anything, since we want to
> attach the lifetime of a directory to a corresponding object.
> 
> (Otherwise we're back to where we are with C, i.e. the user has to remember to
> call the correct thing at the correct time, rather than letting the type system
> take care instead.)
> 
> So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from
> Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer
> vs. CString.

Ok, that's fine, and it gives me an idea of how I can fix up the C api
over time to get rid of exporting the dentry entirely :)

thanks,

greg k-h

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Matthew Maurer]

On Fri, May 2, 2025 at 4:55 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 02, 2025 at 09:33:15AM +0200, Danilo Krummrich wrote:
> > On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote:
> > > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> > > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > > > > > > +#[repr(transparent)]
> > > > > > > +pub struct SubDir(ManuallyDrop<Dir>);
> > > > > >
> > > > > > I think it's not very intuitive if the default is that a SubDir still exists
> > > > > > after it has been dropped. I think your first approach being explicit about this
> > > > > > with keep() consuming the SubDir was much better; please keep this approach.
> > > > >
> > > > > Wait, let's step back.  Why do we care about the difference between a
> > > > > "subdir" and a "dir"?  They both are the same thing, and how do you
> > > > > describe a subdir of a subdir?  :)
> > > >
> > > > We care about the difference, because Dir originally had keep() which drops the
> > > > Dir instance without actually removing it. For subdirs this is fine, since
> > > > they'll be cleaned up when the parent is removed.
> > >
> > > But does that mean a subdir can not be cleaned up without dropping the
> > > parent first?  For many subsystems, they make a "root" debugfs
> > > directory, and then add/remove subdirs all the time within that.
> >
> > In the following I will call the first top level directory created by a module /
> > driver "root".
> >
> > The logic I propose is that "root" is of type Dir, which means there is no
> > keep() and if dropped the whole tree under root is removed.
> >
> > A subdir created under a Dir is of type SubDir and has the keep() method and if
> > called consumes the type instance and subsequently can only ever be removed by
> > dropping root.
> >
> > Alternatively a SubDir can be converted into a Dir, and hence don't has keep()
> > anymore and if dropped will be removed.
> >
> > So, the result is that we still can add / remove subdirs as we want.
> >
> > The advantage is that we don't have keep() for root, which would be a dedicated
> > API for driver / modules to create bugs. If a driver / module would call keep()
> > on the root, it would not only mean that we leak the root directory, but also
> > all subdirs and files that we called keep() on.
> >
> > This becomes even more problematic if we start attaching data to files. Think of
> > an Arc that is attached to a file, which keeps driver data alive just because we
> > leaked the root.
>
> Ok, fair enough, let's try it this way, without keep()
>
> > > > However, we don't want users to be able to call keep() on the directory that has
> > > > been created first, since if that's done we loose our root anchor to ever free
> > > > the tree, which almost always would be a bug.
> > >
> > > Then do a call to debugfs_lookup_and_remove() which is what I really
> > > recommend doing for any C user anyway.  That way no dentry is ever
> > > "stored" anywhere.
> > >
> > > Anyway, if Dir always has an implicit keep() call in it, then I guess
> > > this is ok.  Let's see how this shakes out with some real-world users.
> > > We can always change it over time if it gets unwieldy.
> >
> > I really advise against it, Rust allows us to model such subtile differences
> > properly (and easily) with the type system to avoid bugs. Let's take advantage
> > of that.
> >
> > Using debugfs_lookup_and_remove() wouldn't change anything, since we want to
> > attach the lifetime of a directory to a corresponding object.
> >
> > (Otherwise we're back to where we are with C, i.e. the user has to remember to
> > call the correct thing at the correct time, rather than letting the type system
> > take care instead.)
> >
> > So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from
> > Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer
> > vs. CString.
>
> Ok, that's fine, and it gives me an idea of how I can fix up the C api
> over time to get rid of exporting the dentry entirely :)

I've figured out that the SubDir API (and indeed, all patchsets after
v1) have a flaw in them, so I'm wondering if your rework will resolve
this.

If I do:
```
let dir = Dir::new(c_str!("foo"));
let subdir = dir.subdir(c_str!("bar"));
drop(dir);
// This next line will Oops because subdir is already removed
drop(Dir::from(subdir));
```

Simply removing `Dir::from` from the API won't resolve this - as long
as `SubDir` has any method, accessing it after its parent has been
cleaned up with `remove` will result in an oops.

The options I can see given the existing API are:
A. SubDir needs to be inherently limited to a borrow of another Dir or
SubDir. This will still break if someone uses
`debugfs_lookup_and_remove()` in an antisocial fashion, but we haven't
bound this in Rust, and we don't need to be robust against bad
behavior from C. If we do this, the promotability of subdirectories
back to directories would go away entirely,
B. I can leverage `dget`/`dput` to make it so that the directory I get
back from the APIs are actually owning, and so the dentry will not be
released while the Dir/SubDir exists. I understand this to be
undesired, but putting it out there, since it'd make things safe even
to `debugfs_lookup_and_remove()`.
C. I can limit use of `debugfs_remove` to the builder API (exists in
V1, no longer exists in this version, will be a separate patch soon
for reference). This is the behavior that I had written in V1 before I
started trying to adapt to requests. This means that the subdirectory
handles will remain valid because the non-builder handles will be
`dput` rather than recursively removed. Builder handles can't be
smuggled out due to an existential lifetime. Again, I understand this
is not desired, I'm just trying to lay out all ways to prevent this
class of error.

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Matthew Maurer]

On Thu, May 1, 2025 at 11:37 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > +/// Owning handle to a DebugFS directory.
> > +///
> > +/// This directory will be cleaned up when it goes out of scope.
> > +///
> > +/// # Invariants
> > +///
> > +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> > +#[repr(transparent)]
> > +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
>
> Should probably use Opaque instead of a raw pointer.

Opaque usage is problematic here:
1. It was explicitly requested in the first patch that I not expose
the reference counted nature of the dentry we were being provided. At
the same time, the dentry we are provided isn't a borrow - it doesn't
live for a fixed lifetime, it goes away when we clean it up. This
means that if I do `Dir(Opaque<dentry>)`, there's no way to represent
the return value of directory creation - it's not `Box<Dir>` or
`Arc<Dir>`, and I've been requested to hide the fact that it's
`ARef<Dir>` and not decrement the refcount myself. We can't know the
lifetime at the callsite, so we can't use a `&'a Dir` either. This
lands with the raw pointer wrapper. I could, technically, produce
`InnerDir(Opaque<dentry>)` and `Dir(ManuallyDrop<ARef<Dir>>)`, and
then impl `Drop` for `Dir` to `remove` instead of `dput`, but that
seems like a bunch of work to construct infrastructure and then
suppress it that doesn't actually help.
2. If you were suggesting `Dir(*mut Opaque<dentry>)`, I think this is
meaningless and will just cause cast noise. My understanding is that
the use of `Opaque` is to remove restrictions Rust has over legal
references and values, e.g. it's initialized, it's not being mutated,
thread safety invariants, etc. However, pointers in Rust explicitly do
not induce any of these requirements on their pointee (otherwise
`ptr::dangling()` would be immediate UB in many situations).
3. Unless we produce an `ARef<Opaque<dentry>>` at some point (which
again, we've been asked to pretend isn't there), I don't think there's
any way for the DebugFS bindings to produce a legal `&'a
Opaque<dentry>`, so there's not really a purpose.

>
> > +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
>
> [`Dir`]
>
> > +// between threads.
> > +unsafe impl Send for Dir {}
> > +
> > +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> > +// struct are opaque to Rust.
> > +unsafe impl Sync for Dir {}
> > +
> > +impl Dir {
> > +    /// Create a new directory in DebugFS at the root.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// {
> > +    ///    let parent = Dir::new(c_str!("parent"));
> > +    ///    // The path "parent" exists in DebugFS here.
> > +    /// }
> > +    /// // It does not exist here.
>
> This ready like an explanation for scopes; I think we should drop those comments
> and the scope.
>
> > +    /// ```
> > +    pub fn new(name: &CStr) -> Self {
> > +        Self::create(name, None)
> > +    }
> > +
> > +    /// Create a DebugFS subdirectory.
> > +    ///
> > +    /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> > +    /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// {
> > +    ///    let parent = Dir::new(c_str!("parent"));
> > +    ///    // The path "parent" exists in DebugFS here.
> > +    ///    {
> > +    ///        let child = parent.subdir(c_str!("child"));
> > +    ///        // The path "parent/child" exists in DebugFS here.
> > +    ///    }
> > +    ///    // The path "parent/child" still exists.
> > +    ///    {
> > +    ///        let child2 = Dir::from(parent.subdir(c_str!("child2")));
> > +    ///        // The path "parent/child2" exists in DebugFS here.
> > +    ///    }
> > +    ///    // The path "parent/child2" is gone.
> > +    /// }
> > +    /// // None of the paths exist here.
>
> I think the fact that you need all those comments here proves that it's not
> really intuitive. Please see me comment on SubDir below.
>
> > +    /// ```
> > +    pub fn subdir(&self, name: &CStr) -> SubDir {
> > +        SubDir::new(Self::create(name, Some(self)))
> > +    }
> > +
> > +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    fn create(name: &CStr, parent: Option<&Self>) -> Self {
> > +        let parent_ptr = match parent {
> > +            Some(parent) => parent.as_ptr(),
> > +            None => core::ptr::null_mut(),
> > +        };
> > +        // SAFETY:
> > +        // * `name` argument points to a NUL-terminated string that lives across the call, by
> > +        //   invariants of `&CStr`.
> > +        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> > +        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> > +        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > +        //   so we can call `Self::from_ptr`.
> > +        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
>
> Please split up in two calls, such that we don't have two unsafe function calls
> in a single unsafe block.
>
> > +    }
> > +
> > +    #[cfg(not(CONFIG_DEBUG_FS))]
> > +    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> > +        Self()
> > +    }
> > +
> > +    /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
> > +    /// live DebugFS directory.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
> > +        Self(ptr)
> > +    }
> > +
> > +    /// Returns the pointer representation of the DebugFS directory.
> > +    ///
> > +    /// Due to the type invariant, the value returned from this function will always be an error
> > +    /// code, NUL, or a live DebugFS directory.
>
> Maybe put this in a '# Guarantees' section.
>
> > +    // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
>
> I think you mean ERR_PTR(ENODEV).
>
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    fn as_ptr(&self) -> *mut bindings::dentry {
> > +        self.0
> > +    }
> > +}
> > +
> > +impl Drop for Dir {
> > +    fn drop(&mut self) {
> > +        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> > +        // `as_ptr` guarantees that the pointer is of this form.
> > +        #[cfg(CONFIG_DEBUG_FS)]
> > +        unsafe {
> > +            bindings::debugfs_remove(self.as_ptr())
> > +        }
> > +    }
> > +}
> > +
> > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > +#[repr(transparent)]
> > +pub struct SubDir(ManuallyDrop<Dir>);
>
> I think it's not very intuitive if the default is that a SubDir still exists
> after it has been dropped. I think your first approach being explicit about this
> with keep() consuming the SubDir was much better; please keep this approach.

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Danilo Krummrich]

On Fri, May 02, 2025 at 08:48:36AM -0700, Matthew Maurer wrote:
> On Thu, May 1, 2025 at 11:37 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > +/// Owning handle to a DebugFS directory.
> > > +///
> > > +/// This directory will be cleaned up when it goes out of scope.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> > > +#[repr(transparent)]
> > > +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
> >
> > Should probably use Opaque instead of a raw pointer.
> 
> Opaque usage is problematic here:

Yes, when I wrote this I was back at your v1 in my mind -- the raw pointer is
fine..

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Benno Lossin]

> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..b589c2d9a8d169bd66e98d2894261784e427230e
> --- /dev/null
> +++ b/rust/kernel/debugfs.rs
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Google LLC.
> +
> +//! DebugFS Abstraction
> +//!
> +//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
> +
> +use crate::str::CStr;
> +use core::mem::ManuallyDrop;
> +use core::ops::Deref;
> +
> +/// Owning handle to a DebugFS directory.
> +///
> +/// This directory will be cleaned up when it goes out of scope.

We should also document that it's a unit struct when `CONFIG_DEBUG_FS`
is disabled (and the operations are noops). Maybe even do something
like:

    #[cfg_attr(CONFIG_DEBUG_FS, doc = "`CONFIG_DEBUG_FS=y`")]
    #[cfg_attr(not(CONFIG_DEBUG_FS), doc = "`CONFIG_DEBUG_FS=n`")]

> +///
> +/// # Invariants
> +///
> +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> +#[repr(transparent)]
> +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
> +
> +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
> +// between threads.
> +unsafe impl Send for Dir {}
> +
> +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> +// struct are opaque to Rust.
> +unsafe impl Sync for Dir {}
> +
> +impl Dir {
> +    /// Create a new directory in DebugFS at the root.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// {
> +    ///    let parent = Dir::new(c_str!("parent"));
> +    ///    // The path "parent" exists in DebugFS here.
> +    /// }
> +    /// // It does not exist here.
> +    /// ```
> +    pub fn new(name: &CStr) -> Self {
> +        Self::create(name, None)
> +    }
> +
> +    /// Create a DebugFS subdirectory.
> +    ///
> +    /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> +    /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].

But it will be cleaned up when the parent goes out of scope? We should
also mention that.

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// {
> +    ///    let parent = Dir::new(c_str!("parent"));
> +    ///    // The path "parent" exists in DebugFS here.
> +    ///    {
> +    ///        let child = parent.subdir(c_str!("child"));
> +    ///        // The path "parent/child" exists in DebugFS here.
> +    ///    }
> +    ///    // The path "parent/child" still exists.
> +    ///    {
> +    ///        let child2 = Dir::from(parent.subdir(c_str!("child2")));
> +    ///        // The path "parent/child2" exists in DebugFS here.
> +    ///    }
> +    ///    // The path "parent/child2" is gone.
> +    /// }
> +    /// // None of the paths exist here.
> +    /// ```
> +    pub fn subdir(&self, name: &CStr) -> SubDir {
> +        SubDir::new(Self::create(name, Some(self)))
> +    }
> +
> +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    fn create(name: &CStr, parent: Option<&Self>) -> Self {
> +        let parent_ptr = match parent {
> +            Some(parent) => parent.as_ptr(),
> +            None => core::ptr::null_mut(),
> +        };
> +        // SAFETY:
> +        // * `name` argument points to a NUL-terminated string that lives across the call, by
> +        //   invariants of `&CStr`.
> +        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> +        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> +        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> +        //   so we can call `Self::from_ptr`.
> +        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }

What about when an error got returned? Should that be exposed to the
user?

> +    }
> +
> +    #[cfg(not(CONFIG_DEBUG_FS))]
> +    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> +        Self()
> +    }
> +

> +impl Drop for Dir {
> +    fn drop(&mut self) {
> +        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> +        // `as_ptr` guarantees that the pointer is of this form.
> +        #[cfg(CONFIG_DEBUG_FS)]
> +        unsafe {

I feel a bit uneasy with seeing `cfg` on `unsafe` code, since now the
correctness also depends on the configuration. Someone might add/modify
it making it incorrect under certain configurations.

This case is pretty straight forward, but I'm not so sure if we already
have such a case.

How about having two modules providing the two implementations and then
just conditionally import one or the other?

---
Cheers,
Benno

> +            bindings::debugfs_remove(self.as_ptr())
> +        }
> +    }
> +}

Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman]

On Fri, May 02, 2025 at 10:12:15AM +0200, Benno Lossin wrote:
> > +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    fn create(name: &CStr, parent: Option<&Self>) -> Self {
> > +        let parent_ptr = match parent {
> > +            Some(parent) => parent.as_ptr(),
> > +            None => core::ptr::null_mut(),
> > +        };
> > +        // SAFETY:
> > +        // * `name` argument points to a NUL-terminated string that lives across the call, by
> > +        //   invariants of `&CStr`.
> > +        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> > +        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> > +        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > +        //   so we can call `Self::from_ptr`.
> > +        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
> 
> What about when an error got returned? Should that be exposed to the
> user?

No, not at all.  See my comments on version 1 of this patchset.  No
error should ever go back to the caller, it should never know if a
debugfs call succeeded or not so that it can just keep moving forward
and not act any differently.

Many of the C debugfs apis are already changed to be this way, let's not
go backwards and add this logic to the rust code only to rip it out in
the future.

> > +    }
> > +
> > +    #[cfg(not(CONFIG_DEBUG_FS))]
> > +    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> > +        Self()
> > +    }
> > +
> 
> > +impl Drop for Dir {
> > +    fn drop(&mut self) {
> > +        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> > +        // `as_ptr` guarantees that the pointer is of this form.
> > +        #[cfg(CONFIG_DEBUG_FS)]
> > +        unsafe {
> 
> I feel a bit uneasy with seeing `cfg` on `unsafe` code, since now the
> correctness also depends on the configuration. Someone might add/modify
> it making it incorrect under certain configurations.

The option is either enabled or not, this should be fine.

> This case is pretty straight forward, but I'm not so sure if we already
> have such a case.
> 
> How about having two modules providing the two implementations and then
> just conditionally import one or the other?

That would require a lot more duplicated code that you then have to
always keep in sync.  And from past experience, that's hard to do over
time.  So let's do it this way if at all possible.

thanks,

greg k-h

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

[Matthew Maurer]

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 | 134 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 134 insertions(+)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index b589c2d9a8d169bd66e98d2894261784e427230e..ef69ae8f550a9fe6b0afc1c51bebaad2fc087811 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;
 use core::mem::ManuallyDrop;
 use core::ops::Deref;
 
@@ -118,6 +119,48 @@ unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
     fn as_ptr(&self) -> *mut bindings::dentry {
         self.0
     }
+
+    /// 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);
+    /// // "my_debugfs_dir/foo" now contains the number 200.
+    /// ```
+    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> 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.
+        // * 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)]
+        let dir = 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))]
+        let dir = {
+            // Mark parameters used
+            let (_, _) = (name, data);
+            Self()
+        };
+        File(ManuallyDrop::new(dir))
+    }
 }
 
 impl Drop for Dir {
@@ -153,3 +196,94 @@ fn new(dir: Dir) -> Self {
         Self(ManuallyDrop::new(dir))
     }
 }
+/// Handle to a DebugFS file.
+#[repr(transparent)]
+pub struct File(ManuallyDrop<Dir>);
+
+impl File {
+    /// Remove the file from DebugFS.
+    ///
+    /// # Examples
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("foo"));
+    /// {
+    ///     let file = dir.display_file(c_str!("bar"), &0);
+    ///     // "foo/bar" is created.
+    /// }
+    /// // "foo/bar" still exists.
+    /// {
+    ///     let file = dir.display_file(c_str!("baz"), &0);
+    ///     // "foo/baz" is created.
+    ///     file.remove();
+    ///     // "foo/baz" is gone.
+    /// }
+    pub fn remove(self) {
+        drop(ManuallyDrop::into_inner(self.0))
+    }
+}
+
+#[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 v3 2/4] rust: debugfs: Bind file creation for long-lived Display

[Danilo Krummrich]

On Thu, May 01, 2025 at 10:47:42PM +0000, Matthew Maurer wrote:
> +/// Handle to a DebugFS file.
> +#[repr(transparent)]
> +pub struct File(ManuallyDrop<Dir>);

Same as with SubDir, please keep your original approach with keep().

Exposing this as a separate type is much better, but I still find it a bit weird
that it uses Dir internally, which still provides methods that are not
applicable.

I think it would be good to have the following types instead:

	// Generic wrapper around the dentry pointer.
	struct Entry;

	// Based on Entry; provides Dir specific methods.
	struct Dir;

	// Based on Dir; implements Keep.
	struct SubDir;

	// Based on Entry; implements Keep.
	struct File;

	// Common trait that implements keep().
	trait Keep;

> +impl File {
> +    /// Remove the file from DebugFS.
> +    ///
> +    /// # Examples
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let dir = Dir::new(c_str!("foo"));
> +    /// {
> +    ///     let file = dir.display_file(c_str!("bar"), &0);
> +    ///     // "foo/bar" is created.
> +    /// }
> +    /// // "foo/bar" still exists.
> +    /// {
> +    ///     let file = dir.display_file(c_str!("baz"), &0);
> +    ///     // "foo/baz" is created.
> +    ///     file.remove();
> +    ///     // "foo/baz" is gone.
> +    /// }
> +    pub fn remove(self) {
> +        drop(ManuallyDrop::into_inner(self.0))
> +    }
> +}

Same as with my comment on Dir::subdir(), it really gets confusing if we invert
the normal drop() logic. Removing the file when it is dropped and keeping it
when calling keep() is much more intuitive..

> +
> +#[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) }

Please split up unsafe operations.

> +    }
> +
> +    /// 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) };

This creates an intermediate reference to private, which is UB. Please use
addr_of! instead.

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

[Matthew Maurer]

On Thu, May 1, 2025 at 11:52 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu, May 01, 2025 at 10:47:42PM +0000, Matthew Maurer wrote:
> > +/// Handle to a DebugFS file.
> > +#[repr(transparent)]
> > +pub struct File(ManuallyDrop<Dir>);
>
> Same as with SubDir, please keep your original approach with keep().
>
> Exposing this as a separate type is much better, but I still find it a bit weird
> that it uses Dir internally, which still provides methods that are not
> applicable.
>
> I think it would be good to have the following types instead:
>
>         // Generic wrapper around the dentry pointer.
>         struct Entry;
>
>         // Based on Entry; provides Dir specific methods.
>         struct Dir;
>
>         // Based on Dir; implements Keep.
>         struct SubDir;
>
>         // Based on Entry; implements Keep.
>         struct File;
>
>         // Common trait that implements keep().
>         trait Keep;
>
> > +impl File {
> > +    /// Remove the file from DebugFS.
> > +    ///
> > +    /// # Examples
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// let dir = Dir::new(c_str!("foo"));
> > +    /// {
> > +    ///     let file = dir.display_file(c_str!("bar"), &0);
> > +    ///     // "foo/bar" is created.
> > +    /// }
> > +    /// // "foo/bar" still exists.
> > +    /// {
> > +    ///     let file = dir.display_file(c_str!("baz"), &0);
> > +    ///     // "foo/baz" is created.
> > +    ///     file.remove();
> > +    ///     // "foo/baz" is gone.
> > +    /// }
> > +    pub fn remove(self) {
> > +        drop(ManuallyDrop::into_inner(self.0))
> > +    }
> > +}
>
> Same as with my comment on Dir::subdir(), it really gets confusing if we invert
> the normal drop() logic. Removing the file when it is dropped and keeping it
> when calling keep() is much more intuitive..
>
> > +
> > +#[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) }
>
> Please split up unsafe operations.
>
> > +    }
> > +
> > +    /// 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) };
>
> This creates an intermediate reference to private, which is UB. Please use
> addr_of! instead.

I'm making this change, but so that I can be correct in the future,
can you explain why taking an intermediate reference to private is UB?
My understanding is that my provided vtable are supposed to be the
only methods looking at this field, and they don't mutate it.
Additionally, the `private_data` field on file is accessed this way in
`miscdevice` at the moment - what makes it safe there, and UB here?

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

[Danilo Krummrich]

On Fri, May 02, 2025 at 11:07:47AM -0700, Matthew Maurer wrote:
> I'm making this change, but so that I can be correct in the future,
> can you explain why taking an intermediate reference to private is UB?
> My understanding is that my provided vtable are supposed to be the
> only methods looking at this field, and they don't mutate it.

You're right, I confused it with something else.

[PATCH v3 3/4] rust: debugfs: Support format hooks

[Matthew Maurer]

Rather than always using Display, allow hooking arbitrary functions to
arbitrary files. Display technically has the expressiveness to do this,
but requires a new type be declared for every different way to render
things, which can be very clumsy.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index ef69ae8f550a9fe6b0afc1c51bebaad2fc087811..9689e21cb24903e9f5a21fb93e6652adc5cafbdc 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;
 use core::fmt::Display;
 use core::mem::ManuallyDrop;
 use core::ops::Deref;
@@ -133,6 +134,48 @@ fn as_ptr(&self) -> *mut bindings::dentry {
     /// // "my_debugfs_dir/foo" now contains the number 200.
     /// ```
     pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
+        // SAFETY: As `data` lives for the static lifetime, it outlives the file.
+        unsafe { self.display_file_raw(name, data) }
+    }
+
+    /// 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 core::sync::atomic::{AtomicU32, Ordering};
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("foo"));
+    /// static MY_ATOMIC: AtomicU32 = AtomicU32::new(3);
+    /// let file = dir.fmt_file(c_str!("bar"), &MY_ATOMIC, &|val, f| {
+    ///   let out = val.load(Ordering::Relaxed);
+    ///   writeln!(f, "{out:#010x}")
+    /// });
+    /// MY_ATOMIC.store(10, Ordering::Relaxed);
+    /// ```
+    pub fn fmt_file<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
+        name: &CStr,
+        data: &'static T,
+        f: &'static F,
+    ) -> File {
+        // SAFETY: As `data` lives for the static lifetime, it outlives the file
+        unsafe { self.fmt_file_raw(name, data, f) }
+    }
+
+    /// Creates a DebugFS file backed by the display implementation of the provided pointer.
+    ///
+    /// # Safety
+    ///
+    /// The pointee of `data` must outlive the accessibility of the `Dir` returned by this function.
+    /// 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<T: Display + Sized>(&self, name: &CStr, data: *const T) -> 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.
@@ -161,6 +204,32 @@ pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) ->
         };
         File(ManuallyDrop::new(dir))
     }
+
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking the
+    /// fomatter on the attached data.
+    ///
+    /// The attached function must be a ZST, and will cause a compilation error if it is not.
+    ///
+    /// # Safety
+    ///
+    /// `data` must outlive the resulting file's accessibility
+    unsafe fn fmt_file_raw<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
+        name: &CStr,
+        data: &T,
+        f: &'static F,
+    ) -> File {
+        #[cfg(CONFIG_DEBUG_FS)]
+        let data_adapted = FormatAdapter::new(data, f);
+        #[cfg(not(CONFIG_DEBUG_FS))]
+        let data_adapted = {
+            // Mark used
+            let (_, _) = (data, f);
+            &0
+        };
+        // SAFETY: data outlives the file's accessibility, so data_adapted does too
+        unsafe { self.display_file_raw(name, data_adapted) }
+    }
 }
 
 impl Drop for Dir {
@@ -228,7 +297,9 @@ pub fn remove(self) {
 mod helpers {
     use crate::seq_file::SeqFile;
     use crate::seq_print;
-    use core::fmt::Display;
+    use core::fmt;
+    use core::fmt::{Display, Formatter};
+    use core::marker::PhantomData;
 
     /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`.
     ///
@@ -283,6 +354,51 @@ pub(crate) trait DisplayFile: Display + Sized {
     }
 
     impl<T: Display + Sized> DisplayFile for T {}
+
+    /// Adapter to implement `Display` via a callback with the same representation as `T`.
+    ///
+    /// # Invariants
+    ///
+    /// If an instance for `FormatAdapter<_, F>` is constructed, `F` is inhabited.
+    #[repr(transparent)]
+    pub(crate) struct FormatAdapter<T, F> {
+        inner: T,
+        _formatter: PhantomData<F>,
+    }
+
+    impl<T, F> FormatAdapter<T, F> {
+        pub(crate) fn new<'a>(inner: &'a T, _f: &'static F) -> &'a Self {
+            // SAFETY: FormatAdapater is a repr(transparent) wrapper around T, so
+            // casting a reference is legal
+            // INVARIANT: We were passed a reference to F, so it is inhabited.
+            unsafe { core::mem::transmute(inner) }
+        }
+    }
+
+    impl<T, F> Display for FormatAdapter<T, F>
+    where
+        F: Fn(&T, &mut Formatter<'_>) -> fmt::Result + 'static,
+    {
+        fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
+            // SAFETY: FormatAdapter<_, F> can only be constructed if F is inhabited
+            let f: &F = unsafe { materialize_zst_fmt() };
+            f(&self.inner, fmt)
+        }
+    }
+
+    /// For types with a unique value, produce a static reference to it.
+    ///
+    /// # Safety
+    ///
+    /// The caller asserts that F is inhabited
+    unsafe fn materialize_zst_fmt<F>() -> &'static F {
+        const { assert!(core::mem::size_of::<F>() == 0) };
+        let zst_dangle: core::ptr::NonNull<F> = core::ptr::NonNull::dangling();
+        // SAFETY: While the pointer is dangling, it is a dangling pointer to a ZST, based on the
+        // assertion above. The type is also inhabited, by the caller's assertion. This means
+        // we can materialize it.
+        unsafe { zst_dangle.as_ref() }
+    }
 }
 
 #[cfg(CONFIG_DEBUG_FS)]

-- 
2.49.0.906.g1f30a19c02-goog

[PATCH v3 4/4] rust: samples: Add debugfs sample

[Matthew Maurer]

Provides an example of using the Rust DebugFS bindings.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 MAINTAINERS                  |  1 +
 samples/rust/Kconfig         | 11 +++++++++
 samples/rust/Makefile        |  1 +
 samples/rust/rust_debugfs.rs | 54 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 67 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a3b835e427b083a4ddd690d9e7739851f0af47ae..426bcdac025134e20911de8e2cf5c9efb0591814 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7278,6 +7278,7 @@ F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
 F:	rust/kernel/faux.rs
 F:	rust/kernel/platform.rs
+F:	samples/rust/rust_debugfs.rs
 F:	samples/rust/rust_driver_platform.rs
 F:	samples/rust/rust_driver_faux.rs
 
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 43cb72d72631bb2d6e06185e1d88778edff6ee13..6c42ed73f842cda26256039e6917bb443738d3f1 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -51,6 +51,17 @@ config SAMPLE_RUST_DMA
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DEBUGFS
+	tristate "DebugFS Test Driver"
+	depends on DEBUG_FS
+	help
+	  This option builds the Rust DebugFS Test driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_debugfs.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_DRIVER_PCI
 	tristate "PCI Driver"
 	depends on PCI
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 6a466afd2a21eba84a3b7b2be29f25dce44e9053..b1fc4677ed53fcf7d0f5a3dbf322f65851bc1784 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -4,6 +4,7 @@ ccflags-y += -I$(src)				# needed for trace events
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_DEBUGFS)		+= rust_debugfs.o
 obj-$(CONFIG_SAMPLE_RUST_DMA)			+= rust_dma.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
diff --git a/samples/rust/rust_debugfs.rs b/samples/rust/rust_debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..b22255667f3bb91b10555ac8b91be850cd5f172e
--- /dev/null
+++ b/samples/rust/rust_debugfs.rs
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! Sample DebugFS exporting module
+
+use core::sync::atomic::{AtomicU32, Ordering};
+use kernel::c_str;
+use kernel::debugfs::Dir;
+use kernel::prelude::*;
+
+module! {
+    type: RustDebugFs,
+    name: "rust_debugfs",
+    authors: ["Matthew Maurer"],
+    description: "Rust DebugFS usage sample",
+    license: "GPL",
+}
+
+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 EXAMPLE: AtomicU32 = AtomicU32::new(8);
+
+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"));
+
+        // Create a subdirectory, so "sample_debugfs/subdir" now exists.
+        let sub = debugfs.subdir(c_str!("subdir"));
+
+        // Create a single file in the subdirectory called "example" that will read from the
+        // `EXAMPLE` atomic variable.
+        sub.fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
+            writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
+        });
+        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 8\n" when read.
+
+        // Change the value in the variable displayed by the file. This is intended to demonstrate
+        // that the module can continue to change the value used by the file.
+        EXAMPLE.store(10, Ordering::Relaxed);
+        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 10\n" when read.
+
+        // 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
+        // continue to exist until the module is unloaded.
+        Ok(Self { _debugfs: debugfs })
+    }
+}

-- 
2.49.0.906.g1f30a19c02-goog

Re: [PATCH v3 4/4] rust: samples: Add debugfs sample

[Danilo Krummrich]

On Thu, May 01, 2025 at 10:47:44PM +0000, Matthew Maurer wrote:
> +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.

This feels like an introduction to Rust in the kernel for Rust beginners, which
is a great thing!

However, I wonder if, instead, we may want a dedicated (or even multiple) sample
modules or sample drivers, where we go into such detail and leave those samples
to focus only on the corresponding API?

> +    _debugfs: Dir,
> +}
> +
> +static EXAMPLE: AtomicU32 = AtomicU32::new(8);
> +
> +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"));
> +
> +        // Create a subdirectory, so "sample_debugfs/subdir" now exists.
> +        let sub = debugfs.subdir(c_str!("subdir"));
> +
> +        // Create a single file in the subdirectory called "example" that will read from the
> +        // `EXAMPLE` atomic variable.
> +        sub.fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
> +            writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
> +        });
> +        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 8\n" when read.
> +
> +        // Change the value in the variable displayed by the file. This is intended to demonstrate
> +        // that the module can continue to change the value used by the file.
> +        EXAMPLE.store(10, Ordering::Relaxed);
> +        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 10\n" when read.
> +
> +        // 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
> +        // continue to exist until the module is unloaded.

Same with this comment.

@Greg: I know you proposed this one; for educational purposes I suppose. What's
your take on the above?

Re: [PATCH v3 4/4] rust: samples: Add debugfs sample

[Greg Kroah-Hartman]

On Fri, May 02, 2025 at 09:01:01AM +0200, Danilo Krummrich wrote:
> On Thu, May 01, 2025 at 10:47:44PM +0000, Matthew Maurer wrote:
> > +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.
> 
> This feels like an introduction to Rust in the kernel for Rust beginners, which
> is a great thing!
> 
> However, I wonder if, instead, we may want a dedicated (or even multiple) sample
> modules or sample drivers, where we go into such detail and leave those samples
> to focus only on the corresponding API?

People search for the specific example of what they want to do, so I
recommend just leaving all of these comments in for now as knowing to
search for a different example is usually not going to happen :)


> 
> > +    _debugfs: Dir,
> > +}
> > +
> > +static EXAMPLE: AtomicU32 = AtomicU32::new(8);
> > +
> > +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"));
> > +
> > +        // Create a subdirectory, so "sample_debugfs/subdir" now exists.
> > +        let sub = debugfs.subdir(c_str!("subdir"));
> > +
> > +        // Create a single file in the subdirectory called "example" that will read from the
> > +        // `EXAMPLE` atomic variable.
> > +        sub.fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
> > +            writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
> > +        });
> > +        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 8\n" when read.
> > +
> > +        // Change the value in the variable displayed by the file. This is intended to demonstrate
> > +        // that the module can continue to change the value used by the file.
> > +        EXAMPLE.store(10, Ordering::Relaxed);
> > +        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 10\n" when read.
> > +
> > +        // 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
> > +        // continue to exist until the module is unloaded.
> 
> Same with this comment.
> 
> @Greg: I know you proposed this one; for educational purposes I suppose. What's
> your take on the above?

I like it and want to see this here as it helps explain both how to use
this, AND how to have maintainers review code that uses the api that are
not that familiar with how Rust does things.

thanks,

greg k-h

Re: [PATCH v3 4/4] rust: samples: Add debugfs sample

[Danilo Krummrich]

On Fri, May 02, 2025 at 09:13:28AM +0200, Greg Kroah-Hartman wrote:
> AND how to have maintainers review code that uses the api that are
> not that familiar with how Rust does things.

That sounds very reasonable -- I like it!