Message-Id: <D9LIUW74XIRW.2DUQCSLYX1JP4@kernel.org>
Date: Fri, 02 May 2025 10:12:15 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Matthew Maurer" <mmaurer@...gle.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 "Sami Tolvanen" <samitolvanen@...gle.com>, "Timur Tabi" <ttabi@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

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

Powered by blists - more mailing lists