[PATCH v5 0/4] rust: DebugFS Bindings

[PATCH v5 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.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
Changes in v5:
- Made Dir + File wrappers around Entry
- All functions return owning handles. To discard without drop, use
  `forget`. To keep a handle without drop, use `ManuallyDrop`.
- Fixed bugs around `not(CONFIG_DEBUG_FS)`
- Removed unnecessary `addr_of!`
- Link to v4: https://lore.kernel.org/r/20250502-debugfs-rust-v4-0-788a9c6c2e77@google.com

Changes in v4:
- Remove SubDir, replace with type-level constant.
- Add lifetime to Dir to prevent subdirectories and files from outliving
  their parents and triggering an Oops when accessed.
- Split unsafe blocks with two calls into two blocks
- Access `private` field through direct pointer dereference, avoiding
  creation of a reference to it.
- Notably not changed - owning/non-owning handle defaults. The best read
  I had from the thread was to continue with this mode, but I'm willing
  to change if need be.
- Comment changes
  - More comment markdown
  - Remove scopes from examples
  - Put `as_ptr` properties into a `# Guarantees` section.
- Link to v3: https://lore.kernel.org/r/20250501-debugfs-rust-v3-0-850869fab672@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          | 392 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 samples/rust/Kconfig            |  11 ++
 samples/rust/Makefile           |   1 +
 samples/rust/rust_debugfs.rs    |  60 ++++++
 7 files changed, 468 insertions(+)
---
base-commit: b6a7783d306baf3150ac54cd5124f6e85dd375b0
change-id: 20250428-debugfs-rust-3cd5c97eb7d1

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

[PATCH v5 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          | 139 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 4 files changed, 142 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..ed1aba6d700d064dbfd7e923dbcbf80b9acf5361
--- /dev/null
+++ b/rust/kernel/debugfs.rs
@@ -0,0 +1,139 @@
+// 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::marker::PhantomData;
+
+/// Owning handle to a DebugFS entry.
+///
+/// # Invariants
+///
+/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
+#[repr(transparent)]
+struct Entry<'a> {
+    #[cfg(CONFIG_DEBUG_FS)]
+    entry: *mut bindings::dentry,
+    // We need to be outlived by our parent, if they exist, but we don't actually need to be able
+    // to access the data.
+    _phantom: PhantomData<&'a Entry<'a>>,
+}
+
+// SAFETY: [`Entry`] is just a `dentry` under the hood, which the API promises can be transferred
+// between threads.
+unsafe impl Send for Entry<'_> {}
+
+// 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 Entry<'_> {}
+
+impl<'a> Entry<'a> {
+    /// Constructs a new DebugFS [`Entry`] 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. If this is a child directory or file, `'a` must be less than the
+    /// lifetime of the parent directory.
+    #[cfg(CONFIG_DEBUG_FS)]
+    unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
+        Self {
+            entry,
+            _phantom: PhantomData,
+        }
+    }
+
+    #[cfg(not(CONFIG_DEBUG_FS))]
+    fn new() -> Self {
+        Self {
+            _phantom: PhantomData,
+        }
+    }
+
+    /// Returns the pointer representation of the DebugFS directory.
+    ///
+    /// # Guarantees
+    ///
+    /// 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
+    // `ERR_PTR(ENODEV)`.
+    #[cfg(CONFIG_DEBUG_FS)]
+    fn as_ptr(&self) -> *mut bindings::dentry {
+        self.entry
+    }
+}
+
+impl Drop for Entry<'_> {
+    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())
+        }
+    }
+}
+
+/// Owning handle to a DebugFS directory.
+///
+/// This directory will be cleaned up when the handle leaves scope.
+pub struct Dir<'a>(Entry<'a>);
+
+impl<'a> Dir<'a> {
+    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
+    #[cfg(CONFIG_DEBUG_FS)]
+    fn create<'b>(name: &CStr, parent: Option<&'a Dir<'b>>) -> Self {
+        let parent_ptr = match parent {
+            Some(parent) => parent.0.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.
+        //   so we can call `Self::from_ptr`.
+        let dir = unsafe { bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr) };
+
+        // SAFETY: `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
+        Self(unsafe { Entry::from_ptr(dir) })
+    }
+
+    #[cfg(not(CONFIG_DEBUG_FS))]
+    fn create<'b>(_name: &CStr, _parent: Option<&'a Dir<'b>>) -> Self {
+        Self(Entry::new())
+    }
+
+    /// Create a DebugFS subdirectory.
+    ///
+    /// Subdirectory handles cannot outlive the directory handle they were created from.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let parent = Dir::new(c_str!("parent"));
+    /// let child = parent.subdir(c_str!("child"));
+    /// ```
+    pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
+        Dir::create(name, Some(self))
+    }
+
+    /// Create a new directory in DebugFS at the root.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let debugfs = Dir::new(c_str!("parent"));
+    /// ```
+    pub fn new(name: &CStr) -> Self {
+        Dir::create(name, None)
+    }
+}
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.967.g6a0df3ecc3-goog

[PATCH v5 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 | 132 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index ed1aba6d700d064dbfd7e923dbcbf80b9acf5361..4a138717bd0fdb320033d07446a192c9f520a17b 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -6,6 +6,7 @@
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
 use crate::str::CStr;
+use core::fmt::Display;
 use core::marker::PhantomData;
 
 /// Owning handle to a DebugFS entry.
@@ -46,6 +47,19 @@ unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
         }
     }
 
+    /// Constructs a new DebugFS [`Entry`] 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(not(CONFIG_DEBUG_FS))]
+    unsafe fn from_ptr(_entry: *mut bindings::dentry) -> Self {
+        Self {
+            _phantom: PhantomData,
+        }
+    }
+
     #[cfg(not(CONFIG_DEBUG_FS))]
     fn new() -> Self {
         Self {
@@ -124,6 +138,57 @@ pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
         Dir::create(name, Some(self))
     }
 
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
+    /// [`Display::fmt`] on the provided reference.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
+    /// dir.display_file(c_str!("foo"), &200);
+    /// // "my_debugfs_dir/foo" now contains the number 200.
+    /// ```
+    pub fn display_file<'b, T: Display + Sized>(
+        &'b self,
+        name: &CStr,
+        data: &'static T,
+    ) -> File<'b> {
+        // 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.
+        #[cfg(CONFIG_DEBUG_FS)]
+        let ptr = unsafe {
+            bindings::debugfs_create_file_full(
+                name.as_char_ptr(),
+                0o444,
+                self.0.as_ptr(),
+                data as *const _ as *mut _,
+                core::ptr::null(),
+                &<T as DisplayFile>::VTABLE,
+            )
+        };
+
+        #[cfg(not(CONFIG_DEBUG_FS))]
+        let ptr = {
+            // Mark parameters used
+            let (_, _) = (name, data);
+            crate::error::code::ENODEV.to_ptr()
+        };
+
+        // SAFETY: `debugfs_create_file_full` either returns an error code or a legal
+        // dentry pointer, and without `CONFIG_DEBUGFS` we return an error pointer, so
+        // `Entry::from_ptr` is safe to call here.
+        let entry = unsafe { Entry::from_ptr(ptr) };
+
+        File(entry)
+    }
+
     /// Create a new directory in DebugFS at the root.
     ///
     /// # Examples
@@ -137,3 +202,70 @@ pub fn new(name: &CStr) -> Self {
         Dir::create(name, None)
     }
 }
+/// Handle to a DebugFS file.
+#[repr(transparent)]
+pub struct File<'a>(Entry<'a>);
+
+#[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.967.g6a0df3ecc3-goog

[PATCH v5 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 | 123 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 4a138717bd0fdb320033d07446a192c9f520a17b..e2f5960bb87f24607780b3f4e67039e379f3bda6 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -6,6 +6,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::marker::PhantomData;
 
@@ -154,6 +155,52 @@ pub fn display_file<'b, T: Display + Sized>(
         &'b self,
         name: &CStr,
         data: &'static T,
+    ) -> File<'b> {
+        // 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<'b, T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &'b self,
+        name: &CStr,
+        data: &'static T,
+        f: &'static F,
+    ) -> File<'b> {
+        // 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<'b, T: Display + Sized>(
+        &'b self,
+        name: &CStr,
+        data: *const T,
     ) -> File<'b> {
         // SAFETY:
         // * `name` is a NUL-terminated C string, living across the call, by `CStr` invariant.
@@ -189,6 +236,32 @@ pub fn display_file<'b, T: Display + Sized>(
         File(entry)
     }
 
+    /// 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<'b, T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &'b self,
+        name: &CStr,
+        data: &T,
+        f: &'static F,
+    ) -> File<'b> {
+        #[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) }
+    }
+
     /// Create a new directory in DebugFS at the root.
     ///
     /// # Examples
@@ -202,6 +275,7 @@ pub fn new(name: &CStr) -> Self {
         Dir::create(name, None)
     }
 }
+
 /// Handle to a DebugFS file.
 #[repr(transparent)]
 pub struct File<'a>(Entry<'a>);
@@ -210,7 +284,9 @@ pub fn new(name: &CStr) -> 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`.
     ///
@@ -265,6 +341,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.967.g6a0df3ecc3-goog

[PATCH v5 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 | 60 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 73 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..a4b17c8241330b2f6caf8f17a5b2366138de6ced
--- /dev/null
+++ b/samples/rust/rust_debugfs.rs
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! Sample DebugFS exporting module
+
+use core::mem::{forget, ManuallyDrop};
+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>,
+}
+
+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.
+            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
+            // at the end of the scope - it will be cleaned up when `debugfs` is.
+            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
+
+            // Create a single file in the subdirectory called "example" that will read from the
+            // `EXAMPLE` atomic variable.
+            // We `forget` the result to avoid deleting the file at the end of the scope.
+            forget(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.967.g6a0df3ecc3-goog

Re: [PATCH v5 0/4] rust: DebugFS Bindings

[Danilo Krummrich]

On Mon, May 05, 2025 at 11:51:33PM +0000, Matthew Maurer wrote:
> Changes in v5:
> - Made Dir + File wrappers around Entry
> - All functions return owning handles. To discard without drop, use
>   `forget`. To keep a handle without drop, use `ManuallyDrop`.
> - Fixed bugs around `not(CONFIG_DEBUG_FS)`
> - Removed unnecessary `addr_of!`
> - Link to v4: https://lore.kernel.org/r/20250502-debugfs-rust-v4-0-788a9c6c2e77@google.com

I will go through v5 latest by beginning of next week, thanks!

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

[Timur Tabi]

On Mon, 2025-05-05 at 23:51 +0000, Matthew Maurer wrote:
> 
> +impl<'a> Entry<'a> {
> +    /// Constructs a new DebugFS [`Entry`] 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. If this is a child directory or file, `'a` must be less than the
> +    /// lifetime of the parent directory.
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
> +        Self {
> +            entry,
> +            _phantom: PhantomData,
> +        }
> +    }
> +
> +    #[cfg(not(CONFIG_DEBUG_FS))]
> +    fn new() -> Self {
> +        Self {
> +            _phantom: PhantomData,
> +        }
> +    }

I am new to Rust, so forgive me if this is a dumb question, but it looks to me that if
CONFIG_DEBUG_FS is defined, then you need to call from_ptr() to create a new Entry, but if
CONFIG_DEBUG_FS is not defined, then you need to call new() instead.  Is that right?  If so, is that
really idiomatic?

In the Dir implementation below, you are careful to call from_ptr() only from the CONFIG_DEBUG_FS
version of create(), and you call new() only from the !CONFIG_DEBUG_FS version of create().  So your
bases are covered as long as no driver tries to create an Entry from scratch. 

But I guess that can't happen because Entry is not public, right?

> +    /// Create a DebugFS subdirectory.
> +    ///
> +    /// Subdirectory handles cannot outlive the directory handle they were created from.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let parent = Dir::new(c_str!("parent"));
> +    /// let child = parent.subdir(c_str!("child"));
> +    /// ```
> +    pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
> +        Dir::create(name, Some(self))
> +    }
> +
> +    /// Create a new directory in DebugFS at the root.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let debugfs = Dir::new(c_str!("parent"));
> +    /// ```
> +    pub fn new(name: &CStr) -> Self {
> +        Dir::create(name, None)
> +    }

Is there any real value to having two constructors, just to avoid passing None for the one time that
a root directory will be created?  The C code has no problem passing NULL.

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

[Timur Tabi]

On Mon, 2025-05-05 at 23:51 +0000, Matthew Maurer wrote:
> +    /// Constructs a new DebugFS [`Entry`] 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(not(CONFIG_DEBUG_FS))]
> +    unsafe fn from_ptr(_entry: *mut bindings::dentry) -> Self {
> +        Self {
> +            _phantom: PhantomData,
> +        }
> +    }
> +

Does this diff belong in patch 1/4?  That would explain my confusion.

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

[Timur Tabi]

On Mon, 2025-05-05 at 23:51 +0000, Matthew Maurer wrote:

> +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> +    /// [`Display::fmt`] on the provided reference.

Is there a typo in this sentence?  I can't quite parse it.

> +    pub fn display_file<'b, T: Display + Sized>(
> +        &'b self,
> +        name: &CStr,
> +        data: &'static T,
> +    ) -> File<'b> {
> +        // 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.
> +        #[cfg(CONFIG_DEBUG_FS)]
> +        let ptr = unsafe {
> +            bindings::debugfs_create_file_full(
> +                name.as_char_ptr(),
> +                0o444,

Can you make the mode a parameter?  I get that you're not supporting writing yet, but there should
be a choice as to whether it's 0o444, 0o440, or 0o400.

Also, maybe use S_IRUSR, S_IRGRP, and S_IROTH?

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

[Alice Ryhl]

On Tue, May 6, 2025 at 1:51 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> Allows creation of files for references that live forever and lack
> metadata through the `Display` implementation.
>
> The reference must live forever because we do not have a maximum
> lifetime for the file we are creating.
>
> The `Display` implementation is used because `seq_printf` needs to route
> through `%pA`, which in turn routes through Arguments. A more generic
> API is provided later in the series, implemented in terms of this one.
>
> Signed-off-by: Matthew Maurer <mmaurer@google.com>

I believe it should be possible to bind owned data to a `File` using a
signature like this:

fn create_file<T>(&self, name: &CStr, data: impl PinInit<T>) -> impl
PinInit<FileWithData<T>>

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

[Timur Tabi]

On Mon, 2025-05-05 at 23:51 +0000, Matthew Maurer wrote:
> +    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);

Doesn't this restrict T to data types that are supported by "{}"?  So, for example, T cannot be a
Vec, correct?

For nova-core, we need to be able to "print" an array of bytes as-is.  Specifically, a DMA buffer
that just contains binary data.  But by using seq_print!, aren't we forcing T to contain only
printable characters?

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

[Benno Lossin]

On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> +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.
> +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));

I dislike the direct usage of `ManuallyDrop`. To me the usage of
`ManuallyDrop` signifies that one has to opt out of `Drop` without the
support of the wrapped type. But in this case, `Dir` is sometimes
intended to not be dropped, so I'd rather have a `.keep()` function I
saw mentioned somewhere.

> +
> +            // Create a single file in the subdirectory called "example" that will read from the
> +            // `EXAMPLE` atomic variable.
> +            // We `forget` the result to avoid deleting the file at the end of the scope.
> +            forget(sub.fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
> +                writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
> +            }));

Similarly here, I'd rather have a `.keep()` function than people start
using `forget` all over the place.

---
Cheers,
Benno

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

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

[Benno Lossin]

On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> +impl<'a> Dir<'a> {
> +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    fn create<'b>(name: &CStr, parent: Option<&'a Dir<'b>>) -> Self {
> +        let parent_ptr = match parent {
> +            Some(parent) => parent.0.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.
> +        //   so we can call `Self::from_ptr`.
> +        let dir = unsafe { bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr) };
> +
> +        // SAFETY: `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> +        Self(unsafe { Entry::from_ptr(dir) })
> +    }
> +
> +    #[cfg(not(CONFIG_DEBUG_FS))]
> +    fn create<'b>(_name: &CStr, _parent: Option<&'a Dir<'b>>) -> Self {
> +        Self(Entry::new())
> +    }
> +
> +    /// Create a DebugFS subdirectory.

I'm not familiar with debugfs, if I run `Dir::create(c"foo", None)`
twice, will both of the returned values refer to the same or different
directories? What if I give a parent?
If the answer in both cases is that they will refer to the same
directory, then I'd change the docs to mention that. So instead of
"Creates" we could say "Finds or creates" or something better.
If they refer to different files, then I am confused how that would look
like in user-land :)

> +    ///
> +    /// Subdirectory handles cannot outlive the directory handle they were created from.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let parent = Dir::new(c_str!("parent"));
> +    /// let child = parent.subdir(c_str!("child"));
> +    /// ```
> +    pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
> +        Dir::create(name, Some(self))
> +    }
> +
> +    /// Create a new directory in DebugFS at the root.

