Support creating DebugFS directories and subdirectories. Similar to the
original DebugFS API, errors are hidden.
By default, when a root directory handle leaves scope, it will be cleaned
up.
Subdirectories will by default persist until their root directory is
cleaned up, but can be converted into a root directory if a scoped
lifecycle is desired.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
MAINTAINERS | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/kernel/debugfs.rs | 155 ++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
4 files changed, 158 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 906881b6c5cb6ff743e13b251873b89138c69a1c..a3b835e427b083a4ddd690d9e7739851f0af47ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7271,6 +7271,7 @@ F: include/linux/kobj*
F: include/linux/property.h
F: include/linux/sysfs.h
F: lib/kobj*
+F: rust/kernel/debugfs.rs
F: rust/kernel/device.rs
F: rust/kernel/device_id.rs
F: rust/kernel/devres.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8a2add69e5d66d1c2ebed9d2c950380e61c48842..787f928467faabd02a7f3cf041378fac856c4f89 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -13,6 +13,7 @@
#include <linux/blkdev.h>
#include <linux/cpumask.h>
#include <linux/cred.h>
+#include <linux/debugfs.h>
#include <linux/device/faux.h>
#include <linux/dma-mapping.h>
#include <linux/errname.h>
diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..b589c2d9a8d169bd66e98d2894261784e427230e
--- /dev/null
+++ b/rust/kernel/debugfs.rs
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! DebugFS Abstraction
+//!
+//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
+
+use crate::str::CStr;
+use core::mem::ManuallyDrop;
+use core::ops::Deref;
+
+/// Owning handle to a DebugFS directory.
+///
+/// This directory will be cleaned up when it goes out of scope.
+///
+/// # Invariants
+///
+/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
+#[repr(transparent)]
+pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
+
+// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
+// between threads.
+unsafe impl Send for Dir {}
+
+// SAFETY: All the native functions we re-export use interior locking, and the contents of the
+// struct are opaque to Rust.
+unsafe impl Sync for Dir {}
+
+impl Dir {
+ /// Create a new directory in DebugFS at the root.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// # use kernel::c_str;
+ /// # use kernel::debugfs::Dir;
+ /// {
+ /// let parent = Dir::new(c_str!("parent"));
+ /// // The path "parent" exists in DebugFS here.
+ /// }
+ /// // It does not exist here.
+ /// ```
+ pub fn new(name: &CStr) -> Self {
+ Self::create(name, None)
+ }
+
+ /// Create a DebugFS subdirectory.
+ ///
+ /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
+ /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// # use kernel::c_str;
+ /// # use kernel::debugfs::Dir;
+ /// {
+ /// let parent = Dir::new(c_str!("parent"));
+ /// // The path "parent" exists in DebugFS here.
+ /// {
+ /// let child = parent.subdir(c_str!("child"));
+ /// // The path "parent/child" exists in DebugFS here.
+ /// }
+ /// // The path "parent/child" still exists.
+ /// {
+ /// let child2 = Dir::from(parent.subdir(c_str!("child2")));
+ /// // The path "parent/child2" exists in DebugFS here.
+ /// }
+ /// // The path "parent/child2" is gone.
+ /// }
+ /// // None of the paths exist here.
+ /// ```
+ pub fn subdir(&self, name: &CStr) -> SubDir {
+ SubDir::new(Self::create(name, Some(self)))
+ }
+
+ /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
+ #[cfg(CONFIG_DEBUG_FS)]
+ fn create(name: &CStr, parent: Option<&Self>) -> Self {
+ let parent_ptr = match parent {
+ Some(parent) => parent.as_ptr(),
+ None => core::ptr::null_mut(),
+ };
+ // SAFETY:
+ // * `name` argument points to a NUL-terminated string that lives across the call, by
+ // invariants of `&CStr`.
+ // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
+ // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
+ // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
+ // so we can call `Self::from_ptr`.
+ unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
+ }
+
+ #[cfg(not(CONFIG_DEBUG_FS))]
+ fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
+ Self()
+ }
+
+ /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
+ ///
+ /// # Safety
+ ///
+ /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
+ /// live DebugFS directory.
+ #[cfg(CONFIG_DEBUG_FS)]
+ unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
+ Self(ptr)
+ }
+
+ /// Returns the pointer representation of the DebugFS directory.
+ ///
+ /// Due to the type invariant, the value returned from this function will always be an error
+ /// code, NUL, or a live DebugFS directory.
+ // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
+ #[cfg(CONFIG_DEBUG_FS)]
+ fn as_ptr(&self) -> *mut bindings::dentry {
+ self.0
+ }
+}
+
+impl Drop for Dir {
+ fn drop(&mut self) {
+ // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
+ // `as_ptr` guarantees that the pointer is of this form.
+ #[cfg(CONFIG_DEBUG_FS)]
+ unsafe {
+ bindings::debugfs_remove(self.as_ptr())
+ }
+ }
+}
+
+/// Handle to a DebugFS directory that will stay alive after leaving scope.
+#[repr(transparent)]
+pub struct SubDir(ManuallyDrop<Dir>);
+
+impl Deref for SubDir {
+ type Target = Dir;
+ fn deref(&self) -> &Dir {
+ &self.0
+ }
+}
+
+impl From<SubDir> for Dir {
+ fn from(subdir: SubDir) -> Self {
+ ManuallyDrop::into_inner(subdir.0)
+ }
+}
+
+impl SubDir {
+ fn new(dir: Dir) -> Self {
+ Self(ManuallyDrop::new(dir))
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c3762e80b314316b4b0cee3bfd9442f8f0510b91..86f6055b828d5f711578293d8916a517f2436977 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -45,6 +45,7 @@
#[doc(hidden)]
pub mod build_assert;
pub mod cred;
+pub mod debugfs;
pub mod device;
pub mod device_id;
pub mod devres;
--
2.49.0.906.g1f30a19c02-goog> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..b589c2d9a8d169bd66e98d2894261784e427230e
> --- /dev/null
> +++ b/rust/kernel/debugfs.rs
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Google LLC.
> +
> +//! DebugFS Abstraction
> +//!
> +//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
> +
> +use crate::str::CStr;
> +use core::mem::ManuallyDrop;
> +use core::ops::Deref;
> +
> +/// Owning handle to a DebugFS directory.
> +///
> +/// This directory will be cleaned up when it goes out of scope.
We should also document that it's a unit struct when `CONFIG_DEBUG_FS`
is disabled (and the operations are noops). Maybe even do something
like:
#[cfg_attr(CONFIG_DEBUG_FS, doc = "`CONFIG_DEBUG_FS=y`")]
#[cfg_attr(not(CONFIG_DEBUG_FS), doc = "`CONFIG_DEBUG_FS=n`")]
> +///
> +/// # Invariants
> +///
> +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> +#[repr(transparent)]
> +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
> +
> +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
> +// between threads.
> +unsafe impl Send for Dir {}
> +
> +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> +// struct are opaque to Rust.
> +unsafe impl Sync for Dir {}
> +
> +impl Dir {
> + /// Create a new directory in DebugFS at the root.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use kernel::c_str;
> + /// # use kernel::debugfs::Dir;
> + /// {
> + /// let parent = Dir::new(c_str!("parent"));
> + /// // The path "parent" exists in DebugFS here.
> + /// }
> + /// // It does not exist here.
> + /// ```
> + pub fn new(name: &CStr) -> Self {
> + Self::create(name, None)
> + }
> +
> + /// Create a DebugFS subdirectory.
> + ///
> + /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> + /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
But it will be cleaned up when the parent goes out of scope? We should
also mention that.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use kernel::c_str;
> + /// # use kernel::debugfs::Dir;
> + /// {
> + /// let parent = Dir::new(c_str!("parent"));
> + /// // The path "parent" exists in DebugFS here.
> + /// {
> + /// let child = parent.subdir(c_str!("child"));
> + /// // The path "parent/child" exists in DebugFS here.
> + /// }
> + /// // The path "parent/child" still exists.
> + /// {
> + /// let child2 = Dir::from(parent.subdir(c_str!("child2")));
> + /// // The path "parent/child2" exists in DebugFS here.
> + /// }
> + /// // The path "parent/child2" is gone.
> + /// }
> + /// // None of the paths exist here.
> + /// ```
> + pub fn subdir(&self, name: &CStr) -> SubDir {
> + SubDir::new(Self::create(name, Some(self)))
> + }
> +
> + /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> + #[cfg(CONFIG_DEBUG_FS)]
> + fn create(name: &CStr, parent: Option<&Self>) -> Self {
> + let parent_ptr = match parent {
> + Some(parent) => parent.as_ptr(),
> + None => core::ptr::null_mut(),
> + };
> + // SAFETY:
> + // * `name` argument points to a NUL-terminated string that lives across the call, by
> + // invariants of `&CStr`.
> + // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> + // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> + // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> + // so we can call `Self::from_ptr`.
> + unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
What about when an error got returned? Should that be exposed to the
user?
> + }
> +
> + #[cfg(not(CONFIG_DEBUG_FS))]
> + fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> + Self()
> + }
> +
> +impl Drop for Dir {
> + fn drop(&mut self) {
> + // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> + // `as_ptr` guarantees that the pointer is of this form.
> + #[cfg(CONFIG_DEBUG_FS)]
> + unsafe {
I feel a bit uneasy with seeing `cfg` on `unsafe` code, since now the
correctness also depends on the configuration. Someone might add/modify
it making it incorrect under certain configurations.
This case is pretty straight forward, but I'm not so sure if we already
have such a case.
How about having two modules providing the two implementations and then
just conditionally import one or the other?
---
Cheers,
Benno
> + bindings::debugfs_remove(self.as_ptr())
> + }
> + }
> +}
On Fri, May 02, 2025 at 10:12:15AM +0200, Benno Lossin wrote:
> > + /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + fn create(name: &CStr, parent: Option<&Self>) -> Self {
> > + let parent_ptr = match parent {
> > + Some(parent) => parent.as_ptr(),
> > + None => core::ptr::null_mut(),
> > + };
> > + // SAFETY:
> > + // * `name` argument points to a NUL-terminated string that lives across the call, by
> > + // invariants of `&CStr`.
> > + // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> > + // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> > + // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > + // so we can call `Self::from_ptr`.
> > + unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
>
> What about when an error got returned? Should that be exposed to the
> user?
No, not at all. See my comments on version 1 of this patchset. No
error should ever go back to the caller, it should never know if a
debugfs call succeeded or not so that it can just keep moving forward
and not act any differently.
Many of the C debugfs apis are already changed to be this way, let's not
go backwards and add this logic to the rust code only to rip it out in
the future.
> > + }
> > +
> > + #[cfg(not(CONFIG_DEBUG_FS))]
> > + fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> > + Self()
> > + }
> > +
>
> > +impl Drop for Dir {
> > + fn drop(&mut self) {
> > + // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> > + // `as_ptr` guarantees that the pointer is of this form.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + unsafe {
>
> I feel a bit uneasy with seeing `cfg` on `unsafe` code, since now the
> correctness also depends on the configuration. Someone might add/modify
> it making it incorrect under certain configurations.
The option is either enabled or not, this should be fine.
> This case is pretty straight forward, but I'm not so sure if we already
> have such a case.
>
> How about having two modules providing the two implementations and then
> just conditionally import one or the other?
That would require a lot more duplicated code that you then have to
always keep in sync. And from past experience, that's hard to do over
time. So let's do it this way if at all possible.
thanks,
greg k-h
On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> +/// Owning handle to a DebugFS directory.
> +///
> +/// This directory will be cleaned up when it goes out of scope.
> +///
> +/// # Invariants
> +///
> +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> +#[repr(transparent)]
> +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
Should probably use Opaque instead of a raw pointer.
> +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
[`Dir`]
> +// between threads.
> +unsafe impl Send for Dir {}
> +
> +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> +// struct are opaque to Rust.
> +unsafe impl Sync for Dir {}
> +
> +impl Dir {
> + /// Create a new directory in DebugFS at the root.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use kernel::c_str;
> + /// # use kernel::debugfs::Dir;
> + /// {
> + /// let parent = Dir::new(c_str!("parent"));
> + /// // The path "parent" exists in DebugFS here.
> + /// }
> + /// // It does not exist here.
This ready like an explanation for scopes; I think we should drop those comments
and the scope.
> + /// ```
> + pub fn new(name: &CStr) -> Self {
> + Self::create(name, None)
> + }
> +
> + /// Create a DebugFS subdirectory.
> + ///
> + /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> + /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use kernel::c_str;
> + /// # use kernel::debugfs::Dir;
> + /// {
> + /// let parent = Dir::new(c_str!("parent"));
> + /// // The path "parent" exists in DebugFS here.
> + /// {
> + /// let child = parent.subdir(c_str!("child"));
> + /// // The path "parent/child" exists in DebugFS here.
> + /// }
> + /// // The path "parent/child" still exists.
> + /// {
> + /// let child2 = Dir::from(parent.subdir(c_str!("child2")));
> + /// // The path "parent/child2" exists in DebugFS here.
> + /// }
> + /// // The path "parent/child2" is gone.
> + /// }
> + /// // None of the paths exist here.
I think the fact that you need all those comments here proves that it's not
really intuitive. Please see me comment on SubDir below.
> + /// ```
> + pub fn subdir(&self, name: &CStr) -> SubDir {
> + SubDir::new(Self::create(name, Some(self)))
> + }
> +
> + /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> + #[cfg(CONFIG_DEBUG_FS)]
> + fn create(name: &CStr, parent: Option<&Self>) -> Self {
> + let parent_ptr = match parent {
> + Some(parent) => parent.as_ptr(),
> + None => core::ptr::null_mut(),
> + };
> + // SAFETY:
> + // * `name` argument points to a NUL-terminated string that lives across the call, by
> + // invariants of `&CStr`.
> + // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> + // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> + // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> + // so we can call `Self::from_ptr`.
> + unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
Please split up in two calls, such that we don't have two unsafe function calls
in a single unsafe block.
> + }
> +
> + #[cfg(not(CONFIG_DEBUG_FS))]
> + fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> + Self()
> + }
> +
> + /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
> + ///
> + /// # Safety
> + ///
> + /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
> + /// live DebugFS directory.
> + #[cfg(CONFIG_DEBUG_FS)]
> + unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
> + Self(ptr)
> + }
> +
> + /// Returns the pointer representation of the DebugFS directory.
> + ///
> + /// Due to the type invariant, the value returned from this function will always be an error
> + /// code, NUL, or a live DebugFS directory.
Maybe put this in a '# Guarantees' section.
> + // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
I think you mean ERR_PTR(ENODEV).
> + #[cfg(CONFIG_DEBUG_FS)]
> + fn as_ptr(&self) -> *mut bindings::dentry {
> + self.0
> + }
> +}
> +
> +impl Drop for Dir {
> + fn drop(&mut self) {
> + // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> + // `as_ptr` guarantees that the pointer is of this form.
> + #[cfg(CONFIG_DEBUG_FS)]
> + unsafe {
> + bindings::debugfs_remove(self.as_ptr())
> + }
> + }
> +}
> +
> +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> +#[repr(transparent)]
> +pub struct SubDir(ManuallyDrop<Dir>);
I think it's not very intuitive if the default is that a SubDir still exists
after it has been dropped. I think your first approach being explicit about this
with keep() consuming the SubDir was much better; please keep this approach.
On Thu, May 1, 2025 at 11:37 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > +/// Owning handle to a DebugFS directory.
> > +///
> > +/// This directory will be cleaned up when it goes out of scope.
> > +///
> > +/// # Invariants
> > +///
> > +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> > +#[repr(transparent)]
> > +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
>
> Should probably use Opaque instead of a raw pointer.
Opaque usage is problematic here:
1. It was explicitly requested in the first patch that I not expose
the reference counted nature of the dentry we were being provided. At
the same time, the dentry we are provided isn't a borrow - it doesn't
live for a fixed lifetime, it goes away when we clean it up. This
means that if I do `Dir(Opaque<dentry>)`, there's no way to represent
the return value of directory creation - it's not `Box<Dir>` or
`Arc<Dir>`, and I've been requested to hide the fact that it's
`ARef<Dir>` and not decrement the refcount myself. We can't know the
lifetime at the callsite, so we can't use a `&'a Dir` either. This
lands with the raw pointer wrapper. I could, technically, produce
`InnerDir(Opaque<dentry>)` and `Dir(ManuallyDrop<ARef<Dir>>)`, and
then impl `Drop` for `Dir` to `remove` instead of `dput`, but that
seems like a bunch of work to construct infrastructure and then
suppress it that doesn't actually help.
2. If you were suggesting `Dir(*mut Opaque<dentry>)`, I think this is
meaningless and will just cause cast noise. My understanding is that
the use of `Opaque` is to remove restrictions Rust has over legal
references and values, e.g. it's initialized, it's not being mutated,
thread safety invariants, etc. However, pointers in Rust explicitly do
not induce any of these requirements on their pointee (otherwise
`ptr::dangling()` would be immediate UB in many situations).
3. Unless we produce an `ARef<Opaque<dentry>>` at some point (which
again, we've been asked to pretend isn't there), I don't think there's
any way for the DebugFS bindings to produce a legal `&'a
Opaque<dentry>`, so there's not really a purpose.
>
> > +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
>
> [`Dir`]
>
> > +// between threads.
> > +unsafe impl Send for Dir {}
> > +
> > +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> > +// struct are opaque to Rust.
> > +unsafe impl Sync for Dir {}
> > +
> > +impl Dir {
> > + /// Create a new directory in DebugFS at the root.
> > + ///
> > + /// # Examples
> > + ///
> > + /// ```
> > + /// # use kernel::c_str;
> > + /// # use kernel::debugfs::Dir;
> > + /// {
> > + /// let parent = Dir::new(c_str!("parent"));
> > + /// // The path "parent" exists in DebugFS here.
> > + /// }
> > + /// // It does not exist here.
>
> This ready like an explanation for scopes; I think we should drop those comments
> and the scope.
>
> > + /// ```
> > + pub fn new(name: &CStr) -> Self {
> > + Self::create(name, None)
> > + }
> > +
> > + /// Create a DebugFS subdirectory.
> > + ///
> > + /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> > + /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
> > + ///
> > + /// # Examples
> > + ///
> > + /// ```
> > + /// # use kernel::c_str;
> > + /// # use kernel::debugfs::Dir;
> > + /// {
> > + /// let parent = Dir::new(c_str!("parent"));
> > + /// // The path "parent" exists in DebugFS here.
> > + /// {
> > + /// let child = parent.subdir(c_str!("child"));
> > + /// // The path "parent/child" exists in DebugFS here.
> > + /// }
> > + /// // The path "parent/child" still exists.
> > + /// {
> > + /// let child2 = Dir::from(parent.subdir(c_str!("child2")));
> > + /// // The path "parent/child2" exists in DebugFS here.
> > + /// }
> > + /// // The path "parent/child2" is gone.
> > + /// }
> > + /// // None of the paths exist here.
>
> I think the fact that you need all those comments here proves that it's not
> really intuitive. Please see me comment on SubDir below.
>
> > + /// ```
> > + pub fn subdir(&self, name: &CStr) -> SubDir {
> > + SubDir::new(Self::create(name, Some(self)))
> > + }
> > +
> > + /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + fn create(name: &CStr, parent: Option<&Self>) -> Self {
> > + let parent_ptr = match parent {
> > + Some(parent) => parent.as_ptr(),
> > + None => core::ptr::null_mut(),
> > + };
> > + // SAFETY:
> > + // * `name` argument points to a NUL-terminated string that lives across the call, by
> > + // invariants of `&CStr`.
> > + // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> > + // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> > + // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > + // so we can call `Self::from_ptr`.
> > + unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
>
> Please split up in two calls, such that we don't have two unsafe function calls
> in a single unsafe block.
>
> > + }
> > +
> > + #[cfg(not(CONFIG_DEBUG_FS))]
> > + fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> > + Self()
> > + }
> > +
> > + /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
> > + /// live DebugFS directory.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
> > + Self(ptr)
> > + }
> > +
> > + /// Returns the pointer representation of the DebugFS directory.
> > + ///
> > + /// Due to the type invariant, the value returned from this function will always be an error
> > + /// code, NUL, or a live DebugFS directory.
>
> Maybe put this in a '# Guarantees' section.
>
> > + // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
>
> I think you mean ERR_PTR(ENODEV).
>
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + fn as_ptr(&self) -> *mut bindings::dentry {
> > + self.0
> > + }
> > +}
> > +
> > +impl Drop for Dir {
> > + fn drop(&mut self) {
> > + // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> > + // `as_ptr` guarantees that the pointer is of this form.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + unsafe {
> > + bindings::debugfs_remove(self.as_ptr())
> > + }
> > + }
> > +}
> > +
> > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > +#[repr(transparent)]
> > +pub struct SubDir(ManuallyDrop<Dir>);
>
> I think it's not very intuitive if the default is that a SubDir still exists
> after it has been dropped. I think your first approach being explicit about this
> with keep() consuming the SubDir was much better; please keep this approach.
On Fri, May 02, 2025 at 08:48:36AM -0700, Matthew Maurer wrote: > On Thu, May 1, 2025 at 11:37 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote: > > > +/// Owning handle to a DebugFS directory. > > > +/// > > > +/// This directory will be cleaned up when it goes out of scope. > > > +/// > > > +/// # Invariants > > > +/// > > > +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`. > > > +#[repr(transparent)] > > > +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry); > > > > Should probably use Opaque instead of a raw pointer. > > Opaque usage is problematic here: Yes, when I wrote this I was back at your v1 in my mind -- the raw pointer is fine..
On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > +/// Owning handle to a DebugFS directory.
> > +///
> > +/// This directory will be cleaned up when it goes out of scope.
> > +///
> > +/// # Invariants
> > +///
> > +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> > +#[repr(transparent)]
> > +pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
>
> Should probably use Opaque instead of a raw pointer.
>
> > +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
>
> [`Dir`]
>
> > +// between threads.
> > +unsafe impl Send for Dir {}
> > +
> > +// SAFETY: All the native functions we re-export use interior locking, and the contents of the
> > +// struct are opaque to Rust.
> > +unsafe impl Sync for Dir {}
> > +
> > +impl Dir {
> > + /// Create a new directory in DebugFS at the root.
> > + ///
> > + /// # Examples
> > + ///
> > + /// ```
> > + /// # use kernel::c_str;
> > + /// # use kernel::debugfs::Dir;
> > + /// {
> > + /// let parent = Dir::new(c_str!("parent"));
> > + /// // The path "parent" exists in DebugFS here.
> > + /// }
> > + /// // It does not exist here.
>
> This ready like an explanation for scopes; I think we should drop those comments
> and the scope.
>
> > + /// ```
> > + pub fn new(name: &CStr) -> Self {
> > + Self::create(name, None)
> > + }
> > +
> > + /// Create a DebugFS subdirectory.
> > + ///
> > + /// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope.
> > + /// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
> > + ///
> > + /// # Examples
> > + ///
> > + /// ```
> > + /// # use kernel::c_str;
> > + /// # use kernel::debugfs::Dir;
> > + /// {
> > + /// let parent = Dir::new(c_str!("parent"));
> > + /// // The path "parent" exists in DebugFS here.
> > + /// {
> > + /// let child = parent.subdir(c_str!("child"));
> > + /// // The path "parent/child" exists in DebugFS here.
> > + /// }
> > + /// // The path "parent/child" still exists.
> > + /// {
> > + /// let child2 = Dir::from(parent.subdir(c_str!("child2")));
> > + /// // The path "parent/child2" exists in DebugFS here.
> > + /// }
> > + /// // The path "parent/child2" is gone.
> > + /// }
> > + /// // None of the paths exist here.
>
> I think the fact that you need all those comments here proves that it's not
> really intuitive. Please see me comment on SubDir below.
>
> > + /// ```
> > + pub fn subdir(&self, name: &CStr) -> SubDir {
> > + SubDir::new(Self::create(name, Some(self)))
> > + }
> > +
> > + /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + fn create(name: &CStr, parent: Option<&Self>) -> Self {
> > + let parent_ptr = match parent {
> > + Some(parent) => parent.as_ptr(),
> > + None => core::ptr::null_mut(),
> > + };
> > + // SAFETY:
> > + // * `name` argument points to a NUL-terminated string that lives across the call, by
> > + // invariants of `&CStr`.
> > + // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> > + // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> > + // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > + // so we can call `Self::from_ptr`.
> > + unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
>
> Please split up in two calls, such that we don't have two unsafe function calls
> in a single unsafe block.
>
> > + }
> > +
> > + #[cfg(not(CONFIG_DEBUG_FS))]
> > + fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> > + Self()
> > + }
> > +
> > + /// Constructs a new DebugFS [`Dir`] from the underlying pointer.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
> > + /// live DebugFS directory.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
> > + Self(ptr)
> > + }
> > +
> > + /// Returns the pointer representation of the DebugFS directory.
> > + ///
> > + /// Due to the type invariant, the value returned from this function will always be an error
> > + /// code, NUL, or a live DebugFS directory.
>
> Maybe put this in a '# Guarantees' section.
>
> > + // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
>
> I think you mean ERR_PTR(ENODEV).
>
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + fn as_ptr(&self) -> *mut bindings::dentry {
> > + self.0
> > + }
> > +}
> > +
> > +impl Drop for Dir {
> > + fn drop(&mut self) {
> > + // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> > + // `as_ptr` guarantees that the pointer is of this form.
> > + #[cfg(CONFIG_DEBUG_FS)]
> > + unsafe {
> > + bindings::debugfs_remove(self.as_ptr())
> > + }
> > + }
> > +}
> > +
> > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > +#[repr(transparent)]
> > +pub struct SubDir(ManuallyDrop<Dir>);
>
> I think it's not very intuitive if the default is that a SubDir still exists
> after it has been dropped. I think your first approach being explicit about this
> with keep() consuming the SubDir was much better; please keep this approach.
Wait, let's step back. Why do we care about the difference between a
"subdir" and a "dir"? They both are the same thing, and how do you
describe a subdir of a subdir? :)
Why the "split" here, that just adds additional mental energy to both
someone using the api, as well as someone having to review someone using
it. A directory is a directory, no matter where in debugfs it lives, so
let's just keep it as simple as possible please.
thanks,
greg k-h
On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote: > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote: > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote: > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope. > > > +#[repr(transparent)] > > > +pub struct SubDir(ManuallyDrop<Dir>); > > > > I think it's not very intuitive if the default is that a SubDir still exists > > after it has been dropped. I think your first approach being explicit about this > > with keep() consuming the SubDir was much better; please keep this approach. > > Wait, let's step back. Why do we care about the difference between a > "subdir" and a "dir"? They both are the same thing, and how do you > describe a subdir of a subdir? :) We care about the difference, because Dir originally had keep() which drops the Dir instance without actually removing it. For subdirs this is fine, since they'll be cleaned up when the parent is removed. However, we don't want users to be able to call keep() on the directory that has been created first, since if that's done we loose our root anchor to ever free the tree, which almost always would be a bug. Please also see [1] and subsequent replies. A subdir of a subdir would still be a SubDir. [1] https://lore.kernel.org/lkml/aBNKEewhCP8jRIZL@pollux/
On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote: > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote: > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote: > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote: > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope. > > > > +#[repr(transparent)] > > > > +pub struct SubDir(ManuallyDrop<Dir>); > > > > > > I think it's not very intuitive if the default is that a SubDir still exists > > > after it has been dropped. I think your first approach being explicit about this > > > with keep() consuming the SubDir was much better; please keep this approach. > > > > Wait, let's step back. Why do we care about the difference between a > > "subdir" and a "dir"? They both are the same thing, and how do you > > describe a subdir of a subdir? :) > > We care about the difference, because Dir originally had keep() which drops the > Dir instance without actually removing it. For subdirs this is fine, since > they'll be cleaned up when the parent is removed. But does that mean a subdir can not be cleaned up without dropping the parent first? For many subsystems, they make a "root" debugfs directory, and then add/remove subdirs all the time within that. > However, we don't want users to be able to call keep() on the directory that has > been created first, since if that's done we loose our root anchor to ever free > the tree, which almost always would be a bug. Then do a call to debugfs_lookup_and_remove() which is what I really recommend doing for any C user anyway. That way no dentry is ever "stored" anywhere. Anyway, if Dir always has an implicit keep() call in it, then I guess this is ok. Let's see how this shakes out with some real-world users. We can always change it over time if it gets unwieldy. thanks, greg k-h
On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote: > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote: > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote: > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote: > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote: > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope. > > > > > +#[repr(transparent)] > > > > > +pub struct SubDir(ManuallyDrop<Dir>); > > > > > > > > I think it's not very intuitive if the default is that a SubDir still exists > > > > after it has been dropped. I think your first approach being explicit about this > > > > with keep() consuming the SubDir was much better; please keep this approach. > > > > > > Wait, let's step back. Why do we care about the difference between a > > > "subdir" and a "dir"? They both are the same thing, and how do you > > > describe a subdir of a subdir? :) > > > > We care about the difference, because Dir originally had keep() which drops the > > Dir instance without actually removing it. For subdirs this is fine, since > > they'll be cleaned up when the parent is removed. > > But does that mean a subdir can not be cleaned up without dropping the > parent first? For many subsystems, they make a "root" debugfs > directory, and then add/remove subdirs all the time within that. In the following I will call the first top level directory created by a module / driver "root". The logic I propose is that "root" is of type Dir, which means there is no keep() and if dropped the whole tree under root is removed. A subdir created under a Dir is of type SubDir and has the keep() method and if called consumes the type instance and subsequently can only ever be removed by dropping root. Alternatively a SubDir can be converted into a Dir, and hence don't has keep() anymore and if dropped will be removed. So, the result is that we still can add / remove subdirs as we want. The advantage is that we don't have keep() for root, which would be a dedicated API for driver / modules to create bugs. If a driver / module would call keep() on the root, it would not only mean that we leak the root directory, but also all subdirs and files that we called keep() on. This becomes even more problematic if we start attaching data to files. Think of an Arc that is attached to a file, which keeps driver data alive just because we leaked the root. > > However, we don't want users to be able to call keep() on the directory that has > > been created first, since if that's done we loose our root anchor to ever free > > the tree, which almost always would be a bug. > > Then do a call to debugfs_lookup_and_remove() which is what I really > recommend doing for any C user anyway. That way no dentry is ever > "stored" anywhere. > > Anyway, if Dir always has an implicit keep() call in it, then I guess > this is ok. Let's see how this shakes out with some real-world users. > We can always change it over time if it gets unwieldy. I really advise against it, Rust allows us to model such subtile differences properly (and easily) with the type system to avoid bugs. Let's take advantage of that. Using debugfs_lookup_and_remove() wouldn't change anything, since we want to attach the lifetime of a directory to a corresponding object. (Otherwise we're back to where we are with C, i.e. the user has to remember to call the correct thing at the correct time, rather than letting the type system take care instead.) So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer vs. CString.
On Fri, May 02, 2025 at 09:33:15AM +0200, Danilo Krummrich wrote: > On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote: > > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote: > > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote: > > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote: > > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote: > > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope. > > > > > > +#[repr(transparent)] > > > > > > +pub struct SubDir(ManuallyDrop<Dir>); > > > > > > > > > > I think it's not very intuitive if the default is that a SubDir still exists > > > > > after it has been dropped. I think your first approach being explicit about this > > > > > with keep() consuming the SubDir was much better; please keep this approach. > > > > > > > > Wait, let's step back. Why do we care about the difference between a > > > > "subdir" and a "dir"? They both are the same thing, and how do you > > > > describe a subdir of a subdir? :) > > > > > > We care about the difference, because Dir originally had keep() which drops the > > > Dir instance without actually removing it. For subdirs this is fine, since > > > they'll be cleaned up when the parent is removed. > > > > But does that mean a subdir can not be cleaned up without dropping the > > parent first? For many subsystems, they make a "root" debugfs > > directory, and then add/remove subdirs all the time within that. > > In the following I will call the first top level directory created by a module / > driver "root". > > The logic I propose is that "root" is of type Dir, which means there is no > keep() and if dropped the whole tree under root is removed. > > A subdir created under a Dir is of type SubDir and has the keep() method and if > called consumes the type instance and subsequently can only ever be removed by > dropping root. > > Alternatively a SubDir can be converted into a Dir, and hence don't has keep() > anymore and if dropped will be removed. > > So, the result is that we still can add / remove subdirs as we want. > > The advantage is that we don't have keep() for root, which would be a dedicated > API for driver / modules to create bugs. If a driver / module would call keep() > on the root, it would not only mean that we leak the root directory, but also > all subdirs and files that we called keep() on. > > This becomes even more problematic if we start attaching data to files. Think of > an Arc that is attached to a file, which keeps driver data alive just because we > leaked the root. Ok, fair enough, let's try it this way, without keep() > > > However, we don't want users to be able to call keep() on the directory that has > > > been created first, since if that's done we loose our root anchor to ever free > > > the tree, which almost always would be a bug. > > > > Then do a call to debugfs_lookup_and_remove() which is what I really > > recommend doing for any C user anyway. That way no dentry is ever > > "stored" anywhere. > > > > Anyway, if Dir always has an implicit keep() call in it, then I guess > > this is ok. Let's see how this shakes out with some real-world users. > > We can always change it over time if it gets unwieldy. > > I really advise against it, Rust allows us to model such subtile differences > properly (and easily) with the type system to avoid bugs. Let's take advantage > of that. > > Using debugfs_lookup_and_remove() wouldn't change anything, since we want to > attach the lifetime of a directory to a corresponding object. > > (Otherwise we're back to where we are with C, i.e. the user has to remember to > call the correct thing at the correct time, rather than letting the type system > take care instead.) > > So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from > Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer > vs. CString. Ok, that's fine, and it gives me an idea of how I can fix up the C api over time to get rid of exporting the dentry entirely :) thanks, greg k-h
On Fri, May 2, 2025 at 4:55 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 02, 2025 at 09:33:15AM +0200, Danilo Krummrich wrote:
> > On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote:
> > > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote:
> > > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote:
> > > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope.
> > > > > > > +#[repr(transparent)]
> > > > > > > +pub struct SubDir(ManuallyDrop<Dir>);
> > > > > >
> > > > > > I think it's not very intuitive if the default is that a SubDir still exists
> > > > > > after it has been dropped. I think your first approach being explicit about this
> > > > > > with keep() consuming the SubDir was much better; please keep this approach.
> > > > >
> > > > > Wait, let's step back. Why do we care about the difference between a
> > > > > "subdir" and a "dir"? They both are the same thing, and how do you
> > > > > describe a subdir of a subdir? :)
> > > >
> > > > We care about the difference, because Dir originally had keep() which drops the
> > > > Dir instance without actually removing it. For subdirs this is fine, since
> > > > they'll be cleaned up when the parent is removed.
> > >
> > > But does that mean a subdir can not be cleaned up without dropping the
> > > parent first? For many subsystems, they make a "root" debugfs
> > > directory, and then add/remove subdirs all the time within that.
> >
> > In the following I will call the first top level directory created by a module /
> > driver "root".
> >
> > The logic I propose is that "root" is of type Dir, which means there is no
> > keep() and if dropped the whole tree under root is removed.
> >
> > A subdir created under a Dir is of type SubDir and has the keep() method and if
> > called consumes the type instance and subsequently can only ever be removed by
> > dropping root.
> >
> > Alternatively a SubDir can be converted into a Dir, and hence don't has keep()
> > anymore and if dropped will be removed.
> >
> > So, the result is that we still can add / remove subdirs as we want.
> >
> > The advantage is that we don't have keep() for root, which would be a dedicated
> > API for driver / modules to create bugs. If a driver / module would call keep()
> > on the root, it would not only mean that we leak the root directory, but also
> > all subdirs and files that we called keep() on.
> >
> > This becomes even more problematic if we start attaching data to files. Think of
> > an Arc that is attached to a file, which keeps driver data alive just because we
> > leaked the root.
>
> Ok, fair enough, let's try it this way, without keep()
>
> > > > However, we don't want users to be able to call keep() on the directory that has
> > > > been created first, since if that's done we loose our root anchor to ever free
> > > > the tree, which almost always would be a bug.
> > >
> > > Then do a call to debugfs_lookup_and_remove() which is what I really
> > > recommend doing for any C user anyway. That way no dentry is ever
> > > "stored" anywhere.
> > >
> > > Anyway, if Dir always has an implicit keep() call in it, then I guess
> > > this is ok. Let's see how this shakes out with some real-world users.
> > > We can always change it over time if it gets unwieldy.
> >
> > I really advise against it, Rust allows us to model such subtile differences
> > properly (and easily) with the type system to avoid bugs. Let's take advantage
> > of that.
> >
> > Using debugfs_lookup_and_remove() wouldn't change anything, since we want to
> > attach the lifetime of a directory to a corresponding object.
> >
> > (Otherwise we're back to where we are with C, i.e. the user has to remember to
> > call the correct thing at the correct time, rather than letting the type system
> > take care instead.)
> >
> > So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from
> > Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer
> > vs. CString.
>
> Ok, that's fine, and it gives me an idea of how I can fix up the C api
> over time to get rid of exporting the dentry entirely :)
I've figured out that the SubDir API (and indeed, all patchsets after
v1) have a flaw in them, so I'm wondering if your rework will resolve
this.
If I do:
```
let dir = Dir::new(c_str!("foo"));
let subdir = dir.subdir(c_str!("bar"));
drop(dir);
// This next line will Oops because subdir is already removed
drop(Dir::from(subdir));
```
Simply removing `Dir::from` from the API won't resolve this - as long
as `SubDir` has any method, accessing it after its parent has been
cleaned up with `remove` will result in an oops.
The options I can see given the existing API are:
A. SubDir needs to be inherently limited to a borrow of another Dir or
SubDir. This will still break if someone uses
`debugfs_lookup_and_remove()` in an antisocial fashion, but we haven't
bound this in Rust, and we don't need to be robust against bad
behavior from C. If we do this, the promotability of subdirectories
back to directories would go away entirely,
B. I can leverage `dget`/`dput` to make it so that the directory I get
back from the APIs are actually owning, and so the dentry will not be
released while the Dir/SubDir exists. I understand this to be
undesired, but putting it out there, since it'd make things safe even
to `debugfs_lookup_and_remove()`.
C. I can limit use of `debugfs_remove` to the builder API (exists in
V1, no longer exists in this version, will be a separate patch soon
for reference). This is the behavior that I had written in V1 before I
started trying to adapt to requests. This means that the subdirectory
handles will remain valid because the non-builder handles will be
`dput` rather than recursively removed. Builder handles can't be
smuggled out due to an existential lifetime. Again, I understand this
is not desired, I'm just trying to lay out all ways to prevent this
class of error.
On Fri, May 02, 2025 at 09:33:21AM +0200, Danilo Krummrich wrote: > On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote: > > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote: > > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote: > > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote: > > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote: > > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope. > > > > > > +#[repr(transparent)] > > > > > > +pub struct SubDir(ManuallyDrop<Dir>); > > > > > > > > > > I think it's not very intuitive if the default is that a SubDir still exists > > > > > after it has been dropped. I think your first approach being explicit about this > > > > > with keep() consuming the SubDir was much better; please keep this approach. > > > > > > > > Wait, let's step back. Why do we care about the difference between a > > > > "subdir" and a "dir"? They both are the same thing, and how do you > > > > describe a subdir of a subdir? :) > > > > > > We care about the difference, because Dir originally had keep() which drops the > > > Dir instance without actually removing it. For subdirs this is fine, since > > > they'll be cleaned up when the parent is removed. > > > > But does that mean a subdir can not be cleaned up without dropping the > > parent first? For many subsystems, they make a "root" debugfs > > directory, and then add/remove subdirs all the time within that. > > In the following I will call the first top level directory created by a module / > driver "root". > > The logic I propose is that "root" is of type Dir, which means there is no > keep() and if dropped the whole tree under root is removed. > > A subdir created under a Dir is of type SubDir and has the keep() method and if > called consumes the type instance and subsequently can only ever be removed by > dropping root. > > Alternatively a SubDir can be converted into a Dir, and hence don't has keep() > anymore and if dropped will be removed. > > So, the result is that we still can add / remove subdirs as we want. > > The advantage is that we don't have keep() for root, which would be a dedicated > API for driver / modules to create bugs. If a driver / module would call keep() > on the root, it would not only mean that we leak the root directory, but also > all subdirs and files that we called keep() on. > > This becomes even more problematic if we start attaching data to files. Think of > an Arc that is attached to a file, which keeps driver data alive just because we > leaked the root. Forgot to mention, this Arc could contain vtables into the (driver) module after the module has been removed already, which could be called into if reading from / writing to a corresponding (leaked) debugfs file. I really think Dir::keep() is an invitation for potentially horrible bugs. If we really don't want SubDir, then let's not have keep() at all. > > > However, we don't want users to be able to call keep() on the directory that has > > > been created first, since if that's done we loose our root anchor to ever free > > > the tree, which almost always would be a bug. > > > > Then do a call to debugfs_lookup_and_remove() which is what I really > > recommend doing for any C user anyway. That way no dentry is ever > > "stored" anywhere. > > > > Anyway, if Dir always has an implicit keep() call in it, then I guess > > this is ok. Let's see how this shakes out with some real-world users. > > We can always change it over time if it gets unwieldy. > > I really advise against it, Rust allows us to model such subtile differences > properly (and easily) with the type system to avoid bugs. Let's take advantage > of that. > > Using debugfs_lookup_and_remove() wouldn't change anything, since we want to > attach the lifetime of a directory to a corresponding object. > > (Otherwise we're back to where we are with C, i.e. the user has to remember to > call the correct thing at the correct time, rather than letting the type system > take care instead.) > > So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from > Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer > vs. CString.
© 2016 - 2026 Red Hat, Inc.