rust: DebugFS Bindings

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

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

Quoted 21 lines · Matthew Maurer

rust/kernel/debugfs.rs new file +6 -0

+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

rust/kernel/debugfs.rs new file +13 -0

+    /// 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 {

rust/kernel/debugfs.rs line 47

Quoted 3 lines

rust/kernel/debugfs.rs new file +3 -0

+            _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?

Quoted 19 lines

rust/kernel/debugfs.rs new file +18 -0

+    /// 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.
+    ///

rust/kernel/debugfs.rs line 138

Quoted 10 lines

rust/kernel/debugfs.rs new file +10 -0

+    /// # 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.

rust/kernel/debugfs.rs new file +16 -0

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

rust/kernel/debugfs.rs new file +6 -0

+    ///
+    /// Subdirectory handles cannot outlive the directory handle they were created from.
+    ///
+    /// # Examples
+    ///
+    /// ```

(quoted diff) +1 -0

+}

rust/kernel/lib.rs +1 -0

index c3762e80b314316b4b0cee3bfd9442f8f0510b91..86f6055b828d5f711578293d8916a517f2436977 100644
@@ -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;

Quoted 22 lines · Matthew Maurer

rust/kernel/debugfs.rs new file +22 -0

+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 {

rust/kernel/debugfs.rs line 111

Quoted 4 lines

rust/kernel/debugfs.rs new file +4 -0

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

Quoted 23 lines · Benno Lossin

Quoted 22 lines · Matthew Maurer

rust/kernel/debugfs.rs new file +22 -0

+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 {

Quoted 8 lines

rust/kernel/debugfs.rs line 111

Quoted 4 lines

rust/kernel/debugfs.rs new file +4 -0

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

Quoted 1 line

What if I give a parent?

Same thing, it will fail.

Quoted 2 lines

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.

Quoted 2 lines

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

Find does not happen.

Quoted 2 lines

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 :)

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

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

+    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,

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 <[email protected]>

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

rust/kernel/debugfs.rs +0 -0

index ed1aba6d700d064dbfd7e923dbcbf80b9acf5361..4a138717bd0fdb320033d07446a192c9f520a17b 100644
@@ -46,6 +47,19 @@ unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
         }
     }

rust/kernel/debugfs.rs +34 -0

+            _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(),

rust/kernel/debugfs.rs +19 -0

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

rust/kernel/debugfs.rs +11 -0

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

samples/rust/rust_debugfs.rs new file +15 -0

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

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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.

Quoted 17 lines · Benno Lossin

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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.

Quoted 29 lines · Danilo Krummrich

Quoted 17 lines · Benno Lossin

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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.

Quoted 7 lines

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.

Quoted 4 lines

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

Quoted 51 lines · Benno Lossin

Quoted 29 lines · Danilo Krummrich

Quoted 17 lines · Benno Lossin

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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.

Quoted 7 lines

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!

Quoted 7 lines

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.

Quoted 7 lines

[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).

The docs on keep should definitely warn about leaks.

Quoted 51 lines · Benno Lossin

Quoted 29 lines · Danilo Krummrich

Quoted 17 lines · Benno Lossin

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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.

Quoted 7 lines

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!

Quoted 7 lines

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.

Quoted 7 lines

[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).

Quoted 12 lines

Quoted 5 lines

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

Quoted 29 lines · Danilo Krummrich

Quoted 17 lines · Benno Lossin

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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.

Quoted 7 lines

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.

Quoted 4 lines

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

Quoted 5 lines · Benno Lossin

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.

Quoted 10 lines · Matthew Maurer

Quoted 5 lines · Benno Lossin

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/

Quoted 15 lines · Danilo Krummrich

Quoted 10 lines · Matthew Maurer

Quoted 5 lines · Benno Lossin

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]:

Quoted 8 lines

"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.

Quoted 5 lines · Matthew Maurer

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/[email protected]/

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()?

Quoted 23 lines · Timur Tabi

Quoted 5 lines · Matthew Maurer

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/[email protected]/

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.

Quoted 2 lines

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.

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/[email protected]/
[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

Quoted 17 lines · Benno Lossin

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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

Quoted 20 lines · Danilo Krummrich

Quoted 17 lines · Benno Lossin

samples/rust/rust_debugfs.rs line 38

Quoted 10 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +10 -0

+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.

Quoted 5 lines

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.

Quoted 4 lines

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).

Quoted 7 lines · Danilo Krummrich

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.

Quoted 16 lines · Matthew Maurer

Quoted 7 lines · Danilo Krummrich

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/

Quoted 5 lines

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?

Quoted 23 lines · Benno Lossin

Quoted 16 lines · Matthew Maurer

Quoted 7 lines · Danilo Krummrich

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/

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 32 lines · Greg Kroah-Hartman

Quoted 23 lines · Benno Lossin

Quoted 16 lines · Matthew Maurer

Quoted 7 lines · Danilo Krummrich

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/

Quoted 5 lines

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.

