Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Danilo Krummrich <dakr@kernel.org>]

On Fri, May 02, 2025 at 09:33:21AM +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.

Forgot to mention, this Arc could contain vtables into the (driver) module after
the module has been removed already, which could be called into if reading
from / writing to a corresponding (leaked) debugfs file.

I really think Dir::keep() is an invitation for potentially horrible bugs.

If we really don't want SubDir, then let's not have keep() at all.

> > > 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.