[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aBR2u75eNQ4erBBD@pollux>
Date: Fri, 2 May 2025 09:39:39 +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: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.
Powered by blists - more mailing lists