Quoted 4 lines

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.)

Quoted 1 line

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.

Quoted 23 lines · Benno Lossin

Quoted 16 lines · Matthew Maurer

Quoted 7 lines · Danilo Krummrich

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/

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 29 lines · Greg Kroah-Hartman

Quoted 23 lines · Benno Lossin

Quoted 16 lines · Matthew Maurer

Quoted 7 lines · Danilo Krummrich

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/

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 4 lines

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?

Quoted 42 lines · Alice Ryhl

Quoted 29 lines · Greg Kroah-Hartman

Quoted 23 lines · Benno Lossin

Quoted 16 lines · Matthew Maurer

Quoted 7 lines · Danilo Krummrich

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/

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 4 lines

* 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.

Quoted 3 lines

* 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.

Quoted 8 lines

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

Quoted 29 lines · Greg Kroah-Hartman

Quoted 23 lines · Benno Lossin

Quoted 16 lines · Matthew Maurer

Quoted 7 lines · Danilo Krummrich

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/

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 4 lines

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

Quoted 16 lines · Alice Ryhl

Quoted 10 lines · Greg Kroah-Hartman

Quoted 4 lines · Benno Lossin

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.

Quoted 7 lines

* 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?

Quoted 3 lines

* 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).

Quoted 9 lines · Danilo Krummrich

Quoted 4 lines · Alice Ryhl

* 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.

Quoted 2 lines

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.

Quoted 24 lines

Quoted 3 lines

* 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.

Quoted 7 lines

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.

Quoted 10 lines · Alice Ryhl

Quoted 9 lines · Danilo Krummrich

Quoted 4 lines · Alice Ryhl

* 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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 15 lines

Quoted 7 lines

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.

Quoted 7 lines

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).

Quoted 4 lines

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.

Quoted 32 lines · Alice Ryhl

Quoted 24 lines

Quoted 3 lines

* 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.

Quoted 7 lines

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).

Quoted 4 lines

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.

Quoted 51 lines · Danilo Krummrich

Quoted 32 lines · Alice Ryhl

Quoted 24 lines

Quoted 3 lines

* 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.

Quoted 7 lines

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).

Quoted 4 lines

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.)

Quoted 2 lines

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.

Quoted 13 lines

Quoted 13 lines

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.)

Quoted 7 lines

Quoted 2 lines

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.

Quoted 11 lines · Greg Kroah-Hartman

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

Quoted 26 lines · Greg Kroah-Hartman

Quoted 13 lines

Quoted 13 lines

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.)

Quoted 7 lines

Quoted 2 lines

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 :)

Quoted 6 lines

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.

Quoted 52 lines · Alice Ryhl

Quoted 51 lines · Danilo Krummrich

Quoted 32 lines · Alice Ryhl

Quoted 24 lines

Quoted 3 lines

* 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.

Quoted 7 lines

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).

Quoted 4 lines

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.)

Quoted 7 lines

Quoted 2 lines

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.

Quoted 61 lines · Greg Kroah-Hartman

Quoted 52 lines · Alice Ryhl

Quoted 51 lines · Danilo Krummrich

Quoted 32 lines · Alice Ryhl

Quoted 24 lines

Quoted 3 lines

* 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.

Quoted 7 lines

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).

Quoted 4 lines

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.)

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 3 lines

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. :-)

Quoted 30 lines · Danilo Krummrich

Quoted 3 lines · Greg Kroah-Hartman

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.

Quoted 7 lines

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?

Quoted 75 lines · Greg Kroah-Hartman

Quoted 30 lines · Danilo Krummrich

Quoted 3 lines · Greg Kroah-Hartman

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.

Quoted 7 lines

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);

Quoted 7 lines

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. :-)

Quoted 2 lines

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.

Quoted 9 lines

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.

Quoted 75 lines · Greg Kroah-Hartman

Quoted 30 lines · Danilo Krummrich

Quoted 3 lines · Greg Kroah-Hartman

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.

Quoted 7 lines

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);

Quoted 7 lines

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. :-)

Quoted 2 lines

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.

Quoted 9 lines

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.

Quoted 4 lines · Danilo Krummrich

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()

Quoted 4 lines

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.

Quoted 5 lines · Alice Ryhl

Quoted 4 lines · Danilo Krummrich

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()

Quoted 8 lines

Quoted 4 lines

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.

Quoted 45 lines · Danilo Krummrich

Quoted 5 lines · Alice Ryhl

Quoted 4 lines · Danilo Krummrich

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()

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 75 lines · Danilo Krummrich

Quoted 61 lines · Greg Kroah-Hartman

Quoted 52 lines · Alice Ryhl

Quoted 51 lines · Danilo Krummrich

Quoted 32 lines · Alice Ryhl

Quoted 24 lines

Quoted 3 lines

* 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.

Quoted 7 lines

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).

Quoted 4 lines

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.)

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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:

Quoted 29 lines

Quoted 3 lines

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.

Quoted 7 lines

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?

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/[email protected]