Ditto here.

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let debugfs = Dir::new(c_str!("parent"));
> +    /// ```
> +    pub fn new(name: &CStr) -> Self {
> +        Dir::create(name, None)
> +    }

I think it would make more sense for this function to return
`Dir<'static>`.

---
Cheers,
Benno

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

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

[Benno Lossin]

On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index ed1aba6d700d064dbfd7e923dbcbf80b9acf5361..4a138717bd0fdb320033d07446a192c9f520a17b 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -46,6 +47,19 @@ unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
>          }
>      }
>  
> +    /// Constructs a new DebugFS [`Entry`] 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(not(CONFIG_DEBUG_FS))]
> +    unsafe fn from_ptr(_entry: *mut bindings::dentry) -> Self {
> +        Self {

Why duplicate this function and not just do this to the existing
function?:

    unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
        #[cfg(not(CONFIG_DEBUG_FS))]
        let _ = entry;
        Self {
            #[cfg(CONFIG_DEBUG_FS)]
            entry,
            _phantom: PhantomData,
        }
    }

> +            _phantom: PhantomData,
> +        }
> +    }
> +
>      #[cfg(not(CONFIG_DEBUG_FS))]
>      fn new() -> Self {
>          Self {
> @@ -124,6 +138,57 @@ pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
>          Dir::create(name, Some(self))
>      }
>  
> +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> +    /// [`Display::fmt`] on the provided reference.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
> +    /// dir.display_file(c_str!("foo"), &200);
> +    /// // "my_debugfs_dir/foo" now contains the number 200.
> +    /// ```
> +    pub fn display_file<'b, T: Display + Sized>(
> +        &'b self,
> +        name: &CStr,
> +        data: &'static T,
> +    ) -> File<'b> {
> +        // 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.
> +        #[cfg(CONFIG_DEBUG_FS)]
> +        let ptr = unsafe {
> +            bindings::debugfs_create_file_full(
> +                name.as_char_ptr(),
> +                0o444,
> +                self.0.as_ptr(),
> +                data as *const _ as *mut _,
> +                core::ptr::null(),
> +                &<T as DisplayFile>::VTABLE,
> +            )
> +        };
> +
> +        #[cfg(not(CONFIG_DEBUG_FS))]
> +        let ptr = {
> +            // Mark parameters used
> +            let (_, _) = (name, data);

`let _ = (name, data);` should be sufficient.

> +            crate::error::code::ENODEV.to_ptr()
> +        };
> +
> +        // SAFETY: `debugfs_create_file_full` either returns an error code or a legal
> +        // dentry pointer, and without `CONFIG_DEBUGFS` we return an error pointer, so
> +        // `Entry::from_ptr` is safe to call here.
> +        let entry = unsafe { Entry::from_ptr(ptr) };
> +
> +        File(entry)
> +    }
> +
>      /// Create a new directory in DebugFS at the root.
>      ///
>      /// # Examples
> @@ -137,3 +202,70 @@ pub fn new(name: &CStr) -> Self {
>          Dir::create(name, None)
>      }
>  }
> +/// Handle to a DebugFS file.
> +#[repr(transparent)]
> +pub struct File<'a>(Entry<'a>);
> +
> +#[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>(

Why do these functions need to be pub?

---
Cheers,
Benno

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

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

[Greg Kroah-Hartman]

On Wed, May 14, 2025 at 09:33:05AM +0200, Benno Lossin wrote:
> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > +impl<'a> Dir<'a> {
> > +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    fn create<'b>(name: &CStr, parent: Option<&'a Dir<'b>>) -> Self {
> > +        let parent_ptr = match parent {
> > +            Some(parent) => parent.0.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.
> > +        //   so we can call `Self::from_ptr`.
> > +        let dir = unsafe { bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr) };
> > +
> > +        // SAFETY: `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > +        Self(unsafe { Entry::from_ptr(dir) })
> > +    }
> > +
> > +    #[cfg(not(CONFIG_DEBUG_FS))]
> > +    fn create<'b>(_name: &CStr, _parent: Option<&'a Dir<'b>>) -> Self {
> > +        Self(Entry::new())
> > +    }
> > +
> > +    /// Create a DebugFS subdirectory.
> 
> I'm not familiar with debugfs, if I run `Dir::create(c"foo", None)`
> twice, will both of the returned values refer to the same or different
> directories? 

You can not create a directory, or file, in the same location with the
same name.  The call will fail, so don't do that :)

> What if I give a parent?

Same thing, it will fail.

> If the answer in both cases is that they will refer to the same
> directory, then I'd change the docs to mention that.

Nope, that does not happen.

> So instead of
> "Creates" we could say "Finds or creates" or something better.

Find does not happen.

> If they refer to different files, then I am confused how that would look
> like in user-land :)

Agreed, which is why that does not happen :)

thanks,

greg k-h

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

[Danilo Krummrich]

On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > +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.
> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> 
> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> support of the wrapped type. But in this case, `Dir` is sometimes
> intended to not be dropped, so I'd rather have a `.keep()` function I
> saw mentioned somewhere.

I agree, if we really want to "officially" support to forget() (sub-)directories
and files in order to rely on the recursive cleanup of the "root" directory, it
should be covered explicitly by the API. I.e. (sub-)directories and files should
have some kind of keep() and / or forget() method, to make it clear that this is
supported and by design and won't lead to any leaks.

Consequently, this would mean that we'd need something like your proposed const
generic on the Dir type, such that keep() / forget() cannot be called on the
"root" directory.

However, I really think we should keep the code as it is in this version and
just don't provide an example that utilizes ManuallyDrop and forget().

I don't see how the idea of "manually dropping" (sub-)directories and files
provides any real value compared to just storing their instance in a driver
structure as long as they should stay alive, which is much more intuitive
anyways.

It either just adds complexity to the API (due to the need to distingish between
the "root" directory and sub-directories) or makes the API error prone by
providing a *valid looking* option to users to leak the "root" directory.

- Danilo

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

[Benno Lossin]

On Wed May 14, 2025 at 10:49 AM CEST, Greg Kroah-Hartman wrote:
> On Wed, May 14, 2025 at 09:33:05AM +0200, Benno Lossin wrote:
>> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
>> > +impl<'a> Dir<'a> {
>> > +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
>> > +    #[cfg(CONFIG_DEBUG_FS)]
>> > +    fn create<'b>(name: &CStr, parent: Option<&'a Dir<'b>>) -> Self {
>> > +        let parent_ptr = match parent {
>> > +            Some(parent) => parent.0.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.
>> > +        //   so we can call `Self::from_ptr`.
>> > +        let dir = unsafe { bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr) };
>> > +
>> > +        // SAFETY: `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
>> > +        Self(unsafe { Entry::from_ptr(dir) })
>> > +    }
>> > +
>> > +    #[cfg(not(CONFIG_DEBUG_FS))]
>> > +    fn create<'b>(_name: &CStr, _parent: Option<&'a Dir<'b>>) -> Self {
>> > +        Self(Entry::new())
>> > +    }
>> > +
>> > +    /// Create a DebugFS subdirectory.
>> 
>> I'm not familiar with debugfs, if I run `Dir::create(c"foo", None)`
>> twice, will both of the returned values refer to the same or different
>> directories? 
>
> You can not create a directory, or file, in the same location with the
> same name.  The call will fail, so don't do that :)
>
>> What if I give a parent?
>
> Same thing, it will fail.
>
>> If the answer in both cases is that they will refer to the same
>> directory, then I'd change the docs to mention that.
>
> Nope, that does not happen.
>
>> So instead of
>> "Creates" we could say "Finds or creates" or something better.
>
> Find does not happen.
>
>> If they refer to different files, then I am confused how that would look
>> like in user-land :)
>
> Agreed, which is why that does not happen :)

Ah that makes sense, thanks for explaining :)

---
Cheers,
Benno

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

[Benno Lossin]

On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
>> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
>> > +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.
>> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
>> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
>> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
>> 
>> I dislike the direct usage of `ManuallyDrop`. To me the usage of
>> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
>> support of the wrapped type. But in this case, `Dir` is sometimes
>> intended to not be dropped, so I'd rather have a `.keep()` function I
>> saw mentioned somewhere.
>
> I agree, if we really want to "officially" support to forget() (sub-)directories
> and files in order to rely on the recursive cleanup of the "root" directory, it
> should be covered explicitly by the API. I.e. (sub-)directories and files should
> have some kind of keep() and / or forget() method, to make it clear that this is
> supported and by design and won't lead to any leaks.
>
> Consequently, this would mean that we'd need something like your proposed const
> generic on the Dir type, such that keep() / forget() cannot be called on the
> "root" directory.
>
> However, I really think we should keep the code as it is in this version and
> just don't provide an example that utilizes ManuallyDrop and forget().
>
> I don't see how the idea of "manually dropping" (sub-)directories and files
> provides any real value compared to just storing their instance in a driver
> structure as long as they should stay alive, which is much more intuitive
> anyways.

Yeah that's whats normally done in Rust anyways. But I think that
lifetimes bite us in this case:

    let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));

    let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
    // lifetime `'a` starts in the line above and `sub` borrows `debugfs`

    /* code for creating the file etc */

    Ok(Self { _debugfs: debugfs, _sub: sub })
    // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!

This code won't compile, since we can't store the "root" dir in the same
struct that we want to store the subdir, because the subdir borrows from
the root dir.

Essentially this would require self-referential structs like the
`ouroboros` crate [1] from user-space Rust. We should rather have the
`.keep()` function in the API than use self-referential structs.

[1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html

Another problem that only affects complicated debugfs structures is that
you would have to store all subdirs & files somewhere. If the structure
is dynamic and changes over the lifetime of the driver, then you'll need
a `Vec` or store the dirs in `Arc` or similar, leading to extra
allocations.

> It either just adds complexity to the API (due to the need to distingish between
> the "root" directory and sub-directories) or makes the API error prone by
> providing a *valid looking* option to users to leak the "root" directory.

I agree with this, I want that `ManuallyDrop` & `forget` are only used
rarely mostly for low-level operations.

---
Cheers,
Benno

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

[Danilo Krummrich]

On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> >> > +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.
> >> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> >> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> >> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> >> 
> >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> >> support of the wrapped type. But in this case, `Dir` is sometimes
> >> intended to not be dropped, so I'd rather have a `.keep()` function I
> >> saw mentioned somewhere.
> >
> > I agree, if we really want to "officially" support to forget() (sub-)directories
> > and files in order to rely on the recursive cleanup of the "root" directory, it
> > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > have some kind of keep() and / or forget() method, to make it clear that this is
> > supported and by design and won't lead to any leaks.
> >
> > Consequently, this would mean that we'd need something like your proposed const
> > generic on the Dir type, such that keep() / forget() cannot be called on the
> > "root" directory.
> >
> > However, I really think we should keep the code as it is in this version and
> > just don't provide an example that utilizes ManuallyDrop and forget().
> >
> > I don't see how the idea of "manually dropping" (sub-)directories and files
> > provides any real value compared to just storing their instance in a driver
> > structure as long as they should stay alive, which is much more intuitive
> > anyways.
> 
> Yeah that's whats normally done in Rust anyways. But I think that
> lifetimes bite us in this case:
> 
>     let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
> 
>     let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
>     // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
> 
>     /* code for creating the file etc */
> 
>     Ok(Self { _debugfs: debugfs, _sub: sub })
>     // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
> 
> This code won't compile, since we can't store the "root" dir in the same
> struct that we want to store the subdir, because the subdir borrows from
> the root dir.
> 
> Essentially this would require self-referential structs like the
> `ouroboros` crate [1] from user-space Rust. We should rather have the
> `.keep()` function in the API than use self-referential structs.

Fair enough -- I think we should properly document those limitations, recommend
using keep() for those cases and ensure that we can't call keep() on the "root"
directory then.

Unless, we can find a better solution, which, unfortunately, I can't think of
one. The only thing I can think of is to reference count (parent) directories,
which would be contrary to how the C API works and not desirable.

> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
> 
> Another problem that only affects complicated debugfs structures is that
> you would have to store all subdirs & files somewhere. If the structure
> is dynamic and changes over the lifetime of the driver, then you'll need
> a `Vec` or store the dirs in `Arc` or similar, leading to extra
> allocations.

If it changes dynamically then it's pretty likely that we do not only want to
add entries dynamically, but also remove them, which implies that we need to be
able to drop them. So, I don't think that's a problem.

What I see more likely to happen is a situation where the "root" directory
(almost) lives forever, and hence subsequent calls, such as

	root.subdir("foo").keep()

effectively are leaks.

One specific example for that would be usb_debug_root, which is created in the
module scope of usb-common and is used by USB host / gadget / phy drivers.

The same is true for other cases where the debugfs "root" is created in the
module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively leak memory everytime a
device is unplugged (or unbound in general).

> 
> > It either just adds complexity to the API (due to the need to distingish between
> > the "root" directory and sub-directories) or makes the API error prone by
> > providing a *valid looking* option to users to leak the "root" directory.
> 
> I agree with this, I want that `ManuallyDrop` & `forget` are only used
> rarely mostly for low-level operations.
> 
> ---
> Cheers,
> Benno

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

[Benno Lossin]

On Wed May 14, 2025 at 1:24 PM CEST, Danilo Krummrich wrote:
> On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
>> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
>> >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
>> >> > +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.
>> >> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
>> >> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
>> >> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
>> >> 
>> >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
>> >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
>> >> support of the wrapped type. But in this case, `Dir` is sometimes
>> >> intended to not be dropped, so I'd rather have a `.keep()` function I
>> >> saw mentioned somewhere.
>> >
>> > I agree, if we really want to "officially" support to forget() (sub-)directories
>> > and files in order to rely on the recursive cleanup of the "root" directory, it
>> > should be covered explicitly by the API. I.e. (sub-)directories and files should
>> > have some kind of keep() and / or forget() method, to make it clear that this is
>> > supported and by design and won't lead to any leaks.
>> >
>> > Consequently, this would mean that we'd need something like your proposed const
>> > generic on the Dir type, such that keep() / forget() cannot be called on the
>> > "root" directory.
>> >
>> > However, I really think we should keep the code as it is in this version and
>> > just don't provide an example that utilizes ManuallyDrop and forget().
>> >
>> > I don't see how the idea of "manually dropping" (sub-)directories and files
>> > provides any real value compared to just storing their instance in a driver
>> > structure as long as they should stay alive, which is much more intuitive
>> > anyways.
>> 
>> Yeah that's whats normally done in Rust anyways. But I think that
>> lifetimes bite us in this case:
>> 
>>     let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
>> 
>>     let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
>>     // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
>> 
>>     /* code for creating the file etc */
>> 
>>     Ok(Self { _debugfs: debugfs, _sub: sub })
>>     // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
>> 
>> This code won't compile, since we can't store the "root" dir in the same
>> struct that we want to store the subdir, because the subdir borrows from
>> the root dir.
>> 
>> Essentially this would require self-referential structs like the
>> `ouroboros` crate [1] from user-space Rust. We should rather have the
>> `.keep()` function in the API than use self-referential structs.
>
> Fair enough -- I think we should properly document those limitations, recommend
> using keep() for those cases and ensure that we can't call keep() on the "root"
> directory then.
>
> Unless, we can find a better solution, which, unfortunately, I can't think of
> one. The only thing I can think of is to reference count (parent) directories,
> which would be contrary to how the C API works and not desirable.

Yeah, I also don't have an idea, but if I find something, I'll let you
know.

>> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
>> 
>> Another problem that only affects complicated debugfs structures is that
>> you would have to store all subdirs & files somewhere. If the structure
>> is dynamic and changes over the lifetime of the driver, then you'll need
>> a `Vec` or store the dirs in `Arc` or similar, leading to extra
>> allocations.
>
> If it changes dynamically then it's pretty likely that we do not only want to
> add entries dynamically, but also remove them, which implies that we need to be
> able to drop them. So, I don't think that's a problem.

Yeah that's true.

> What I see more likely to happen is a situation where the "root" directory
> (almost) lives forever, and hence subsequent calls, such as
>
> 	root.subdir("foo").keep()
>
> effectively are leaks.
>
> One specific example for that would be usb_debug_root, which is created in the
> module scope of usb-common and is used by USB host / gadget / phy drivers.
>
> The same is true for other cases where the debugfs "root" is created in the
> module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively leak memory everytime a
> device is unplugged (or unbound in general).

Hmm that is unfortunate. But I don't see a problem with having:

    static USB_DEBUGFS: Dir<'static> = ...; // or some on-demand init process

Then users can store subdir that also is `Dir<'static>` and just borrow
the USB_DEBUGFS for `'static`.

The docs on `keep` should definitely warn about leaks.

---
Cheers,
Benno

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

[Danilo Krummrich]

On Wed, May 14, 2025 at 02:21:41PM +0200, Benno Lossin wrote:
> The docs on `keep` should definitely warn about leaks.

Sounds like it could be a good candidate for `FORGET` comments as discussed in
[1]. :-)

[1] https://hackmd.io/@rust-for-linux-/HyJipr_3Jl

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

[Matthew Maurer]

