Message-ID: <CAGSQo00GPnpkzY12udM8QNZpA+Nh+OrNJDH7zd1nVZPrQfOb4A@mail.gmail.com>
Date: Fri, 2 May 2025 09:13:56 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Danilo Krummrich <dakr@...nel.org>, 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 2, 2025 at 4:55 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Fri, May 02, 2025 at 09:33:15AM +0200, Danilo Krummrich wrote:
> > 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.
>
> Ok, fair enough, let's try it this way, without keep()
>
> > > > 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.
>
> Ok, that's fine, and it gives me an idea of how I can fix up the C api
> over time to get rid of exporting the dentry entirely :)

I've figured out that the SubDir API (and indeed, all patchsets after
v1) have a flaw in them, so I'm wondering if your rework will resolve
this.

If I do:
```
let dir = Dir::new(c_str!("foo"));
let subdir = dir.subdir(c_str!("bar"));
drop(dir);
// This next line will Oops because subdir is already removed
drop(Dir::from(subdir));
```

Simply removing `Dir::from` from the API won't resolve this - as long
as `SubDir` has any method, accessing it after its parent has been
cleaned up with `remove` will result in an oops.

The options I can see given the existing API are:
A. SubDir needs to be inherently limited to a borrow of another Dir or
SubDir. This will still break if someone uses
`debugfs_lookup_and_remove()` in an antisocial fashion, but we haven't
bound this in Rust, and we don't need to be robust against bad
behavior from C. If we do this, the promotability of subdirectories
back to directories would go away entirely,
B. I can leverage `dget`/`dput` to make it so that the directory I get
back from the APIs are actually owning, and so the dentry will not be
released while the Dir/SubDir exists. I understand this to be
undesired, but putting it out there, since it'd make things safe even
to `debugfs_lookup_and_remove()`.
C. I can limit use of `debugfs_remove` to the builder API (exists in
V1, no longer exists in this version, will be a separate patch soon
for reference). This is the behavior that I had written in V1 before I
started trying to adapt to requests. This means that the subdirectory
handles will remain valid because the non-builder handles will be
`dput` rather than recursively removed. Builder handles can't be
smuggled out due to an existential lifetime. Again, I understand this
is not desired, I'm just trying to lay out all ways to prevent this
class of error.

Powered by blists - more mailing lists