linux-kernel - Re: [PATCH v4 1/4] rust: debugfs: Bind DebugFS directory creation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aBYNyqTRlpGAJVuB@polis>
Date: Sat, 3 May 2025 14:36:26 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Matthew Maurer <mmaurer@...gle.com>
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 v4 1/4] rust: debugfs: Bind DebugFS directory creation

On Fri, May 02, 2025 at 07:49:30PM +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<'a, const KEEP: bool = false> {

Why did you move to a const generic, rather than a new type? What's the
advantage? AFAICS, it just makes it less obvious to see from the type itself how
it will behave. Reading Dir<true> doesn't make it obvious what it does.

While I prefer a new type over the const generic, I'm fine with it. But I think
we should use something more descriptive than a bool. Please see
device::DeviceContext for reference.

> +    /// Create a DebugFS subdirectory.
> +    ///
> +    /// This returns a [`Dir<'_, true>`], 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::owning`].
> +    ///
> +    /// Regardless of conversion, 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, true> {
> +        Dir::create(name, Some(self))
> +    }

The default should be that the directory is removed when the Dir instance is
dropped.

The common case (which people expect) is that an object is cleaned up on drop().

> +impl<'a> Dir<'a, true> {
> +    /// Upgrade a non-owning directory to one which will be removed on drop.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let debugfs = Dir::new(c_str!("parent"));
> +    /// let subdir = debugfs.subdir(c_str!("child"));
> +    /// // If subdir were dropped, the directory would not be removed.
> +    /// let owned_subdir = subdir.owning();
> +    /// // If owned_subdir is dropped, "child" will be removed.
> +    /// ```
> +    pub fn owning(self) -> Dir<'a, false> {
> +        Dir {
> +            dir: self.dir,
> +            _phantom: self._phantom,
> +        }
> +    }

As mentioned above, please make it the other way around.