On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > > +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.
> > > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> > > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> >
> > I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > support of the wrapped type. But in this case, `Dir` is sometimes
> > intended to not be dropped, so I'd rather have a `.keep()` function I
> > saw mentioned somewhere.
>
> I agree, if we really want to "officially" support to forget() (sub-)directories
> and files in order to rely on the recursive cleanup of the "root" directory, it
> should be covered explicitly by the API. I.e. (sub-)directories and files should
> have some kind of keep() and / or forget() method, to make it clear that this is
> supported and by design and won't lead to any leaks.

OK, I can do this with `.keep()` just being equivalent to
`ManuallyDrop::new`. Since we now only have "by default, things are
dropped", rather than two states, we shouldn't need a `.destroy()`
method.

>
> Consequently, this would mean that we'd need something like your proposed const
> generic on the Dir type, such that keep() / forget() cannot be called on the
> "root" directory.

Just to ensure I understand correctly, your opinion is that on
balance, additional complexity in the API types are worth making it
less ergonomic to suppress drop on a root directory, keeping in mind
that they still *can* suppress drop on that directory. You don't want
the extra API variants to do anything else special other than change
the ergonomics of `.keep()` (e.g. don't have subdirectories be default
pre-`.keep()`'d).

>
> However, I really think we should keep the code as it is in this version and
> just don't provide an example that utilizes ManuallyDrop and forget().
>
> I don't see how the idea of "manually dropping" (sub-)directories and files
> provides any real value compared to just storing their instance in a driver
> structure as long as they should stay alive, which is much more intuitive
> anyways.

We can't easily do this, because dropping a root directory recursively
drops everything underneath it. This means that if I have

foo/
  - bar/
  - baz/

Then my directory handle for `bar` have to be guaranteed to outlive my
directory handle for `foo` so that I know it's didn't get deleted
under me. This is why they have a borrow onto their parent directory.
This borrow means that you can't (without `unsafe`, or something like
`yoke`) keep handles to `foo` and `bar` in the same struct.

I could arguably make the subdir handle creator *take ownership* of
the root handle, which would mitigate this issue, but if we want to
allow creation of arbitrarily shaped trees, we'd either end up rapidly
reproducing something like my builder interface from the follow-up
series which I understand folks aren't enthused about, or we'd end up
with Rust having a mirror struct of some kind, e.g.

```
struct Tree {
  name: CString,
  // Order is load bearing, any children must be dropped before their parent
  children: KVec<Tree>
  root: Dir,
}

impl Tree {
  fn subdir(&mut self, name: &CStr) -> &mut Tree {
    if let Some(x) = children.iter_mut().find(|tree| tree.name == name) {
      x
    } else {
      // This is an internal function, similar to Dir::create
      // SAFETY: The structure promises all children will be released
before the parent directory is destroyed.
      self.children.push(unsafe { Tree::sys_subdir(self.dir, name) })
      self.children.last_mut().unwrap()
    }
}
```

This structure has the downside that we now need to track the file
structure in Rust too, not just in the main kernel. This gets more
complex when we add Files, but follows the same approximate structure.

If we don't track the filenames, you have no way to reference
subdirectories again later. If we do track them, we're reproducing
dentry structure less efficiently in Rust. Additionally, because this
needs to track filenames, this now means that `CONFIG_DEBUG_FS=n` is
no longer free.

>
> It either just adds complexity to the API (due to the need to distingish between
> the "root" directory and sub-directories) or makes the API error prone by
> providing a *valid looking* option to users to leak the "root" directory.
>
> - Danilo

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

[Matthew Maurer]

On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote:
>
> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> >> > +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.
> >> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> >> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> >> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> >>
> >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> >> support of the wrapped type. But in this case, `Dir` is sometimes
> >> intended to not be dropped, so I'd rather have a `.keep()` function I
> >> saw mentioned somewhere.
> >
> > I agree, if we really want to "officially" support to forget() (sub-)directories
> > and files in order to rely on the recursive cleanup of the "root" directory, it
> > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > have some kind of keep() and / or forget() method, to make it clear that this is
> > supported and by design and won't lead to any leaks.
> >
> > Consequently, this would mean that we'd need something like your proposed const
> > generic on the Dir type, such that keep() / forget() cannot be called on the
> > "root" directory.
> >
> > However, I really think we should keep the code as it is in this version and
> > just don't provide an example that utilizes ManuallyDrop and forget().
> >
> > I don't see how the idea of "manually dropping" (sub-)directories and files
> > provides any real value compared to just storing their instance in a driver
> > structure as long as they should stay alive, which is much more intuitive
> > anyways.
>
> Yeah that's whats normally done in Rust anyways. But I think that
> lifetimes bite us in this case:
>
>     let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
>
>     let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
>     // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
>
>     /* code for creating the file etc */
>
>     Ok(Self { _debugfs: debugfs, _sub: sub })
>     // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
>
> This code won't compile, since we can't store the "root" dir in the same
> struct that we want to store the subdir, because the subdir borrows from
> the root dir.
>
> Essentially this would require self-referential structs like the
> `ouroboros` crate [1] from user-space Rust. We should rather have the
> `.keep()` function in the API than use self-referential structs.
>
> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html

Yes, this is also an issue when it comes to attaching data to debugfs
if you look at the WIP patchset [1]. If we do end up deciding to try
to do self-referential structs for users (I'd rather not, but I
understand I don't have the deciding vote here), I'd strongly suggest
considering self_cell [2] as a model instead of ouroboros - there are
structural UB issues [3] with ouroboros that are unlikely to be
resolved, and the author of ouroboros encourages use of self_cell
instead [4]

[1]: https://lore.kernel.org/all/20250506-debugfs-rust-attach-v2-0-c6f88be3890a@google.com/
[2]: https://docs.rs/self_cell/latest/self_cell/
[3]: https://github.com/rust-lang/miri/issues/2844#issuecomment-1510577943
[4]: https://github.com/someguynamedjosh/ouroboros/issues/88

>
> Another problem that only affects complicated debugfs structures is that
> you would have to store all subdirs & files somewhere. If the structure
> is dynamic and changes over the lifetime of the driver, then you'll need
> a `Vec` or store the dirs in `Arc` or similar, leading to extra
> allocations.

Yep, this is part of why I tried to follow the "Build it, then hold
the needed handles to keep it alive" approach rather than keeping the
entire structure around.

>
> > It either just adds complexity to the API (due to the need to distingish between
> > the "root" directory and sub-directories) or makes the API error prone by
> > providing a *valid looking* option to users to leak the "root" directory.
>
> I agree with this, I want that `ManuallyDrop` & `forget` are only used
> rarely mostly for low-level operations.

Sure, I'll re-instate `Dir::keep` and `File::keep` as aliases for
`ManuallyDrop::new` in the next version. TBD whether we add the bit
that Danilo suggested to make `Dir::keep` not exist for root-level
directories.

>
> ---
> Cheers,
> Benno

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

[Matthew Maurer]

On Wed, May 14, 2025 at 4:24 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
> > On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> > > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > >> > +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.
> > >> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > >> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> > >> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> > >>
> > >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > >> support of the wrapped type. But in this case, `Dir` is sometimes
> > >> intended to not be dropped, so I'd rather have a `.keep()` function I
> > >> saw mentioned somewhere.
> > >
> > > I agree, if we really want to "officially" support to forget() (sub-)directories
> > > and files in order to rely on the recursive cleanup of the "root" directory, it
> > > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > > have some kind of keep() and / or forget() method, to make it clear that this is
> > > supported and by design and won't lead to any leaks.
> > >
> > > Consequently, this would mean that we'd need something like your proposed const
> > > generic on the Dir type, such that keep() / forget() cannot be called on the
> > > "root" directory.
> > >
> > > However, I really think we should keep the code as it is in this version and
> > > just don't provide an example that utilizes ManuallyDrop and forget().
> > >
> > > I don't see how the idea of "manually dropping" (sub-)directories and files
> > > provides any real value compared to just storing their instance in a driver
> > > structure as long as they should stay alive, which is much more intuitive
> > > anyways.
> >
> > Yeah that's whats normally done in Rust anyways. But I think that
> > lifetimes bite us in this case:
> >
> >     let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
> >
> >     let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
> >     // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
> >
> >     /* code for creating the file etc */
> >
> >     Ok(Self { _debugfs: debugfs, _sub: sub })
> >     // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
> >
> > This code won't compile, since we can't store the "root" dir in the same
> > struct that we want to store the subdir, because the subdir borrows from
> > the root dir.
> >
> > Essentially this would require self-referential structs like the
> > `ouroboros` crate [1] from user-space Rust. We should rather have the
> > `.keep()` function in the API than use self-referential structs.
>
> Fair enough -- I think we should properly document those limitations, recommend
> using keep() for those cases and ensure that we can't call keep() on the "root"
> directory then.
>
> Unless, we can find a better solution, which, unfortunately, I can't think of
> one. The only thing I can think of is to reference count (parent) directories,
> which would be contrary to how the C API works and not desirable.
>
> > [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
> >
> > Another problem that only affects complicated debugfs structures is that
> > you would have to store all subdirs & files somewhere. If the structure
> > is dynamic and changes over the lifetime of the driver, then you'll need
> > a `Vec` or store the dirs in `Arc` or similar, leading to extra
> > allocations.
>
> If it changes dynamically then it's pretty likely that we do not only want to
> add entries dynamically, but also remove them, which implies that we need to be
> able to drop them. So, I don't think that's a problem.
>
> What I see more likely to happen is a situation where the "root" directory
> (almost) lives forever, and hence subsequent calls, such as
>
>         root.subdir("foo").keep()
>
> effectively are leaks.
>
> One specific example for that would be usb_debug_root, which is created in the
> module scope of usb-common and is used by USB host / gadget / phy drivers.
>
> The same is true for other cases where the debugfs "root" is created in the
> module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively leak memory everytime a
> device is unplugged (or unbound in general).

Yes, this is one of the things I don't currently have a good safe
solution for without introducing something with similar power to
`self_cell` or a bespoke type implemented unsafely like the `Tree` I
mentioned earlier in the chain.

>
> >
> > > It either just adds complexity to the API (due to the need to distingish between
> > > the "root" directory and sub-directories) or makes the API error prone by
> > > providing a *valid looking* option to users to leak the "root" directory.
> >
> > I agree with this, I want that `ManuallyDrop` & `forget` are only used
> > rarely mostly for low-level operations.
> >
> > ---
> > Cheers,
> > Benno

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

[Danilo Krummrich]

On Wed, May 14, 2025 at 03:08:21PM -0700, Matthew Maurer wrote:
> On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote:
> > Another problem that only affects complicated debugfs structures is that
> > you would have to store all subdirs & files somewhere. If the structure
> > is dynamic and changes over the lifetime of the driver, then you'll need
> > a `Vec` or store the dirs in `Arc` or similar, leading to extra
> > allocations.
> 
> Yep, this is part of why I tried to follow the "Build it, then hold
> the needed handles to keep it alive" approach rather than keeping the
> entire structure around.

I already replied to that [1]:

"If it changes dynamically then it's pretty likely that we do not only want to
add entries dynamically, but also remove them, which implies that we need to be
able to drop them. So, I don't think that's a problem."

It is much more of a problem if we can't remove certain entries anymore without
removing all of them.

[1] https://lore.kernel.org/rust-for-linux/aCR9cD7OcSefeaUm@pollux/

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

[Danilo Krummrich]

On Wed, May 14, 2025 at 02:55:02PM -0700, Matthew Maurer wrote:
> On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > > On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > > > +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.
> > > > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > > > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> > > > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> > >
> > > I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > > `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > > support of the wrapped type. But in this case, `Dir` is sometimes
> > > intended to not be dropped, so I'd rather have a `.keep()` function I
> > > saw mentioned somewhere.
> >
> > I agree, if we really want to "officially" support to forget() (sub-)directories
> > and files in order to rely on the recursive cleanup of the "root" directory, it
> > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > have some kind of keep() and / or forget() method, to make it clear that this is
> > supported and by design and won't lead to any leaks.
> 
> OK, I can do this with `.keep()` just being equivalent to
> `ManuallyDrop::new`. Since we now only have "by default, things are
> dropped", rather than two states, we shouldn't need a `.destroy()`
> method.
> 
> >
> > Consequently, this would mean that we'd need something like your proposed const
> > generic on the Dir type, such that keep() / forget() cannot be called on the
> > "root" directory.
> 
> Just to ensure I understand correctly, your opinion is that on
> balance, additional complexity in the API types are worth making it
> less ergonomic to suppress drop on a root directory, keeping in mind
> that they still *can* suppress drop on that directory. You don't want
> the extra API variants to do anything else special other than change
> the ergonomics of `.keep()` (e.g. don't have subdirectories be default
> pre-`.keep()`'d).

Absolutely, yes! Having the keep() method available for "root" directories is
clearly a footgun, because it suggests that it is a valid thing to call for a
"root" directory, but actually is almost always a bug.

What do we have a powerful type system for if we don't take advantage of it to
prevent bugs? :-)

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

[Matthew Maurer]

On Wed, May 14, 2025 at 3:14 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, May 14, 2025 at 03:08:21PM -0700, Matthew Maurer wrote:
> > On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote:
> > > Another problem that only affects complicated debugfs structures is that
> > > you would have to store all subdirs & files somewhere. If the structure
> > > is dynamic and changes over the lifetime of the driver, then you'll need
> > > a `Vec` or store the dirs in `Arc` or similar, leading to extra
> > > allocations.
> >
> > Yep, this is part of why I tried to follow the "Build it, then hold
> > the needed handles to keep it alive" approach rather than keeping the
> > entire structure around.
>
> I already replied to that [1]:
>
> "If it changes dynamically then it's pretty likely that we do not only want to
> add entries dynamically, but also remove them, which implies that we need to be
> able to drop them. So, I don't think that's a problem."
>
> It is much more of a problem if we can't remove certain entries anymore without
> removing all of them.
>
> [1] https://lore.kernel.org/rust-for-linux/aCR9cD7OcSefeaUm@pollux/

I think the main question here is this - is it OK to land an API that
does static-layout or add-only-layout first, and we figure out how to
do dynamic modification later, or do you think we need to solve the
self-referential lifetimes issue in the first edition of DebugFS
support?

If you do think we need support for dynamic layout modification in the
first patchset, do you think we want to allow static layouts that
forget things, or mandate that we always manage all of the handles for
the user?

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

[Matthew Maurer]

On Wed, May 7, 2025 at 11:46 AM Timur Tabi <ttabi@nvidia.com> wrote:
>
> On Mon, 2025-05-05 at 23:51 +0000, Matthew Maurer wrote:
> >
> > +impl<'a> Entry<'a> {
> > +    /// Constructs a new DebugFS [`Entry`] 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. If this is a child directory or file, `'a` must be less than the
> > +    /// lifetime of the parent directory.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
> > +        Self {
> > +            entry,
> > +            _phantom: PhantomData,
> > +        }
> > +    }
> > +
> > +    #[cfg(not(CONFIG_DEBUG_FS))]
> > +    fn new() -> Self {
> > +        Self {
> > +            _phantom: PhantomData,
> > +        }
> > +    }
>
> I am new to Rust, so forgive me if this is a dumb question, but it looks to me that if
> CONFIG_DEBUG_FS is defined, then you need to call from_ptr() to create a new Entry, but if
> CONFIG_DEBUG_FS is not defined, then you need to call new() instead.  Is that right?  If so, is that
> really idiomatic?

I could make `from_ptr` take an arbitrary pointer and discard it as
well, but the callsite for `from_ptr` involves calling into the C
bindings to get a pointer back. I can do one of the following:
1. Create a stub function for the CONFIG_DEBUG_FS=n variant of those
functions (since those are in header files, so they need a special
helper) which gets compiled in, and just returns ERR_PTR(ENODEV), call
that, and pass it back in. (This leads to code bloat, though not
much.)
2. Manually call `ptr::dangling()` and pass it to the alt `from_ptr`
that ignores its argument
3. Create and call `::new`.

If I had more call-sites where I had a pointer-like object to put in
there, I'd use a `from_ptr` that discards. I used `::new` just because
it was easier.

>
> In the Dir implementation below, you are careful to call from_ptr() only from the CONFIG_DEBUG_FS
> version of create(), and you call new() only from the !CONFIG_DEBUG_FS version of create().  So your
> bases are covered as long as no driver tries to create an Entry from scratch.
>
> But I guess that can't happen because Entry is not public, right?

Correct, `Entry` is a private type.

>
> > +    /// Create a DebugFS subdirectory.
> > +    ///
> > +    /// Subdirectory handles cannot outlive the directory handle they were created from.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// let parent = Dir::new(c_str!("parent"));
> > +    /// let child = parent.subdir(c_str!("child"));
> > +    /// ```
> > +    pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
> > +        Dir::create(name, Some(self))
> > +    }
> > +
> > +    /// Create a new directory in DebugFS at the root.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// let debugfs = Dir::new(c_str!("parent"));
> > +    /// ```
> > +    pub fn new(name: &CStr) -> Self {
> > +        Dir::create(name, None)
> > +    }
>
> Is there any real value to having two constructors, just to avoid passing None for the one time that
> a root directory will be created?  The C code has no problem passing NULL.

