linux-kernel - Re: [PATCH v3 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: <aBR1O6d6YBszgVlU@pollux>
Date: Fri, 2 May 2025 09:33:15 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: 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>,
	"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 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.