On Wed, Dec 17, 2025 at 5:40 PM Timur Tabi <[email protected]> wrote:
>
> Add a new constructor for Dir that uses the debugfs_lookup() API to
> obtain a reference to an existing debugfs directory entry.
>
> The key difference from Dir::new() and Dir::subdir() is the cleanup
> semantics: when a Dir obtained via lookup() is dropped, it calls
> dput() to release the reference rather than debugfs_remove() which
> would delete the directory.
>
> To implement this cleanup distinction, the Entry class now includes
> an is_lookup boolean that specifies how the entry was created and
> therefore how it should be dropped.
>
> Signed-off-by: Timur Tabi <[email protected]>
> ---
> rust/kernel/debugfs.rs | 43 +++++++++++++++++++++++++++++++
> rust/kernel/debugfs/entry.rs | 49 +++++++++++++++++++++++++++++++++---
> 2 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index facad81e8290..eee799f64f79 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -110,6 +110,49 @@ pub fn new(name: &CStr) -> Self {
> Dir::create(name, None)
> }
>
> + /// Looks up an existing directory in DebugFS.
> + ///
> + /// If `parent` is [`None`], the lookup is performed from the root of
> the debugfs filesystem.
> + ///
> + /// Returns [`Some(Dir)`] representing the looked-up directory if found,
> or [`None`] if the
> + /// directory does not exist or if debugfs is not enabled. When dropped,
> the [`Dir`] will
> + /// release its reference to the dentry without removing the directory
> from the filesystem.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use kernel::c_str;
> + /// # use kernel::debugfs::Dir;
> + /// // Look up a top-level directory
> + /// let nova_core = Dir::lookup(c_str!("nova_core"), None);
> + ///
> + /// // Look up a subdirectory within a parent
> + /// let parent = Dir::new(c_str!("parent"));
> + /// let child = parent.subdir(c_str!("child"));
> + /// let looked_up = Dir::lookup(c_str!("child"), Some(&parent));
> + /// // `looked_up` now refers to the same directory as `child`.
> + /// // Dropping `looked_up` will not remove the directory.
> + /// ```
This also breaks the assumption people were able to have with `Dir`
previously that if they have a `Dir` and debugfs is enabled, it's
usable for creating subdirs/files.
This might be considered an issue, but at minimum it needs to be documented.
> + pub fn lookup(name: &CStr, parent: Option<&Dir>) -> Option<Self> {
We were explicitly asked by Greg *not* to expose error conditions in
directory constructors. I would expect that to extend to `lookup` as
well, so this would return a `Self`, not allowing the caller to find
out if the file was actually there. This might be a bit of a grey area
as the comment he cited on `debugfs_create_file` and friends has
explicit verbiage about it being bad form to check errors here and
`debugfs_lookup` mentions explicitly a null return for the file not
being there.
Another point in favor of just returning `Self` rather than
`Option<Self>` is that in your proposed real-world usage of this, you
create a dummy directory in the `None` case.
> + #[cfg(CONFIG_DEBUG_FS)]
> + {
We shouldn't need to track a parent entry here, and so shouldn't need
to do the whole cloning dance.
> + let parent_entry = match parent {
> + // If the parent couldn't be allocated, just early-return
> + Some(Dir(None)) => return None,
> + Some(Dir(Some(entry))) => Some(entry.clone()),
> + None => None,
> + };
> + let entry = Entry::lookup(name, parent_entry)?;
There's no guarantee that this lookup has returned a directory - a
lookup can return files too afaict
> + Some(Self(
> + // If Arc creation fails, the `Entry` will be dropped, so
> the reference will be
> + // released.
> + Arc::new(entry, GFP_KERNEL).ok(),
> + ))
> + }
> + #[cfg(not(CONFIG_DEBUG_FS))]
> + None
> + }
> +
> /// Creates a subdirectory within this directory.
> ///
> /// # Examples
> diff --git a/rust/kernel/debugfs/entry.rs b/rust/kernel/debugfs/entry.rs
> index 706cb7f73d6c..455d7bbb8036 100644
> --- a/rust/kernel/debugfs/entry.rs
> +++ b/rust/kernel/debugfs/entry.rs
> @@ -18,6 +18,9 @@ pub(crate) struct Entry<'a> {
> _parent: Option<Arc<Entry<'static>>>,
> // If we were created with a non-owning parent, this prevents us from
> outliving it
> _phantom: PhantomData<&'a ()>,
> + // If true, this entry was obtained via debugfs_lookup and should be
> released
> + // with dput() instead of debugfs_remove().
> + is_lookup: bool,
I agree with Danilo - I think this would be cleaner as a different
type (or at least an enum variant) rather than adding another field
(if we need it). Notably:
1. `_parent` is not required for a lookup entry - the lookup doesn't
need to keep it alive, because it's not using the "If I have the
directory handle, I can create real files in it" semantics we had
before.
2. The `_phantom` is irrelevant for a lookup entry because they can't be scoped
3. The drop behavior is different (which you're gating with `is_lookup`).
Basically the only part that is the same is the presence of a dentry
pointer under the hood.
> }
>
> // SAFETY: [`Entry`] is just a `dentry` under the hood, which the API
> promises can be transferred
> @@ -43,9 +46,38 @@ pub(crate) fn dynamic_dir(name: &CStr, parent:
> Option<Arc<Self>>) -> Self {
> entry,
> _parent: parent,
> _phantom: PhantomData,
> + is_lookup: false,
> }
> }
>
> + /// Looks up an existing entry in debugfs.
> + ///
> + /// Returns [`Some(Entry)`] representing the looked-up dentry if the
> entry exists, or [`None`]
> + /// if the entry was not found. When dropped, the entry will release its
> reference via `dput()`
> + /// instead of removing the directory.
> + pub(crate) fn lookup(name: &CStr, parent: Option<Arc<Self>>) ->
> Option<Self> {
> + let parent_ptr = match &parent {
> + Some(entry) => entry.as_ptr(),
> + None => core::ptr::null_mut(),
> + };
> + // SAFETY: The invariants of this function's arguments ensure the
> safety of this call.
> + // * `name` is a valid C string by the invariants of `&CStr`.
> + // * `parent_ptr` is either `NULL` (if `parent` is `None`), or a
> pointer to a valid
> + // `dentry` by our invariant. `debugfs_lookup` handles `NULL`
> pointers correctly.
> + let entry = unsafe { bindings::debugfs_lookup(name.as_char_ptr(),
> parent_ptr) };
> +
> + if entry.is_null() {
> + return None;
> + }
> +
> + Some(Entry {
> + entry,
> + _parent: parent,
> + _phantom: PhantomData,
> + is_lookup: true,
> + })
> + }
> +
> /// # Safety
> ///
> /// * `data` must outlive the returned `Entry`.
> @@ -76,6 +108,7 @@ pub(crate) unsafe fn dynamic_file<T>(
> entry,
> _parent: Some(parent),
> _phantom: PhantomData,
> + is_lookup: false,
> }
> }
> }
> @@ -97,6 +130,7 @@ pub(crate) fn dir(name: &CStr, parent: Option<&'a
> Entry<'_>>) -> Self {
> entry,
> _parent: None,
> _phantom: PhantomData,
> + is_lookup: false,
> }
> }
>
> @@ -129,6 +163,7 @@ pub(crate) fn file<T>(
> entry,
> _parent: None,
> _phantom: PhantomData,
> + is_lookup: false,
> }
> }
> }
> @@ -140,6 +175,7 @@ pub(crate) fn empty() -> Self {
> entry: core::ptr::null_mut(),
> _parent: None,
> _phantom: PhantomData,
> + is_lookup: false,
> }
> }
>
> @@ -157,8 +193,15 @@ pub(crate) fn as_ptr(&self) -> *mut bindings::dentry {
>
> impl Drop for Entry<'_> {
> fn drop(&mut self) {
> - // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal
> DebugFS dentries.
> - // `as_ptr` guarantees that the pointer is of this form.
> - unsafe { bindings::debugfs_remove(self.as_ptr()) }
> + if self.is_lookup {
> + // SAFETY: `dput` can take `NULL` and legal dentries.
> + // `as_ptr` guarantees that the pointer is of this form.
> + // This entry was obtained via `debugfs_lookup`, so we release
> the reference.
> + unsafe { bindings::dput(self.as_ptr()) }
> + } else {
> + // SAFETY: `debugfs_remove` can take `NULL`, error values, and
> legal DebugFS dentries.
> + // `as_ptr` guarantees that the pointer is of this form.
> + unsafe { bindings::debugfs_remove(self.as_ptr()) }
> + }
> }
> }
> --
> 2.52.0
>
>