Past revisions (and some of Danilo's suggestions on this revision)
required the ability to return different types when a directory was
not a subdir. In earlier versions, because subdirectories were not
automatically cleaned on drop unless opted in, where the root
directory was. In future versions, he would like me to use this to
suppress `Dir::keep` from being callable on root directories.

>
>

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

[Matthew Maurer]

On Wed, May 14, 2025 at 3:23 PM Matthew Maurer <mmaurer@google.com> wrote:
>
> On Wed, May 14, 2025 at 3:14 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, May 14, 2025 at 03:08:21PM -0700, Matthew Maurer wrote:
> > > On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote:
> > > > Another problem that only affects complicated debugfs structures is that
> > > > you would have to store all subdirs & files somewhere. If the structure
> > > > is dynamic and changes over the lifetime of the driver, then you'll need
> > > > a `Vec` or store the dirs in `Arc` or similar, leading to extra
> > > > allocations.
> > >
> > > Yep, this is part of why I tried to follow the "Build it, then hold
> > > the needed handles to keep it alive" approach rather than keeping the
> > > entire structure around.
> >
> > I already replied to that [1]:
> >
> > "If it changes dynamically then it's pretty likely that we do not only want to
> > add entries dynamically, but also remove them, which implies that we need to be
> > able to drop them. So, I don't think that's a problem."
> >
> > It is much more of a problem if we can't remove certain entries anymore without
> > removing all of them.
> >
> > [1] https://lore.kernel.org/rust-for-linux/aCR9cD7OcSefeaUm@pollux/
>
> I think the main question here is this - is it OK to land an API that
> does static-layout or add-only-layout first, and we figure out how to
> do dynamic modification later, or do you think we need to solve the
> self-referential lifetimes issue in the first edition of DebugFS
> support?
>
> If you do think we need support for dynamic layout modification in the
> first patchset, do you think we want to allow static layouts that
> forget things, or mandate that we always manage all of the handles for
> the user?

One further possibility here, which we'd need Greg to weigh in on - we
could add a method to the debugfs API intended for Rust usage which
specifically releases a directory or file *without* releasing any
nested elements. This would mean we could get rid of all the lifetimes
on directory and file handles. This variant would mean that if the
user did something wrong (e.g. released a root directory before taking
an action on a child or releasing it), they wouldn't get undefined
behavior, they'd just get no effect. If they wrote this kind of buggy
behavior, they might be *surprised* at the file being absent, but no
UB would happen. If we wanted a variant that used the current
recursive remove, it would still need the lifetimes, with all the
difficulties that entails.

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

[Timur Tabi]

On Wed, 2025-05-14 at 15:32 -0700, Matthew Maurer wrote:
> One further possibility here, which we'd need Greg to weigh in on - we
> could add a method to the debugfs API intended for Rust usage which
> specifically releases a directory or file *without* releasing any
> nested elements. This would mean we could get rid of all the lifetimes
> on directory and file handles. 

I had a conversation with Greg about this topic just the other week.

https://lore.kernel.org/linux-doc/20250429173958.3973958-1-ttabi@nvidia.com/

There are two versions of debugfs_remove:

void debugfs_remove(struct dentry *dentry);
#define debugfs_remove_recursive debugfs_remove

Unfortunately, the direction that we've been going is to get rid of debugfs_remove_recursive() and
have drivers only call debugfs_remove().  

What would solve your problem is doing the opposite: making debugfs_remove() be non-recursive and
require drivers to call debugfs_remove_recursive() if that's what they really want.

Maybe we need debugfs_remove_single()?

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

[Matthew Maurer]

On Wed, May 14, 2025 at 3:40 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> On Wed, 2025-05-14 at 15:32 -0700, Matthew Maurer wrote:
> > One further possibility here, which we'd need Greg to weigh in on - we
> > could add a method to the debugfs API intended for Rust usage which
> > specifically releases a directory or file *without* releasing any
> > nested elements. This would mean we could get rid of all the lifetimes
> > on directory and file handles.
>
> I had a conversation with Greg about this topic just the other week.
>
> https://lore.kernel.org/linux-doc/20250429173958.3973958-1-ttabi@nvidia.com/
>
> There are two versions of debugfs_remove:
>
> void debugfs_remove(struct dentry *dentry);
> #define debugfs_remove_recursive debugfs_remove
>
> Unfortunately, the direction that we've been going is to get rid of debugfs_remove_recursive() and
> have drivers only call debugfs_remove().
>
> What would solve your problem is doing the opposite: making debugfs_remove() be non-recursive and
> require drivers to call debugfs_remove_recursive() if that's what they really want.
>
> Maybe we need debugfs_remove_single()?
>

Yes, having access to `debugfs_remove_single()`, if it has the
properties I would expect (namely that the kernel objects for
inaccessible directories continue to exist, they're just not reachable
through the VFS) would allow this design. It's not obvious to me if
it's the design we want, but it would enable that design.

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

[gregkh]

On Wed, May 14, 2025 at 03:42:46PM -0700, Matthew Maurer wrote:
> On Wed, May 14, 2025 at 3:40 PM Timur Tabi <ttabi@nvidia.com> wrote:
> >
> > On Wed, 2025-05-14 at 15:32 -0700, Matthew Maurer wrote:
> > > One further possibility here, which we'd need Greg to weigh in on - we
> > > could add a method to the debugfs API intended for Rust usage which
> > > specifically releases a directory or file *without* releasing any
> > > nested elements. This would mean we could get rid of all the lifetimes
> > > on directory and file handles.
> >
> > I had a conversation with Greg about this topic just the other week.
> >
> > https://lore.kernel.org/linux-doc/20250429173958.3973958-1-ttabi@nvidia.com/
> >
> > There are two versions of debugfs_remove:
> >
> > void debugfs_remove(struct dentry *dentry);
> > #define debugfs_remove_recursive debugfs_remove
> >
> > Unfortunately, the direction that we've been going is to get rid of debugfs_remove_recursive() and
> > have drivers only call debugfs_remove().
> >
> > What would solve your problem is doing the opposite: making debugfs_remove() be non-recursive and
> > require drivers to call debugfs_remove_recursive() if that's what they really want.
> >
> > Maybe we need debugfs_remove_single()?
> >
> 
> Yes, having access to `debugfs_remove_single()`, if it has the
> properties I would expect (namely that the kernel objects for
> inaccessible directories continue to exist, they're just not reachable
> through the VFS) would allow this design. It's not obvious to me if
> it's the design we want, but it would enable that design.

Ick, no, we got rid of debugfs_remove() only removing a single dentry a
while ago as it was causing problems.  Let's not go back to that at all.

This shouldn't be all that complex, it's supposed to be simple to use
and reason about.  If you create a directory, you can "own" it until you
drop the reference and then it, and all files/subdirs will then be
removed.  I like that logic much better than what we are stuck with in C
where we can create a directory but then be forced to remember to look
it up later to remove it when finished.

Ideally we can migrate the C code to be more like the rust api in the
future as that should be easier to deal with over time by driver
authors.

thanks,

greg k-h

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

[Benno Lossin]

On Thu May 15, 2025 at 12:08 AM CEST, Matthew Maurer wrote:
> Yes, this is also an issue when it comes to attaching data to debugfs
> if you look at the WIP patchset [1]. If we do end up deciding to try
> to do self-referential structs for users (I'd rather not, but I
> understand I don't have the deciding vote here), I'd strongly suggest
> considering self_cell [2] as a model instead of ouroboros - there are
> structural UB issues [3] with ouroboros that are unlikely to be
> resolved, and the author of ouroboros encourages use of self_cell
> instead [4]

I do not think we should add something that allows us to have
self-referential structs. I only mentioned ouroboros, because in theory
it's possible to do in Rust, but we shouldn't do it.

---
Cheers,
Benno

> [1]: https://lore.kernel.org/all/20250506-debugfs-rust-attach-v2-0-c6f88be3890a@google.com/
> [2]: https://docs.rs/self_cell/latest/self_cell/
> [3]: https://github.com/rust-lang/miri/issues/2844#issuecomment-1510577943
> [4]: https://github.com/someguynamedjosh/ouroboros/issues/88

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

[Benno Lossin]

On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> However, I really think we should keep the code as it is in this version and
>> just don't provide an example that utilizes ManuallyDrop and forget().
>>
>> I don't see how the idea of "manually dropping" (sub-)directories and files
>> provides any real value compared to just storing their instance in a driver
>> structure as long as they should stay alive, which is much more intuitive
>> anyways.
>
> We can't easily do this, because dropping a root directory recursively
> drops everything underneath it. This means that if I have
>
> foo/
>   - bar/
>   - baz/
>
> Then my directory handle for `bar` have to be guaranteed to outlive my
> directory handle for `foo` so that I know it's didn't get deleted
> under me. This is why they have a borrow onto their parent directory.
> This borrow means that you can't (without `unsafe`, or something like
> `yoke`) keep handles to `foo` and `bar` in the same struct.

Is there no refcount that we can use instead of borrowing? I guess not,
since one can call `debugfs_remove`. What about a refcount on the rust
side? or is debugfs not used for "debugging" and needs to have the
performance of no refcount?

---
Cheers,
Benno

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

[Greg Kroah-Hartman]

On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> However, I really think we should keep the code as it is in this version and
> >> just don't provide an example that utilizes ManuallyDrop and forget().
> >>
> >> I don't see how the idea of "manually dropping" (sub-)directories and files
> >> provides any real value compared to just storing their instance in a driver
> >> structure as long as they should stay alive, which is much more intuitive
> >> anyways.
> >
> > We can't easily do this, because dropping a root directory recursively
> > drops everything underneath it. This means that if I have
> >
> > foo/
> >   - bar/
> >   - baz/
> >
> > Then my directory handle for `bar` have to be guaranteed to outlive my
> > directory handle for `foo` so that I know it's didn't get deleted
> > under me. This is why they have a borrow onto their parent directory.
> > This borrow means that you can't (without `unsafe`, or something like
> > `yoke`) keep handles to `foo` and `bar` in the same struct.
> 
> Is there no refcount that we can use instead of borrowing? I guess not,
> since one can call `debugfs_remove`. What about a refcount on the rust
> side? or is debugfs not used for "debugging" and needs to have the
> performance of no refcount?

debugfs should never have any performance issues (i.e. you don't use it
for performant things.)

So refcount away!  That should never be an issue.

thanks,

greg k-h

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

[Danilo Krummrich]

On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >> However, I really think we should keep the code as it is in this version and
> > >> just don't provide an example that utilizes ManuallyDrop and forget().
> > >>
> > >> I don't see how the idea of "manually dropping" (sub-)directories and files
> > >> provides any real value compared to just storing their instance in a driver
> > >> structure as long as they should stay alive, which is much more intuitive
> > >> anyways.
> > >
> > > We can't easily do this, because dropping a root directory recursively
> > > drops everything underneath it. This means that if I have
> > >
> > > foo/
> > >   - bar/
> > >   - baz/
> > >
> > > Then my directory handle for `bar` have to be guaranteed to outlive my
> > > directory handle for `foo` so that I know it's didn't get deleted
> > > under me. This is why they have a borrow onto their parent directory.
> > > This borrow means that you can't (without `unsafe`, or something like
> > > `yoke`) keep handles to `foo` and `bar` in the same struct.
> > 
> > Is there no refcount that we can use instead of borrowing? I guess not,
> > since one can call `debugfs_remove`. What about a refcount on the rust
> > side? or is debugfs not used for "debugging" and needs to have the
> > performance of no refcount?
> 
> debugfs should never have any performance issues (i.e. you don't use it
> for performant things.)
> 
> So refcount away!  That should never be an issue.

Reference counting (parent) directories should lead to a much cleaner solution.

I mentioned that previously, but also said in that context that it's a bit
contrary to how the C API is utilized currently, which usually isn't desired.

However, if we're fine with that I think it's superior to the borrowing
solution, which requires keep(). IMHO keep() is a footgun in general, even if
not callable for "root" directories.

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

[Benno Lossin]

On Thu May 15, 2025 at 2:37 PM CEST, Danilo Krummrich wrote:
> On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
>> > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
>> > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> > >> However, I really think we should keep the code as it is in this version and
>> > >> just don't provide an example that utilizes ManuallyDrop and forget().
>> > >>
>> > >> I don't see how the idea of "manually dropping" (sub-)directories and files
>> > >> provides any real value compared to just storing their instance in a driver
>> > >> structure as long as they should stay alive, which is much more intuitive
>> > >> anyways.
>> > >
>> > > We can't easily do this, because dropping a root directory recursively
>> > > drops everything underneath it. This means that if I have
>> > >
>> > > foo/
>> > >   - bar/
>> > >   - baz/
>> > >
>> > > Then my directory handle for `bar` have to be guaranteed to outlive my
>> > > directory handle for `foo` so that I know it's didn't get deleted
>> > > under me. This is why they have a borrow onto their parent directory.
>> > > This borrow means that you can't (without `unsafe`, or something like
>> > > `yoke`) keep handles to `foo` and `bar` in the same struct.
>> > 
>> > Is there no refcount that we can use instead of borrowing? I guess not,
>> > since one can call `debugfs_remove`. What about a refcount on the rust
>> > side? or is debugfs not used for "debugging" and needs to have the
>> > performance of no refcount?
>> 
>> debugfs should never have any performance issues (i.e. you don't use it
>> for performant things.)
>> 
>> So refcount away!  That should never be an issue.
>
> Reference counting (parent) directories should lead to a much cleaner solution.
>
> I mentioned that previously, but also said in that context that it's a bit
> contrary to how the C API is utilized currently, which usually isn't desired.

We could also change the C side to use refcounting :) It is probably a
bigger change (I have no idea how common the usage of debugfs is).

In my mind, it would also allow the C side to benefit from the same
"drop the dirs that you don't need anymore and all subdirs will be
removed if they also aren't referenced any longer" thing.

> However, if we're fine with that I think it's superior to the borrowing
> solution, which requires keep(). IMHO keep() is a footgun in general, even if
> not callable for "root" directories.

I would prefer refcounting over forgetting, it much more clearly shows
who owns the debugfs dirs. Also, in which cases would one not call
`.keep()`? The USB example from the other thread comes to mind, but
there you might be able to borrow a `Dir<'static` for `'static`, are
there other cases?

---
Cheers,
Benno

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

[Alice Ryhl]

On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >> However, I really think we should keep the code as it is in this version and
> > >> just don't provide an example that utilizes ManuallyDrop and forget().
> > >>
> > >> I don't see how the idea of "manually dropping" (sub-)directories and files
> > >> provides any real value compared to just storing their instance in a driver
> > >> structure as long as they should stay alive, which is much more intuitive
> > >> anyways.
> > >
> > > We can't easily do this, because dropping a root directory recursively
> > > drops everything underneath it. This means that if I have
> > >
> > > foo/
> > >   - bar/
> > >   - baz/
> > >
> > > Then my directory handle for `bar` have to be guaranteed to outlive my
> > > directory handle for `foo` so that I know it's didn't get deleted
> > > under me. This is why they have a borrow onto their parent directory.
> > > This borrow means that you can't (without `unsafe`, or something like
> > > `yoke`) keep handles to `foo` and `bar` in the same struct.
> > 
> > Is there no refcount that we can use instead of borrowing? I guess not,
> > since one can call `debugfs_remove`. What about a refcount on the rust
> > side? or is debugfs not used for "debugging" and needs to have the
> > performance of no refcount?
> 
> debugfs should never have any performance issues (i.e. you don't use it
> for performant things.)
> 
> So refcount away!  That should never be an issue.

What I imagine would be the ideal API for Rust is the following:

* For each file / dir you create, you get a Rust object that owns it.

* When you destroy one of these Rust objects, it disappears from the
  file system. I.e., dropping a directory removes things recursively.

* If you remove a directory before the removing objects inside it, then
  the Rust objects become "ghost" objects that are still usable, but not
  visible in the file system anywhere. I.e. calling methods on them
  succeed but are no-ops.

* Possibly we have a way to drop a Rust object without removing it from
  the file system. In this case, it can never be accessed from Rust
  again, and the only way to remove it is to drop its parent directory.

This way, you can drop foo/ before dropping bar/ and baz/ without that
having to be unsafe.

Whether that's best implemented by calling dget/dput on the dentry or by
having Rust keep track of a separate Rust-only refcount, I don't know.
But I think this is the API we want.

Thoughts?

Alice

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

[Greg Kroah-Hartman]

