Message-ID: <CAGSQo00hYLubqJy9zkO3_O3b6V15WtmoSybC+Ep5uJeU6k3t4A@mail.gmail.com>
Date: Fri, 2 May 2025 08:48:36 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: 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>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Sami Tolvanen <samitolvanen@...gle.com>, Timur Tabi <ttabi@...dia.com>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

On Thu, May 1, 2025 at 11:37 PM Danilo Krummrich <dakr@...nel.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.

Powered by blists - more mailing lists