On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >> However, I really think we should keep the code as it is in this version and
> > > >> just don't provide an example that utilizes ManuallyDrop and forget().
> > > >>
> > > >> I don't see how the idea of "manually dropping" (sub-)directories and files
> > > >> provides any real value compared to just storing their instance in a driver
> > > >> structure as long as they should stay alive, which is much more intuitive
> > > >> anyways.
> > > >
> > > > We can't easily do this, because dropping a root directory recursively
> > > > drops everything underneath it. This means that if I have
> > > >
> > > > foo/
> > > >   - bar/
> > > >   - baz/
> > > >
> > > > Then my directory handle for `bar` have to be guaranteed to outlive my
> > > > directory handle for `foo` so that I know it's didn't get deleted
> > > > under me. This is why they have a borrow onto their parent directory.
> > > > This borrow means that you can't (without `unsafe`, or something like
> > > > `yoke`) keep handles to `foo` and `bar` in the same struct.
> > > 
> > > Is there no refcount that we can use instead of borrowing? I guess not,
> > > since one can call `debugfs_remove`. What about a refcount on the rust
> > > side? or is debugfs not used for "debugging" and needs to have the
> > > performance of no refcount?
> > 
> > debugfs should never have any performance issues (i.e. you don't use it
> > for performant things.)
> > 
> > So refcount away!  That should never be an issue.
> 
> What I imagine would be the ideal API for Rust is the following:
> 
> * For each file / dir you create, you get a Rust object that owns it.
> 
> * When you destroy one of these Rust objects, it disappears from the
>   file system. I.e., dropping a directory removes things recursively.
> 
> * If you remove a directory before the removing objects inside it, then
>   the Rust objects become "ghost" objects that are still usable, but not
>   visible in the file system anywhere. I.e. calling methods on them
>   succeed but are no-ops.

Why not just also remove those objects at the same time?  That would be
more like what the filesystem logic itself does today.

> * Possibly we have a way to drop a Rust object without removing it from
>   the file system. In this case, it can never be accessed from Rust
>   again, and the only way to remove it is to drop its parent directory.

This too would be nice as that's how the existing api works in C.

> This way, you can drop foo/ before dropping bar/ and baz/ without that
> having to be unsafe.
> 
> Whether that's best implemented by calling dget/dput on the dentry or by
> having Rust keep track of a separate Rust-only refcount, I don't know.
> But I think this is the API we want.
> 
> Thoughts?

Sounds reasonable to me and should be easy to use, which is the key
feature/requirement of debugfs.

thanks,

greg k-h

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

[Danilo Krummrich]

On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >> However, I really think we should keep the code as it is in this version and
> > > >> just don't provide an example that utilizes ManuallyDrop and forget().
> > > >>
> > > >> I don't see how the idea of "manually dropping" (sub-)directories and files
> > > >> provides any real value compared to just storing their instance in a driver
> > > >> structure as long as they should stay alive, which is much more intuitive
> > > >> anyways.
> > > >
> > > > We can't easily do this, because dropping a root directory recursively
> > > > drops everything underneath it. This means that if I have
> > > >
> > > > foo/
> > > >   - bar/
> > > >   - baz/
> > > >
> > > > Then my directory handle for `bar` have to be guaranteed to outlive my
> > > > directory handle for `foo` so that I know it's didn't get deleted
> > > > under me. This is why they have a borrow onto their parent directory.
> > > > This borrow means that you can't (without `unsafe`, or something like
> > > > `yoke`) keep handles to `foo` and `bar` in the same struct.
> > > 
> > > Is there no refcount that we can use instead of borrowing? I guess not,
> > > since one can call `debugfs_remove`. What about a refcount on the rust
> > > side? or is debugfs not used for "debugging" and needs to have the
> > > performance of no refcount?
> > 
> > debugfs should never have any performance issues (i.e. you don't use it
> > for performant things.)
> > 
> > So refcount away!  That should never be an issue.
> 
> What I imagine would be the ideal API for Rust is the following:
> 
> * For each file / dir you create, you get a Rust object that owns it.
> 
> * When you destroy one of these Rust objects, it disappears from the
>   file system. I.e., dropping a directory removes things recursively.
> 
> * If you remove a directory before the removing objects inside it, then
>   the Rust objects become "ghost" objects that are still usable, but not
>   visible in the file system anywhere. I.e. calling methods on them
>   succeed but are no-ops.

If we would want to keep an entry alive as long as there are more leaves, we'd
obviously need to reference count things.

But what do we need reference counting for with this logic? Shouldn't this be
also possible without?

> * Possibly we have a way to drop a Rust object without removing it from
>   the file system. In this case, it can never be accessed from Rust
>   again, and the only way to remove it is to drop its parent directory.

I'm not sure about this one.

IIUC, this basically brings back the "keep() logic", which I still think is a
footgun and should be avoided.

Also, we only needed this, since due to the borrowing design we couldn't store
parent and child nodes in the same structure. With reference counting (or the
logic above) this goes away.

I wrote the following in a previous conversation [1].

--

What I see more likely to happen is a situation where the "root" directory
(almost) lives forever, and hence subsequent calls, such as

	root.subdir("foo").keep()

effectively are leaks.

One specific example for that would be usb_debug_root, which is created in the
module scope of usb-common and is used by USB host / gadget / phy drivers.

The same is true for other cases where the debugfs "root" is created in the
module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively leak memory everytime a
device is unplugged (or unbound in general).

--

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

> This way, you can drop foo/ before dropping bar/ and baz/ without that
> having to be unsafe.
> 
> Whether that's best implemented by calling dget/dput on the dentry or by
> having Rust keep track of a separate Rust-only refcount, I don't know.
> But I think this is the API we want.
> 
> Thoughts?
> 
> Alice

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

[Alice Ryhl]

On Wed, May 21, 2025 at 06:47:40AM +0200, Greg Kroah-Hartman wrote:
> On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >> However, I really think we should keep the code as it is in this version and
> > > > >> just don't provide an example that utilizes ManuallyDrop and forget().
> > > > >>
> > > > >> I don't see how the idea of "manually dropping" (sub-)directories and files
> > > > >> provides any real value compared to just storing their instance in a driver
> > > > >> structure as long as they should stay alive, which is much more intuitive
> > > > >> anyways.
> > > > >
> > > > > We can't easily do this, because dropping a root directory recursively
> > > > > drops everything underneath it. This means that if I have
> > > > >
> > > > > foo/
> > > > >   - bar/
> > > > >   - baz/
> > > > >
> > > > > Then my directory handle for `bar` have to be guaranteed to outlive my
> > > > > directory handle for `foo` so that I know it's didn't get deleted
> > > > > under me. This is why they have a borrow onto their parent directory.
> > > > > This borrow means that you can't (without `unsafe`, or something like
> > > > > `yoke`) keep handles to `foo` and `bar` in the same struct.
> > > > 
> > > > Is there no refcount that we can use instead of borrowing? I guess not,
> > > > since one can call `debugfs_remove`. What about a refcount on the rust
> > > > side? or is debugfs not used for "debugging" and needs to have the
> > > > performance of no refcount?
> > > 
> > > debugfs should never have any performance issues (i.e. you don't use it
> > > for performant things.)
> > > 
> > > So refcount away!  That should never be an issue.
> > 
> > What I imagine would be the ideal API for Rust is the following:
> > 
> > * For each file / dir you create, you get a Rust object that owns it.
> > 
> > * When you destroy one of these Rust objects, it disappears from the
> >   file system. I.e., dropping a directory removes things recursively.
> > 
> > * If you remove a directory before the removing objects inside it, then
> >   the Rust objects become "ghost" objects that are still usable, but not
> >   visible in the file system anywhere. I.e. calling methods on them
> >   succeed but are no-ops.
> 
> Why not just also remove those objects at the same time?  That would be
> more like what the filesystem logic itself does today.

I mean, if I write a driver and I store a Rust object that holds a
debugfs directory in some random struct of mine, how is debugfs going to
go remove it from my struct? It can't.

At most we could enforce that you destroy them in the right order, but
actually designing an API which enforces that is difficult and results
in something inconvenient to use.

Of course, drivers probably shouldn't keep those ghost objects around,
but I don't think the API should hard enforce that.

Alice

> > * Possibly we have a way to drop a Rust object without removing it from
> >   the file system. In this case, it can never be accessed from Rust
> >   again, and the only way to remove it is to drop its parent directory.
> 
> This too would be nice as that's how the existing api works in C.
> 
> > This way, you can drop foo/ before dropping bar/ and baz/ without that
> > having to be unsafe.
> > 
> > Whether that's best implemented by calling dget/dput on the dentry or by
> > having Rust keep track of a separate Rust-only refcount, I don't know.
> > But I think this is the API we want.
> > 
> > Thoughts?
> 
> Sounds reasonable to me and should be easy to use, which is the key
> feature/requirement of debugfs.
> 
> thanks,
> 
> greg k-h

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

[Alice Ryhl]

On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > > > Is there no refcount that we can use instead of borrowing? I guess not,
> > > > since one can call `debugfs_remove`. What about a refcount on the rust
> > > > side? or is debugfs not used for "debugging" and needs to have the
> > > > performance of no refcount?
> > > 
> > > debugfs should never have any performance issues (i.e. you don't use it
> > > for performant things.)
> > > 
> > > So refcount away!  That should never be an issue.
> > 
> > What I imagine would be the ideal API for Rust is the following:
> > 
> > * For each file / dir you create, you get a Rust object that owns it.
> > 
> > * When you destroy one of these Rust objects, it disappears from the
> >   file system. I.e., dropping a directory removes things recursively.
> > 
> > * If you remove a directory before the removing objects inside it, then
> >   the Rust objects become "ghost" objects that are still usable, but not
> >   visible in the file system anywhere. I.e. calling methods on them
> >   succeed but are no-ops.
> 
> If we would want to keep an entry alive as long as there are more leaves, we'd
> obviously need to reference count things.
> 
> But what do we need reference counting for with this logic? Shouldn't this be
> also possible without?

Well, my understanding is that when you remove the parent directory, the
dentries for child directories and files are freed, so trying to use the
Rust objects that correspond to those child dentries would be a UAF. I
want to refcount those child entries so that they at least remain valid
memory even if they're no longer visible in the file system.

> > * Possibly we have a way to drop a Rust object without removing it from
> >   the file system. In this case, it can never be accessed from Rust
> >   again, and the only way to remove it is to drop its parent directory.
> 
> I'm not sure about this one.
> 
> IIUC, this basically brings back the "keep() logic", which I still think is a
> footgun and should be avoided.
> 
> Also, we only needed this, since due to the borrowing design we couldn't store
> parent and child nodes in the same structure. With reference counting (or the
> logic above) this goes away.
> 
> I wrote the following in a previous conversation [1].
> 
> --
> 
> What I see more likely to happen is a situation where the "root" directory
> (almost) lives forever, and hence subsequent calls, such as
> 
> 	root.subdir("foo").keep()
> 
> effectively are leaks.
> 
> One specific example for that would be usb_debug_root, which is created in the
> module scope of usb-common and is used by USB host / gadget / phy drivers.
> 
> The same is true for other cases where the debugfs "root" is created in the
> module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively leak memory everytime a
> device is unplugged (or unbound in general).

Where is the leak? If the file is still visible in the file system, then
it's not a leak to still have the memory. If the file got removed by
removing its parent, then my intent is that this should free the memory
of the child object.

Alice

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

[Danilo Krummrich]

On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> > On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > > * If you remove a directory before the removing objects inside it, then
> > >   the Rust objects become "ghost" objects that are still usable, but not
> > >   visible in the file system anywhere. I.e. calling methods on them
> > >   succeed but are no-ops.
> > 
> > If we would want to keep an entry alive as long as there are more leaves, we'd
> > obviously need to reference count things.
> > 
> > But what do we need reference counting for with this logic? Shouldn't this be
> > also possible without?
> 
> Well, my understanding is that when you remove the parent directory, the
> dentries for child directories and files are freed, so trying to use the
> Rust objects that correspond to those child dentries would be a UAF. I
> want to refcount those child entries so that they at least remain valid
> memory even if they're no longer visible in the file system.

Yes, that makes sense.

Instead of using the dentry pointer as a handle, we could also use the entry's
path and do a lookup whenever the entry is used. Not saying this is better
though.

> > > * Possibly we have a way to drop a Rust object without removing it from
> > >   the file system. In this case, it can never be accessed from Rust
> > >   again, and the only way to remove it is to drop its parent directory.
> > 
> > I'm not sure about this one.
> > 
> > IIUC, this basically brings back the "keep() logic", which I still think is a
> > footgun and should be avoided.
> > 
> > Also, we only needed this, since due to the borrowing design we couldn't store
> > parent and child nodes in the same structure. With reference counting (or the
> > logic above) this goes away.
> > 
> > I wrote the following in a previous conversation [1].
> > 
> > --
> > 
> > What I see more likely to happen is a situation where the "root" directory
> > (almost) lives forever, and hence subsequent calls, such as
> > 
> > 	root.subdir("foo").keep()
> > 
> > effectively are leaks.
> > 
> > One specific example for that would be usb_debug_root, which is created in the
> > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > 
> > The same is true for other cases where the debugfs "root" is created in the
> > module scope, but subsequent entries are created by driver instances. If a
> > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > device is unplugged (or unbound in general).
> 
> Where is the leak? If the file is still visible in the file system, then
> it's not a leak to still have the memory. If the file got removed by
> removing its parent, then my intent is that this should free the memory
> of the child object.

Well, take the case I described above, where the debugfs "root" is created in
the module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively the file / directory
(and subsequently also the corresponding memory) everytime a device is unplugged
(or unbound in general)."

If the module is built-in the directory from the module scope is *never*
removed, but the entries the driver e.g. creates in probe() for a particular
device with keep() will pile up endlessly, especially for hot-pluggable devices.

(It's getting even worse when there's data bound to such a leaked file, that
might even contain a vtable that is entered from any of the fops of the file.)

That'd be clearly a bug, but for the driver author calling keep() seems like a
valid thing to do -- to me that's clearly a built-in footgun.

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

[Greg Kroah-Hartman]

On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> > > On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > > > * If you remove a directory before the removing objects inside it, then
> > > >   the Rust objects become "ghost" objects that are still usable, but not
> > > >   visible in the file system anywhere. I.e. calling methods on them
> > > >   succeed but are no-ops.
> > > 
> > > If we would want to keep an entry alive as long as there are more leaves, we'd
> > > obviously need to reference count things.
> > > 
> > > But what do we need reference counting for with this logic? Shouldn't this be
> > > also possible without?
> > 
> > Well, my understanding is that when you remove the parent directory, the
> > dentries for child directories and files are freed, so trying to use the
> > Rust objects that correspond to those child dentries would be a UAF. I
> > want to refcount those child entries so that they at least remain valid
> > memory even if they're no longer visible in the file system.
> 
> Yes, that makes sense.
> 
> Instead of using the dentry pointer as a handle, we could also use the entry's
> path and do a lookup whenever the entry is used. Not saying this is better
> though.

Either is fine, as long as that "handle" isn't exported out of the
internal binding for anyone to access directly.

> > > What I see more likely to happen is a situation where the "root" directory
> > > (almost) lives forever, and hence subsequent calls, such as
> > > 
> > > 	root.subdir("foo").keep()
> > > 
> > > effectively are leaks.
> > > 
> > > One specific example for that would be usb_debug_root, which is created in the
> > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > 
> > > The same is true for other cases where the debugfs "root" is created in the
> > > module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > device is unplugged (or unbound in general).
> > 
> > Where is the leak? If the file is still visible in the file system, then
> > it's not a leak to still have the memory. If the file got removed by
> > removing its parent, then my intent is that this should free the memory
> > of the child object.
> 
> Well, take the case I described above, where the debugfs "root" is created in
> the module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively the file / directory
> (and subsequently also the corresponding memory) everytime a device is unplugged
> (or unbound in general)."
> 
> If the module is built-in the directory from the module scope is *never*
> removed, but the entries the driver e.g. creates in probe() for a particular
> device with keep() will pile up endlessly, especially for hot-pluggable devices.
> 
> (It's getting even worse when there's data bound to such a leaked file, that
> might even contain a vtable that is entered from any of the fops of the file.)
> 
> That'd be clearly a bug, but for the driver author calling keep() seems like a
> valid thing to do -- to me that's clearly a built-in footgun.

Yeah, I like the keep() thing less and less and I think it can be done
without it entirely.

thanks,

greg k-h

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

[Alice Ryhl]

On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > * Possibly we have a way to drop a Rust object without removing it from
> > > >   the file system. In this case, it can never be accessed from Rust
> > > >   again, and the only way to remove it is to drop its parent directory.
> > > 
> > > I'm not sure about this one.
> > > 
> > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > footgun and should be avoided.
> > > 
> > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > parent and child nodes in the same structure. With reference counting (or the
> > > logic above) this goes away.
> > > 
> > > I wrote the following in a previous conversation [1].
> > > 
> > > --
> > > 
> > > What I see more likely to happen is a situation where the "root" directory
> > > (almost) lives forever, and hence subsequent calls, such as
> > > 
> > > 	root.subdir("foo").keep()
> > > 
> > > effectively are leaks.
> > > 
> > > One specific example for that would be usb_debug_root, which is created in the
> > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > 
> > > The same is true for other cases where the debugfs "root" is created in the
> > > module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > device is unplugged (or unbound in general).
> > 
> > Where is the leak? If the file is still visible in the file system, then
> > it's not a leak to still have the memory. If the file got removed by
> > removing its parent, then my intent is that this should free the memory
> > of the child object.
> 
> Well, take the case I described above, where the debugfs "root" is created in
> the module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively the file / directory
> (and subsequently also the corresponding memory) everytime a device is unplugged
> (or unbound in general)."
> 
> If the module is built-in the directory from the module scope is *never*
> removed, but the entries the driver e.g. creates in probe() for a particular
> device with keep() will pile up endlessly, especially for hot-pluggable devices.
> 
> (It's getting even worse when there's data bound to such a leaked file, that
> might even contain a vtable that is entered from any of the fops of the file.)
> 
> That'd be clearly a bug, but for the driver author calling keep() seems like a
> valid thing to do -- to me that's clearly a built-in footgun.

I mean, for cases such as this, I could imagine that you use `keep()` on
the files stored inside of the driver directory, but don't use it on the
directory. That way, you only have to keep a single reference to an
entire directory around, which may be more convenient.

But I also see your point. I don't think keep() is a critical part of
what I think the approach should be.

Alice

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

[Greg Kroah-Hartman]

On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > >   the file system. In this case, it can never be accessed from Rust
> > > > >   again, and the only way to remove it is to drop its parent directory.
> > > > 
> > > > I'm not sure about this one.
> > > > 
> > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > footgun and should be avoided.
> > > > 
> > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > parent and child nodes in the same structure. With reference counting (or the
> > > > logic above) this goes away.
> > > > 
> > > > I wrote the following in a previous conversation [1].
> > > > 
> > > > --
> > > > 
> > > > What I see more likely to happen is a situation where the "root" directory
> > > > (almost) lives forever, and hence subsequent calls, such as
> > > > 
> > > > 	root.subdir("foo").keep()
> > > > 
> > > > effectively are leaks.
> > > > 
> > > > One specific example for that would be usb_debug_root, which is created in the
> > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > > 
> > > > The same is true for other cases where the debugfs "root" is created in the
> > > > module scope, but subsequent entries are created by driver instances. If a
> > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > device is unplugged (or unbound in general).
> > > 
> > > Where is the leak? If the file is still visible in the file system, then
> > > it's not a leak to still have the memory. If the file got removed by
> > > removing its parent, then my intent is that this should free the memory
> > > of the child object.
> > 
> > Well, take the case I described above, where the debugfs "root" is created in
> > the module scope, but subsequent entries are created by driver instances. If a
> > driver would use keep() in such a case, we'd effectively the file / directory
> > (and subsequently also the corresponding memory) everytime a device is unplugged
> > (or unbound in general)."
> > 
> > If the module is built-in the directory from the module scope is *never*
> > removed, but the entries the driver e.g. creates in probe() for a particular
> > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > 
> > (It's getting even worse when there's data bound to such a leaked file, that
> > might even contain a vtable that is entered from any of the fops of the file.)
> > 
> > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > valid thing to do -- to me that's clearly a built-in footgun.
> 
> I mean, for cases such as this, I could imagine that you use `keep()` on
> the files stored inside of the driver directory, but don't use it on the
> directory. That way, you only have to keep a single reference to an
> entire directory around, which may be more convenient.

No, sorry, but debugfs files are "create and forget" type of things.
The caller has NO reference back to the file at all in the C version,
let's not add that functionality back to the rust side after I spent a
long time removing it from the C code :)

If you really want to delete a debugfs file that you have created in the
past, then look it up and delete it with the call that is present for
that.

The only thing I think that might be worth "keeping" in some form, as an
object reference as discussed, is a debugfs directory.

thanks,

greg k-h

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

[Alice Ryhl]

On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > Well, take the case I described above, where the debugfs "root" is created in
> > > the module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively the file / directory
> > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > (or unbound in general)."
> > > 
> > > If the module is built-in the directory from the module scope is *never*
> > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > 
> > > (It's getting even worse when there's data bound to such a leaked file, that
> > > might even contain a vtable that is entered from any of the fops of the file.)
> > > 
> > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > valid thing to do -- to me that's clearly a built-in footgun.
> > 
> > I mean, for cases such as this, I could imagine that you use `keep()` on
> > the files stored inside of the driver directory, but don't use it on the
> > directory. That way, you only have to keep a single reference to an
> > entire directory around, which may be more convenient.
> 
> No, sorry, but debugfs files are "create and forget" type of things.
> The caller has NO reference back to the file at all in the C version,
> let's not add that functionality back to the rust side after I spent a
> long time removing it from the C code :)
> 
> If you really want to delete a debugfs file that you have created in the
> past, then look it up and delete it with the call that is present for
> that.
> 
> The only thing I think that might be worth "keeping" in some form, as an
> object reference as discussed, is a debugfs directory.

That could work if we don't have any Rust value for files at all. The
problem is that if we do have such values, then code like this:

let my_file = dir.create_file("my_file_name");
dir.delete_file("my_file_name");
my_file.do_something();

would be a UAF on the last line. We have to design the Rust API to avoid
such UAF, which is why I suggested the ghost objects; the delete_file()
call leaves my_file in a valid but useless state. And as a ghost object,
the .do_something() call becomes a no-op since the file is now missing
from the filesystem.

Alice

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

[Danilo Krummrich]

On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> > On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > > >   the file system. In this case, it can never be accessed from Rust
> > > > > >   again, and the only way to remove it is to drop its parent directory.
> > > > > 
> > > > > I'm not sure about this one.
> > > > > 
> > > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > > footgun and should be avoided.
> > > > > 
> > > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > > parent and child nodes in the same structure. With reference counting (or the
> > > > > logic above) this goes away.
> > > > > 
> > > > > I wrote the following in a previous conversation [1].
> > > > > 
> > > > > --
> > > > > 
> > > > > What I see more likely to happen is a situation where the "root" directory
> > > > > (almost) lives forever, and hence subsequent calls, such as
> > > > > 
> > > > > 	root.subdir("foo").keep()
> > > > > 
> > > > > effectively are leaks.
> > > > > 
> > > > > One specific example for that would be usb_debug_root, which is created in the
> > > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > > > 
> > > > > The same is true for other cases where the debugfs "root" is created in the
> > > > > module scope, but subsequent entries are created by driver instances. If a
> > > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > > device is unplugged (or unbound in general).
> > > > 
> > > > Where is the leak? If the file is still visible in the file system, then
> > > > it's not a leak to still have the memory. If the file got removed by
> > > > removing its parent, then my intent is that this should free the memory
> > > > of the child object.
> > > 
> > > Well, take the case I described above, where the debugfs "root" is created in
> > > the module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively the file / directory
> > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > (or unbound in general)."
> > > 
> > > If the module is built-in the directory from the module scope is *never*
> > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > 
> > > (It's getting even worse when there's data bound to such a leaked file, that
> > > might even contain a vtable that is entered from any of the fops of the file.)
> > > 
> > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > valid thing to do -- to me that's clearly a built-in footgun.
> > 
> > I mean, for cases such as this, I could imagine that you use `keep()` on
> > the files stored inside of the driver directory, but don't use it on the
> > directory. That way, you only have to keep a single reference to an
> > entire directory around, which may be more convenient.
> 
> No, sorry, but debugfs files are "create and forget" type of things.
> The caller has NO reference back to the file at all in the C version,
> let's not add that functionality back to the rust side after I spent a
> long time removing it from the C code :)

This response sounds like we may have a different understanding of what "keep"
means.

Earlier versions of this patch series introduced a keep() method for both
debugfs::File and debugfs::Dir, which would consume, and hence get rid of, the
instance of a debugfs::File or a debugfs::Dir object, while *keeping* the
corresponding directory / file in the filesystem.

This makes it easy for a driver to leak the directory / file in the filesystem,
as explained in [1], which you seemed to agree with in [2].

Was this a misunderstanding?

> If you really want to delete a debugfs file that you have created in the
> past, then look it up and delete it with the call that is present for
> that.

This is promoting a C pattern (which for C code obviously makes a lot of sense),
i.e. we have a function that creates a thing and another function that removes
the thing given a handle of the created thing. Whether the handle is valid is
the responsibility of the caller.

In this case the handle would be the filename. For instance:

	debugfs_create_file("foo", parent, ...);
	debugfs_remove(debugfs_lookup("foo", parent));

This leaves room to the caller for making mistakes, e.g. if the caller makes a
typo in the filename. In this case we wouldn't even recognize it from the return
value, since there is none.

In Rust, we do things differently, i.e. we wrap things into an object that
cleans up itself, once it goes out of scope. For instance:

	let file = debugfs::File::new("foo", parent);

Subsequently we store file in a structure that represents the time we want this
file to exist. Once it goes out of scope, it's removed automatically.

This is better, since we can't make any mistakes anymore, i.e. we can't mess up
the filename or pass the wrong parent to clean things up.

Depending on whether the above was a misunderstanding and depending on how you
think about it with this rationale, I have quite some more reasons why we don't
want to have files / directories around in the filesystem without a
corresponding object representation in Rust.

But before I write up a lot more text, I'd like to see if we're not already on
the same page. :-)

> The only thing I think that might be worth "keeping" in some form, as an
> object reference as discussed, is a debugfs directory.

[1] https://lore.kernel.org/lkml/aC7DVewqqWIKetmk@pollux/
[2] https://lore.kernel.org/lkml/2025052205-thing-daylight-1115@gregkh/

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

[Benno Lossin]

On Thu May 22, 2025 at 7:40 PM CEST, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
>> No, sorry, but debugfs files are "create and forget" type of things.
>> The caller has NO reference back to the file at all in the C version,
>> let's not add that functionality back to the rust side after I spent a
>> long time removing it from the C code :)
>> 
>> If you really want to delete a debugfs file that you have created in the
>> past, then look it up and delete it with the call that is present for
>> that.
>> 
>> The only thing I think that might be worth "keeping" in some form, as an
>> object reference as discussed, is a debugfs directory.
>
> That could work if we don't have any Rust value for files at all. The
> problem is that if we do have such values, then code like this:
>
> let my_file = dir.create_file("my_file_name");
> dir.delete_file("my_file_name");
> my_file.do_something();

I might have misunderstood something, but "deleting a debugfs file" is
not the same as freeing the representing object (is that a dentry?). So
you could still call `do_something`, it just wouldn't do anything.

Or did I misunderstand?

---
Cheers,
Benno

> would be a UAF on the last line. We have to design the Rust API to avoid
> such UAF, which is why I suggested the ghost objects; the delete_file()
> call leaves my_file in a valid but useless state. And as a ghost object,
> the .do_something() call becomes a no-op since the file is now missing
> from the filesystem.
>
> Alice

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

[Greg Kroah-Hartman]

On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> > > On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > > > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > > > >   the file system. In this case, it can never be accessed from Rust
> > > > > > >   again, and the only way to remove it is to drop its parent directory.
> > > > > > 
> > > > > > I'm not sure about this one.
> > > > > > 
> > > > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > > > footgun and should be avoided.
> > > > > > 
> > > > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > > > parent and child nodes in the same structure. With reference counting (or the
> > > > > > logic above) this goes away.
> > > > > > 
> > > > > > I wrote the following in a previous conversation [1].
> > > > > > 
> > > > > > --
> > > > > > 
> > > > > > What I see more likely to happen is a situation where the "root" directory
> > > > > > (almost) lives forever, and hence subsequent calls, such as
> > > > > > 
> > > > > > 	root.subdir("foo").keep()
> > > > > > 
> > > > > > effectively are leaks.
> > > > > > 
> > > > > > One specific example for that would be usb_debug_root, which is created in the
> > > > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > > > > 
> > > > > > The same is true for other cases where the debugfs "root" is created in the
> > > > > > module scope, but subsequent entries are created by driver instances. If a
> > > > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > > > device is unplugged (or unbound in general).
> > > > > 
> > > > > Where is the leak? If the file is still visible in the file system, then
> > > > > it's not a leak to still have the memory. If the file got removed by
> > > > > removing its parent, then my intent is that this should free the memory
> > > > > of the child object.
> > > > 
> > > > Well, take the case I described above, where the debugfs "root" is created in
> > > > the module scope, but subsequent entries are created by driver instances. If a
> > > > driver would use keep() in such a case, we'd effectively the file / directory
> > > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > > (or unbound in general)."
> > > > 
> > > > If the module is built-in the directory from the module scope is *never*
> > > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > > 
> > > > (It's getting even worse when there's data bound to such a leaked file, that
> > > > might even contain a vtable that is entered from any of the fops of the file.)
> > > > 
> > > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > > valid thing to do -- to me that's clearly a built-in footgun.
> > > 
> > > I mean, for cases such as this, I could imagine that you use `keep()` on
> > > the files stored inside of the driver directory, but don't use it on the
> > > directory. That way, you only have to keep a single reference to an
> > > entire directory around, which may be more convenient.
> > 
> > No, sorry, but debugfs files are "create and forget" type of things.
> > The caller has NO reference back to the file at all in the C version,
> > let's not add that functionality back to the rust side after I spent a
> > long time removing it from the C code :)
> 
> This response sounds like we may have a different understanding of what "keep"
> means.
> 
> Earlier versions of this patch series introduced a keep() method for both
> debugfs::File and debugfs::Dir, which would consume, and hence get rid of, the
> instance of a debugfs::File or a debugfs::Dir object, while *keeping* the
> corresponding directory / file in the filesystem.
> 
> This makes it easy for a driver to leak the directory / file in the filesystem,
> as explained in [1], which you seemed to agree with in [2].
> 
> Was this a misunderstanding?

Kind of, see below:

> > If you really want to delete a debugfs file that you have created in the
> > past, then look it up and delete it with the call that is present for
> > that.
> 
> This is promoting a C pattern (which for C code obviously makes a lot of sense),
> i.e. we have a function that creates a thing and another function that removes
> the thing given a handle of the created thing. Whether the handle is valid is
> the responsibility of the caller.
> 
> In this case the handle would be the filename. For instance:
> 
> 	debugfs_create_file("foo", parent, ...);
> 	debugfs_remove(debugfs_lookup("foo", parent));
> 
> This leaves room to the caller for making mistakes, e.g. if the caller makes a
> typo in the filename. In this case we wouldn't even recognize it from the return
> value, since there is none.
> 
> In Rust, we do things differently, i.e. we wrap things into an object that
> cleans up itself, once it goes out of scope. For instance:
> 
> 	let file = debugfs::File::new("foo", parent);
> 
> Subsequently we store file in a structure that represents the time we want this
> file to exist. Once it goes out of scope, it's removed automatically.
> 
> This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> the filename or pass the wrong parent to clean things up.
> 
> Depending on whether the above was a misunderstanding and depending on how you
> think about it with this rationale, I have quite some more reasons why we don't
> want to have files / directories around in the filesystem without a
> corresponding object representation in Rust.
> 
> But before I write up a lot more text, I'd like to see if we're not already on
> the same page. :-)

Ok, let's knock up the api here first before worrying about the
implementation, as we seem to all be a bit confused as to what we want
to do.

Ideally, yes, I would like to NOT have any rust structure represent a
debugfs file, as that is the way the C api has been evolving.  We are
one step away from debugfs_create_file() being a void function that
doesn't return anything, and I don't want to see us go "backwards" here.

But the main reason I did that work was to keep people from trying to
determine if a file creation worked or not and to do different logic
based on that.  I think the Rust api CAN do both here, as you and Alice
have explained, so we should be ok.

So how about this for an example api, two structures, debugfs::Directory
and debugfs::File, which have only two types of operations that can be
done on them:

To create a debugfs directory, we do:

	let my_dir = debugfs::Directory::new("foo_dir", parent);

There is no other Directory method, other than "drop", when the object
goes out of scope, which will clean up the directory and ALL child
objects at the same time (implementation details to be figured out
later.)

That will ALWAYS succeed no matter if the backend debugfs call failed or
not, and you have no way of knowing if it really did create something or
not.  That directory you can then use to pass into debugfs file or
directory creation functions.

Same for creating a debugfs file, you would do something like:

	let my_file = debugfs::File::new("foo_file", my_dir);

depending on the "file type" you want to create, there will be different
new_* methods (new_u32, new_u64, etc.).

which also ALWAYS succeeds.  And again, as you want to keep objects
around, you have to have a reference on it at all times for it to exist
in the filesystem.  If you drop it, then it too will be removed from
debugfs.

And again, that's the only operation you can do on this object, there
are no other methods for it other than "new" and "drop".

Now there are issues with implementation details here like:
  - Do you want to keep the list of file and dir objects in the rust
    structure to manually clean them up when we go to drop the
    directory?
  - Do we want to force all files to be dropped before the directory?
    Or do we want to just let the C side clean it all up automagically
    instead?
and other things like that, but we can argue about that once we settle
on the outward-facing api first.

I can live with this type of api.  It's simple and seems hard to abuse
or get wrong from a user point-of-view, and should be pretty
straight-forward on the binding side as well.  It will take a bit more
work on the user when creating debugfs files than the C api did, but
that's something that you want to impose on this api, not me :)

Sound reasonable?  Or am I missing something else here?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, May 22, 2025 at 05:40:43PM +0000, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > > Well, take the case I described above, where the debugfs "root" is created in
> > > > the module scope, but subsequent entries are created by driver instances. If a
> > > > driver would use keep() in such a case, we'd effectively the file / directory
> > > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > > (or unbound in general)."
> > > > 
> > > > If the module is built-in the directory from the module scope is *never*
> > > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > > 
> > > > (It's getting even worse when there's data bound to such a leaked file, that
> > > > might even contain a vtable that is entered from any of the fops of the file.)
> > > > 
> > > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > > valid thing to do -- to me that's clearly a built-in footgun.
> > > 
> > > I mean, for cases such as this, I could imagine that you use `keep()` on
> > > the files stored inside of the driver directory, but don't use it on the
> > > directory. That way, you only have to keep a single reference to an
> > > entire directory around, which may be more convenient.
> > 
> > No, sorry, but debugfs files are "create and forget" type of things.
> > The caller has NO reference back to the file at all in the C version,
> > let's not add that functionality back to the rust side after I spent a
> > long time removing it from the C code :)
> > 
> > If you really want to delete a debugfs file that you have created in the
> > past, then look it up and delete it with the call that is present for
> > that.
> > 
> > The only thing I think that might be worth "keeping" in some form, as an
> > object reference as discussed, is a debugfs directory.
> 
> That could work if we don't have any Rust value for files at all. The
> problem is that if we do have such values, then code like this:
> 
> let my_file = dir.create_file("my_file_name");
> dir.delete_file("my_file_name");
> my_file.do_something();
> 
> would be a UAF on the last line. We have to design the Rust API to avoid
> such UAF, which is why I suggested the ghost objects; the delete_file()
> call leaves my_file in a valid but useless state. And as a ghost object,
> the .do_something() call becomes a no-op since the file is now missing
> from the filesystem.

See my other response now, but I don't want there to be any
.do_something() calls that are possible here at all.  All a debugfs file
object can do is be created, or be destroyed.

thanks,

greg k-h

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

[Danilo Krummrich]

On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > If you really want to delete a debugfs file that you have created in the
> > > past, then look it up and delete it with the call that is present for
> > > that.
> > 
> > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > i.e. we have a function that creates a thing and another function that removes
> > the thing given a handle of the created thing. Whether the handle is valid is
> > the responsibility of the caller.
> > 
> > In this case the handle would be the filename. For instance:
> > 
> > 	debugfs_create_file("foo", parent, ...);
> > 	debugfs_remove(debugfs_lookup("foo", parent));
> > 
> > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > typo in the filename. In this case we wouldn't even recognize it from the return
> > value, since there is none.
> > 
> > In Rust, we do things differently, i.e. we wrap things into an object that
> > cleans up itself, once it goes out of scope. For instance:
> > 
> > 	let file = debugfs::File::new("foo", parent);
> > 
> > Subsequently we store file in a structure that represents the time we want this
> > file to exist. Once it goes out of scope, it's removed automatically.
> > 
> > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > the filename or pass the wrong parent to clean things up.
> > 
> > Depending on whether the above was a misunderstanding and depending on how you
> > think about it with this rationale, I have quite some more reasons why we don't
> > want to have files / directories around in the filesystem without a
> > corresponding object representation in Rust.
> > 
> > But before I write up a lot more text, I'd like to see if we're not already on
> > the same page. :-)
> 
> Ok, let's knock up the api here first before worrying about the
> implementation, as we seem to all be a bit confused as to what we want
> to do.
> 
> Ideally, yes, I would like to NOT have any rust structure represent a
> debugfs file, as that is the way the C api has been evolving.  We are
> one step away from debugfs_create_file() being a void function that
> doesn't return anything, and I don't want to see us go "backwards" here.
> 
> But the main reason I did that work was to keep people from trying to
> determine if a file creation worked or not and to do different logic
> based on that.  I think the Rust api CAN do both here, as you and Alice
> have explained, so we should be ok.
> 
> So how about this for an example api, two structures, debugfs::Directory
> and debugfs::File, which have only two types of operations that can be
> done on them:
> 
> To create a debugfs directory, we do:
> 
> 	let my_dir = debugfs::Directory::new("foo_dir", parent);
> 
> There is no other Directory method, other than "drop", when the object
> goes out of scope, which will clean up the directory and ALL child
> objects at the same time (implementation details to be figured out
> later.)
> 
> That will ALWAYS succeed no matter if the backend debugfs call failed or
> not, and you have no way of knowing if it really did create something or
> not.  That directory you can then use to pass into debugfs file or
> directory creation functions.
> 
> Same for creating a debugfs file, you would do something like:
> 
> 	let my_file = debugfs::File::new("foo_file", my_dir);
> 
> depending on the "file type" you want to create, there will be different
> new_* methods (new_u32, new_u64, etc.).
> 
> which also ALWAYS succeeds.  And again, as you want to keep objects
> around, you have to have a reference on it at all times for it to exist
> in the filesystem.  If you drop it, then it too will be removed from
> debugfs.

Great! That's exactly what I think the API should look like as well. :-)

> And again, that's the only operation you can do on this object, there
> are no other methods for it other than "new" and "drop".

We probably still want

	let foo_dir = my_dir.subdir("foo_dir")
	foo_dir.file("foo_file")

for convinience, rather than having to call

	let bar_dir = Dir::new("foo_dir", my_dir)
	File::new("bar_file", bar_dir)

all the time. But otherwise, sounds reasonable to me.

> Now there are issues with implementation details here like:
>   - Do you want to keep the list of file and dir objects in the rust
>     structure to manually clean them up when we go to drop the
>     directory?
>   - Do we want to force all files to be dropped before the directory?
>     Or do we want to just let the C side clean it all up automagically
>     instead?
> and other things like that, but we can argue about that once we settle
> on the outward-facing api first.

The only thing I don't want to is to allow to leak files or directories, i.e.

	File::new("bar_file", bar_dir).leak()

which keeps the file in the filesystem, but takes away the handle from the Rust
side, such that it won't be removed from the filesystem anymore when the handle
goes out of scope, which can cause nasty bugs. But I think there isn't any
benefit to allow this anyways and it isn't needed with reference counting.

I'm open on things like having "ghost" objects as proposed by Alice or force all
files to be dropped before the directory, etc. though.

> I can live with this type of api.  It's simple and seems hard to abuse
> or get wrong from a user point-of-view, and should be pretty
> straight-forward on the binding side as well.  It will take a bit more
> work on the user when creating debugfs files than the C api did, but
> that's something that you want to impose on this api, not me :)

Let's see if it actually does, I'm not sure it will be, at least not for all
use-cases. But even if, it's probably worth the extra robustness.

> Sound reasonable?  Or am I missing something else here?

Yes, sounds good to me.

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

[Greg Kroah-Hartman]

On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > > If you really want to delete a debugfs file that you have created in the
> > > > past, then look it up and delete it with the call that is present for
> > > > that.
> > > 
> > > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > > i.e. we have a function that creates a thing and another function that removes
> > > the thing given a handle of the created thing. Whether the handle is valid is
> > > the responsibility of the caller.
> > > 
> > > In this case the handle would be the filename. For instance:
> > > 
> > > 	debugfs_create_file("foo", parent, ...);
> > > 	debugfs_remove(debugfs_lookup("foo", parent));
> > > 
> > > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > > typo in the filename. In this case we wouldn't even recognize it from the return
> > > value, since there is none.
> > > 
> > > In Rust, we do things differently, i.e. we wrap things into an object that
> > > cleans up itself, once it goes out of scope. For instance:
> > > 
> > > 	let file = debugfs::File::new("foo", parent);
> > > 
> > > Subsequently we store file in a structure that represents the time we want this
> > > file to exist. Once it goes out of scope, it's removed automatically.
> > > 
> > > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > > the filename or pass the wrong parent to clean things up.
> > > 
> > > Depending on whether the above was a misunderstanding and depending on how you
> > > think about it with this rationale, I have quite some more reasons why we don't
> > > want to have files / directories around in the filesystem without a
> > > corresponding object representation in Rust.
> > > 
> > > But before I write up a lot more text, I'd like to see if we're not already on
> > > the same page. :-)
> > 
> > Ok, let's knock up the api here first before worrying about the
> > implementation, as we seem to all be a bit confused as to what we want
> > to do.
> > 
> > Ideally, yes, I would like to NOT have any rust structure represent a
> > debugfs file, as that is the way the C api has been evolving.  We are
> > one step away from debugfs_create_file() being a void function that
> > doesn't return anything, and I don't want to see us go "backwards" here.
> > 
> > But the main reason I did that work was to keep people from trying to
> > determine if a file creation worked or not and to do different logic
> > based on that.  I think the Rust api CAN do both here, as you and Alice
> > have explained, so we should be ok.
> > 
> > So how about this for an example api, two structures, debugfs::Directory
> > and debugfs::File, which have only two types of operations that can be
> > done on them:
> > 
> > To create a debugfs directory, we do:
> > 
> > 	let my_dir = debugfs::Directory::new("foo_dir", parent);
> > 
> > There is no other Directory method, other than "drop", when the object
> > goes out of scope, which will clean up the directory and ALL child
> > objects at the same time (implementation details to be figured out
> > later.)
> > 
> > That will ALWAYS succeed no matter if the backend debugfs call failed or
> > not, and you have no way of knowing if it really did create something or
> > not.  That directory you can then use to pass into debugfs file or
> > directory creation functions.
> > 
> > Same for creating a debugfs file, you would do something like:
> > 
> > 	let my_file = debugfs::File::new("foo_file", my_dir);
> > 
> > depending on the "file type" you want to create, there will be different
> > new_* methods (new_u32, new_u64, etc.).
> > 
> > which also ALWAYS succeeds.  And again, as you want to keep objects
> > around, you have to have a reference on it at all times for it to exist
> > in the filesystem.  If you drop it, then it too will be removed from
> > debugfs.
> 
> Great! That's exactly what I think the API should look like as well. :-)
> 
> > And again, that's the only operation you can do on this object, there
> > are no other methods for it other than "new" and "drop".
> 
> We probably still want
> 
> 	let foo_dir = my_dir.subdir("foo_dir")
> 	foo_dir.file("foo_file")
> 
> for convinience, rather than having to call
> 
> 	let bar_dir = Dir::new("foo_dir", my_dir)
> 	File::new("bar_file", bar_dir)
> 
> all the time. But otherwise, sounds reasonable to me.

That would be very convenient, but given that we have 20+ different
types of debugfs files that we can create, that might get complex to
define that way.  But that's just an implementation detail to work
out...

> > Now there are issues with implementation details here like:
> >   - Do you want to keep the list of file and dir objects in the rust
> >     structure to manually clean them up when we go to drop the
> >     directory?
> >   - Do we want to force all files to be dropped before the directory?
> >     Or do we want to just let the C side clean it all up automagically
> >     instead?
> > and other things like that, but we can argue about that once we settle
> > on the outward-facing api first.
> 
> The only thing I don't want to is to allow to leak files or directories, i.e.
> 
> 	File::new("bar_file", bar_dir).leak()
> 
> which keeps the file in the filesystem, but takes away the handle from the Rust
> side, such that it won't be removed from the filesystem anymore when the handle
> goes out of scope, which can cause nasty bugs. But I think there isn't any
> benefit to allow this anyways and it isn't needed with reference counting.

I agree, I can live with that too.

> I'm open on things like having "ghost" objects as proposed by Alice or force all
> files to be dropped before the directory, etc. though.

"ghost" objects will get messy I think, and yes, cleaning up will be a
bit tricky here, which is why at the C level I am taking away the idea
of a file that can be controlled as an individual object entirely.  If
you want the Rust code to have objects for the files, that's something
that the bindings are going to have to handle well.  Somehow :)

thanks,

greg k-h

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

[Alice Ryhl]

On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> > > > On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > > > > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > > > > >   the file system. In this case, it can never be accessed from Rust
> > > > > > > >   again, and the only way to remove it is to drop its parent directory.
> > > > > > > 
> > > > > > > I'm not sure about this one.
> > > > > > > 
> > > > > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > > > > footgun and should be avoided.
> > > > > > > 
> > > > > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > > > > parent and child nodes in the same structure. With reference counting (or the
> > > > > > > logic above) this goes away.
> > > > > > > 
> > > > > > > I wrote the following in a previous conversation [1].
> > > > > > > 
> > > > > > > --
> > > > > > > 
> > > > > > > What I see more likely to happen is a situation where the "root" directory
> > > > > > > (almost) lives forever, and hence subsequent calls, such as
> > > > > > > 
> > > > > > > 	root.subdir("foo").keep()
> > > > > > > 
> > > > > > > effectively are leaks.
> > > > > > > 
> > > > > > > One specific example for that would be usb_debug_root, which is created in the
> > > > > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > > > > > 
> > > > > > > The same is true for other cases where the debugfs "root" is created in the
> > > > > > > module scope, but subsequent entries are created by driver instances. If a
> > > > > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > > > > device is unplugged (or unbound in general).
> > > > > > 
> > > > > > Where is the leak? If the file is still visible in the file system, then
> > > > > > it's not a leak to still have the memory. If the file got removed by
> > > > > > removing its parent, then my intent is that this should free the memory
> > > > > > of the child object.
> > > > > 
> > > > > Well, take the case I described above, where the debugfs "root" is created in
> > > > > the module scope, but subsequent entries are created by driver instances. If a
> > > > > driver would use keep() in such a case, we'd effectively the file / directory
> > > > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > > > (or unbound in general)."
> > > > > 
> > > > > If the module is built-in the directory from the module scope is *never*
> > > > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > > > 
> > > > > (It's getting even worse when there's data bound to such a leaked file, that
> > > > > might even contain a vtable that is entered from any of the fops of the file.)
> > > > > 
> > > > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > > > valid thing to do -- to me that's clearly a built-in footgun.
> > > > 
> > > > I mean, for cases such as this, I could imagine that you use `keep()` on
> > > > the files stored inside of the driver directory, but don't use it on the
> > > > directory. That way, you only have to keep a single reference to an
> > > > entire directory around, which may be more convenient.
> > > 
> > > No, sorry, but debugfs files are "create and forget" type of things.
> > > The caller has NO reference back to the file at all in the C version,
> > > let's not add that functionality back to the rust side after I spent a
> > > long time removing it from the C code :)
> > 
> > This response sounds like we may have a different understanding of what "keep"
> > means.
> > 
> > Earlier versions of this patch series introduced a keep() method for both
> > debugfs::File and debugfs::Dir, which would consume, and hence get rid of, the
> > instance of a debugfs::File or a debugfs::Dir object, while *keeping* the
> > corresponding directory / file in the filesystem.
> > 
> > This makes it easy for a driver to leak the directory / file in the filesystem,
> > as explained in [1], which you seemed to agree with in [2].
> > 
> > Was this a misunderstanding?
> 
> Kind of, see below:
> 
> > > If you really want to delete a debugfs file that you have created in the
> > > past, then look it up and delete it with the call that is present for
> > > that.
> > 
> > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > i.e. we have a function that creates a thing and another function that removes
> > the thing given a handle of the created thing. Whether the handle is valid is
> > the responsibility of the caller.
> > 
> > In this case the handle would be the filename. For instance:
> > 
> > 	debugfs_create_file("foo", parent, ...);
> > 	debugfs_remove(debugfs_lookup("foo", parent));
> > 
> > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > typo in the filename. In this case we wouldn't even recognize it from the return
> > value, since there is none.
> > 
> > In Rust, we do things differently, i.e. we wrap things into an object that
> > cleans up itself, once it goes out of scope. For instance:
> > 
> > 	let file = debugfs::File::new("foo", parent);
> > 
> > Subsequently we store file in a structure that represents the time we want this
> > file to exist. Once it goes out of scope, it's removed automatically.
> > 
> > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > the filename or pass the wrong parent to clean things up.
> > 
> > Depending on whether the above was a misunderstanding and depending on how you
> > think about it with this rationale, I have quite some more reasons why we don't
> > want to have files / directories around in the filesystem without a
> > corresponding object representation in Rust.
> > 
> > But before I write up a lot more text, I'd like to see if we're not already on
> > the same page. :-)
> 
> Ok, let's knock up the api here first before worrying about the
> implementation, as we seem to all be a bit confused as to what we want
> to do.
> 
> Ideally, yes, I would like to NOT have any rust structure represent a
> debugfs file, as that is the way the C api has been evolving.  We are
> one step away from debugfs_create_file() being a void function that
> doesn't return anything, and I don't want to see us go "backwards" here.
> 
> But the main reason I did that work was to keep people from trying to
> determine if a file creation worked or not and to do different logic
> based on that.  I think the Rust api CAN do both here, as you and Alice
> have explained, so we should be ok.
> 
> So how about this for an example api, two structures, debugfs::Directory
> and debugfs::File, which have only two types of operations that can be
> done on them:
> 
> To create a debugfs directory, we do:
> 
> 	let my_dir = debugfs::Directory::new("foo_dir", parent);
> 
> There is no other Directory method, other than "drop", when the object
> goes out of scope, which will clean up the directory and ALL child
> objects at the same time (implementation details to be figured out
> later.)
> 
> That will ALWAYS succeed no matter if the backend debugfs call failed or
> not, and you have no way of knowing if it really did create something or
> not.  That directory you can then use to pass into debugfs file or
> directory creation functions.
> 
> Same for creating a debugfs file, you would do something like:
> 
> 	let my_file = debugfs::File::new("foo_file", my_dir);
> 
> depending on the "file type" you want to create, there will be different
> new_* methods (new_u32, new_u64, etc.).
> 
> which also ALWAYS succeeds.  And again, as you want to keep objects
> around, you have to have a reference on it at all times for it to exist
> in the filesystem.  If you drop it, then it too will be removed from
> debugfs.
> 
> And again, that's the only operation you can do on this object, there
> are no other methods for it other than "new" and "drop".
> 
> Now there are issues with implementation details here like:
>   - Do you want to keep the list of file and dir objects in the rust
>     structure to manually clean them up when we go to drop the
>     directory?
>   - Do we want to force all files to be dropped before the directory?
>     Or do we want to just let the C side clean it all up automagically
>     instead?
> and other things like that, but we can argue about that once we settle
> on the outward-facing api first.
> 
> I can live with this type of api.  It's simple and seems hard to abuse
> or get wrong from a user point-of-view, and should be pretty
> straight-forward on the binding side as well.  It will take a bit more
> work on the user when creating debugfs files than the C api did, but
> that's something that you want to impose on this api, not me :)
> 
> Sound reasonable?  Or am I missing something else here?

This does sound reasonable to me. I'm fine with always succeeding. I do
agree with Danilo that I would bias towards dir.file("foo_file"), but
it's not a super big deal to me.

I think the main thing I want to say here is that we should *not* force
files to be dropped before the directory, and we should also not force
child directories to be dropped before their parent directories. Trying
to impose that requirement on callers will make the API significantly
harder to use, and it's not worth it.

Alice

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

[Alice Ryhl]

On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > > If you really want to delete a debugfs file that you have created in the
> > > > past, then look it up and delete it with the call that is present for
> > > > that.
> > > 
> > > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > > i.e. we have a function that creates a thing and another function that removes
> > > the thing given a handle of the created thing. Whether the handle is valid is
> > > the responsibility of the caller.
> > > 
> > > In this case the handle would be the filename. For instance:
> > > 
> > > 	debugfs_create_file("foo", parent, ...);
> > > 	debugfs_remove(debugfs_lookup("foo", parent));
> > > 
> > > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > > typo in the filename. In this case we wouldn't even recognize it from the return
> > > value, since there is none.
> > > 
> > > In Rust, we do things differently, i.e. we wrap things into an object that
> > > cleans up itself, once it goes out of scope. For instance:
> > > 
> > > 	let file = debugfs::File::new("foo", parent);
> > > 
> > > Subsequently we store file in a structure that represents the time we want this
> > > file to exist. Once it goes out of scope, it's removed automatically.
> > > 
> > > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > > the filename or pass the wrong parent to clean things up.
> > > 
> > > Depending on whether the above was a misunderstanding and depending on how you
> > > think about it with this rationale, I have quite some more reasons why we don't
> > > want to have files / directories around in the filesystem without a
> > > corresponding object representation in Rust.
> > > 
> > > But before I write up a lot more text, I'd like to see if we're not already on
> > > the same page. :-)
> > 
> > Ok, let's knock up the api here first before worrying about the
> > implementation, as we seem to all be a bit confused as to what we want
> > to do.
> > 
> > Ideally, yes, I would like to NOT have any rust structure represent a
> > debugfs file, as that is the way the C api has been evolving.  We are
> > one step away from debugfs_create_file() being a void function that
> > doesn't return anything, and I don't want to see us go "backwards" here.
> > 
> > But the main reason I did that work was to keep people from trying to
> > determine if a file creation worked or not and to do different logic
> > based on that.  I think the Rust api CAN do both here, as you and Alice
> > have explained, so we should be ok.
> > 
> > So how about this for an example api, two structures, debugfs::Directory
> > and debugfs::File, which have only two types of operations that can be
> > done on them:
> > 
> > To create a debugfs directory, we do:
> > 
> > 	let my_dir = debugfs::Directory::new("foo_dir", parent);
> > 
> > There is no other Directory method, other than "drop", when the object
> > goes out of scope, which will clean up the directory and ALL child
> > objects at the same time (implementation details to be figured out
> > later.)
> > 
> > That will ALWAYS succeed no matter if the backend debugfs call failed or
> > not, and you have no way of knowing if it really did create something or
> > not.  That directory you can then use to pass into debugfs file or
> > directory creation functions.
> > 
> > Same for creating a debugfs file, you would do something like:
> > 
> > 	let my_file = debugfs::File::new("foo_file", my_dir);
> > 
> > depending on the "file type" you want to create, there will be different
> > new_* methods (new_u32, new_u64, etc.).
> > 
> > which also ALWAYS succeeds.  And again, as you want to keep objects
> > around, you have to have a reference on it at all times for it to exist
> > in the filesystem.  If you drop it, then it too will be removed from
> > debugfs.
> 
> Great! That's exactly what I think the API should look like as well. :-)
> 
> > And again, that's the only operation you can do on this object, there
> > are no other methods for it other than "new" and "drop".
> 
> We probably still want
> 
> 	let foo_dir = my_dir.subdir("foo_dir")
> 	foo_dir.file("foo_file")
> 
> for convinience, rather than having to call
> 
> 	let bar_dir = Dir::new("foo_dir", my_dir)
> 	File::new("bar_file", bar_dir)
> 
> all the time. But otherwise, sounds reasonable to me.
> 
> > Now there are issues with implementation details here like:
> >   - Do you want to keep the list of file and dir objects in the rust
> >     structure to manually clean them up when we go to drop the
> >     directory?
> >   - Do we want to force all files to be dropped before the directory?
> >     Or do we want to just let the C side clean it all up automagically
> >     instead?
> > and other things like that, but we can argue about that once we settle
> > on the outward-facing api first.
> 
> The only thing I don't want to is to allow to leak files or directories, i.e.
> 
> 	File::new("bar_file", bar_dir).leak()
> 
> which keeps the file in the filesystem, but takes away the handle from the Rust
> side, such that it won't be removed from the filesystem anymore when the handle
> goes out of scope, which can cause nasty bugs. But I think there isn't any
> benefit to allow this anyways and it isn't needed with reference counting.

Why not have leak() for files only? That way, the files still go away
when you drop the directory, and you don't have to keep around a bunch
of File objects in your Rust structs.

Alice

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

[Danilo Krummrich]

On Fri, May 23, 2025 at 05:09:13PM +0000, Alice Ryhl wrote:
> On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> > The only thing I don't want to is to allow to leak files or directories, i.e.
> > 
> > 	File::new("bar_file", bar_dir).leak()
> > 
> > which keeps the file in the filesystem, but takes away the handle from the Rust
> > side, such that it won't be removed from the filesystem anymore when the handle
> > goes out of scope, which can cause nasty bugs. But I think there isn't any
> > benefit to allow this anyways and it isn't needed with reference counting.
> 
> Why not have leak() for files only? That way, the files still go away
> when you drop the directory, and you don't have to keep around a bunch
> of File objects in your Rust structs.

In a previous mail I said that there are more reasons why we don't want to have
files (or directories) in the filesystem without an object representation in
Rust.

One of them is when attaching private data to a file, like we do with
debugfs_create_file().

When we do this, in most of the cases we want to share data between a driver
structure and the debugfs file (e.g. a GPU's VM to dump the virtual address
space layout).

And most likely we do this by passing an Arc<T> to dir.file(), to make the file
own a reference to the corresponding reference counted object (in C we just hope
that no one frees the object while the debugfs file holds a pointer to it).

The question is, what happens to this reference if we would subsequently call
file.leak()? We can't drop the Arc, because otherwise we risk a UAF in the
filesystem callback, but we can't keep it either because otherwise we *really*
leak this reference, even if the parent directory is removed eventually.

--

Now, one could say, what about attaching the file's private data to the directory
instead? (And I think this has been proposed already elsewhere.)

But I really don't like this approach, because it would mean that when creating
the directory we would necessarily need to know all the files we will ever
create in this directory *and* have all the corresponding private data
available at this point of time. But that would be an insane requirement.

For instance, let's assume a driver creates a directory 'clients', which for
every connected userspace "client" wants to provide a file with some metadata
for this client.

Sure, you can work around this somehow, e.g. by using a mutex protected Vec as
private data or by always creating a new directory first for dynamically created
debugfs entires. But this sounds pretty horrible to me.

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

[Alice Ryhl]

On Sat, May 24, 2025 at 02:25:27PM +0200, Danilo Krummrich wrote:
> On Fri, May 23, 2025 at 05:09:13PM +0000, Alice Ryhl wrote:
> > On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> > > The only thing I don't want to is to allow to leak files or directories, i.e.
> > > 
> > > 	File::new("bar_file", bar_dir).leak()
> > > 
> > > which keeps the file in the filesystem, but takes away the handle from the Rust
> > > side, such that it won't be removed from the filesystem anymore when the handle
> > > goes out of scope, which can cause nasty bugs. But I think there isn't any
> > > benefit to allow this anyways and it isn't needed with reference counting.
> > 
> > Why not have leak() for files only? That way, the files still go away
> > when you drop the directory, and you don't have to keep around a bunch
> > of File objects in your Rust structs.
> 
> In a previous mail I said that there are more reasons why we don't want to have
> files (or directories) in the filesystem without an object representation in
> Rust.
> 
> One of them is when attaching private data to a file, like we do with
> debugfs_create_file().
> 
> When we do this, in most of the cases we want to share data between a driver
> structure and the debugfs file (e.g. a GPU's VM to dump the virtual address
> space layout).
> 
> And most likely we do this by passing an Arc<T> to dir.file(), to make the file
> own a reference to the corresponding reference counted object (in C we just hope
> that no one frees the object while the debugfs file holds a pointer to it).
> 
> The question is, what happens to this reference if we would subsequently call
> file.leak()? We can't drop the Arc, because otherwise we risk a UAF in the
> filesystem callback, but we can't keep it either because otherwise we *really*
> leak this reference, even if the parent directory is removed eventually.
> 
> --
> 
> Now, one could say, what about attaching the file's private data to the directory
> instead? (And I think this has been proposed already elsewhere.)
> 
> But I really don't like this approach, because it would mean that when creating
> the directory we would necessarily need to know all the files we will ever
> create in this directory *and* have all the corresponding private data
> available at this point of time. But that would be an insane requirement.
> 
> For instance, let's assume a driver creates a directory 'clients', which for
> every connected userspace "client" wants to provide a file with some metadata
> for this client.
> 
> Sure, you can work around this somehow, e.g. by using a mutex protected Vec as
> private data or by always creating a new directory first for dynamically created
> debugfs entires. But this sounds pretty horrible to me.

I do agree that if we support file.leak(), then the private data should:

1. Be provided when creating the file, not the directory.
2. Be stored with the directory and be freed with it.
3. Not be encoded in the type of the directory.

Some sort of list with data to free when the directory is freed could
work. But it sounds like we should not consider file.leak() for the
initial API.

Alice

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

[Danilo Krummrich]

On Tue, May 27, 2025 at 11:38:02AM +0000, Alice Ryhl wrote:
> On Sat, May 24, 2025 at 02:25:27PM +0200, Danilo Krummrich wrote:
> > On Fri, May 23, 2025 at 05:09:13PM +0000, Alice Ryhl wrote:
> > > On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> > > > The only thing I don't want to is to allow to leak files or directories, i.e.
> > > > 
> > > > 	File::new("bar_file", bar_dir).leak()
> > > > 
> > > > which keeps the file in the filesystem, but takes away the handle from the Rust
> > > > side, such that it won't be removed from the filesystem anymore when the handle
> > > > goes out of scope, which can cause nasty bugs. But I think there isn't any
> > > > benefit to allow this anyways and it isn't needed with reference counting.
> > > 
> > > Why not have leak() for files only? That way, the files still go away
> > > when you drop the directory, and you don't have to keep around a bunch
> > > of File objects in your Rust structs.
> > 
> > In a previous mail I said that there are more reasons why we don't want to have
> > files (or directories) in the filesystem without an object representation in
> > Rust.
> > 
> > One of them is when attaching private data to a file, like we do with
> > debugfs_create_file().
> > 
> > When we do this, in most of the cases we want to share data between a driver
> > structure and the debugfs file (e.g. a GPU's VM to dump the virtual address
> > space layout).
> > 
> > And most likely we do this by passing an Arc<T> to dir.file(), to make the file
> > own a reference to the corresponding reference counted object (in C we just hope
> > that no one frees the object while the debugfs file holds a pointer to it).
> > 
> > The question is, what happens to this reference if we would subsequently call
> > file.leak()? We can't drop the Arc, because otherwise we risk a UAF in the
> > filesystem callback, but we can't keep it either because otherwise we *really*
> > leak this reference, even if the parent directory is removed eventually.
> > 
> > --
> > 
> > Now, one could say, what about attaching the file's private data to the directory
> > instead? (And I think this has been proposed already elsewhere.)
> > 
> > But I really don't like this approach, because it would mean that when creating
> > the directory we would necessarily need to know all the files we will ever
> > create in this directory *and* have all the corresponding private data
> > available at this point of time. But that would be an insane requirement.
> > 
> > For instance, let's assume a driver creates a directory 'clients', which for
> > every connected userspace "client" wants to provide a file with some metadata
> > for this client.
> > 
> > Sure, you can work around this somehow, e.g. by using a mutex protected Vec as
> > private data or by always creating a new directory first for dynamically created
> > debugfs entires. But this sounds pretty horrible to me.
> 
> I do agree that if we support file.leak(), then the private data should:
> 
> 1. Be provided when creating the file, not the directory.
> 2. Be stored with the directory and be freed with it.
> 3. Not be encoded in the type of the directory.
>
> Some sort of list with data to free when the directory is freed could
> work.

I think this would be problematic too.

Think of the client example I gave above: Let's assume a driver creates a
'client' directory for every bound device and we create a single file per client
connecting to the corresponding device exposed to userspace.

Let's assume clients do connect and disconnect frequently. If we'd allow
file.leak() in this case, it would mean that all the private data of the clients
that have ever been connected piles up in the 'client' directories's private
data storage until the 'client' directory is removed, which is when the device
is unbound, which might be never.

> But it sounds like we should not consider file.leak() for the
> initial API.

Yeah, let's not have file.leak() either.

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

[Matthew Maurer]

I'm going to try to rework the patch based on the feedback here, and
have a few things I want to check on to reduce cycling:

1. Every approach described in this thread will result in the creation
APIs (create a root dir, create a subdir, create a file) being
fallible (returns `Result<>`) because they use `Arc` in the
implementation which fundamentally allocates. I previously got the
feedback that the debugfs API should have no possibility of handleable
errors. This would *not* mean that we were signaling the errors out of
the raw debugfs bindings into Rust, but Rust would be able to detect
`Arc` allocation failures unless you really want me to be hiding
allocation failures (I could theoretically do this by generating a
dummy object on allocation failure, but this would add complexity and
seems questionable).
2. If we're not using borrowing to statically prevent entries from
outliving their parents, there are two possible semantics for what to
do when an entry *does* outlive its parent:
  a. The parent doesn't actually get `debugfs_remove` called until all
its child objects have left scope too. This can be accomplished by
just having a `_parent: Option<Arc<Entry>>` in each `Entry` structure.
  b. We get ghost objects, e.g. the child object is still around, but
it is somehow able to discover that its `dentry` pointer is no longer
valid and avoids trying to use it. The best solution I have to this is
to again maintain an optional parent `Arc`, but also have `Entry` hold
an additional refcount member. When you want to create a subdir, a
file in a directory, or delete any of those, you'd walk the spine of
parents, trying to claim the extra refcount on everything. Once you've
got it, you can safely delete yourself or create a subdirectory or
file. Then, whoever is last out of the extra refcount (either the
owning object or someone holding a reference while they perform an
operation on themselves) can try to clean up, again by claiming the
spine above them.
3. If we're not supporting `leak` or lifetime-based `File` anymore,
then I don't think my method of attaching data is needed anymore. It
was complex primarily in order to support statically enforced
lifetimes on dirs/subdirs/files, and several of the tricks I was using
don't work without a lifetime on the `File` type. Instead, I'll make
it so that `File` objects can attach a `ForeignOwnable`, with the
expected common cases being either `&'static MyData` or `Arc<MyData>`.

Unless I get additional feedback, I'm currently implementing 2-a,
using `ForeignOwnable` based attachments of data to files, and
returning `Result` where the only actual failure possibility is
allocation failure.