Re: [PATCH v5 4/4] rust: samples: Add debugfs sample

[Danilo Krummrich <dakr@kernel.org>]

On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> > On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > > * If you remove a directory before the removing objects inside it, then
> > >   the Rust objects become "ghost" objects that are still usable, but not
> > >   visible in the file system anywhere. I.e. calling methods on them
> > >   succeed but are no-ops.
> > 
> > If we would want to keep an entry alive as long as there are more leaves, we'd
> > obviously need to reference count things.
> > 
> > But what do we need reference counting for with this logic? Shouldn't this be
> > also possible without?
> 
> Well, my understanding is that when you remove the parent directory, the
> dentries for child directories and files are freed, so trying to use the
> Rust objects that correspond to those child dentries would be a UAF. I
> want to refcount those child entries so that they at least remain valid
> memory even if they're no longer visible in the file system.

Yes, that makes sense.

Instead of using the dentry pointer as a handle, we could also use the entry's
path and do a lookup whenever the entry is used. Not saying this is better
though.

> > > * Possibly we have a way to drop a Rust object without removing it from
> > >   the file system. In this case, it can never be accessed from Rust
> > >   again, and the only way to remove it is to drop its parent directory.
> > 
> > I'm not sure about this one.
> > 
> > IIUC, this basically brings back the "keep() logic", which I still think is a
> > footgun and should be avoided.
> > 
> > Also, we only needed this, since due to the borrowing design we couldn't store
> > parent and child nodes in the same structure. With reference counting (or the
> > logic above) this goes away.
> > 
> > I wrote the following in a previous conversation [1].
> > 
> > --
> > 
> > What I see more likely to happen is a situation where the "root" directory
> > (almost) lives forever, and hence subsequent calls, such as
> > 
> > 	root.subdir("foo").keep()
> > 
> > effectively are leaks.
> > 
> > One specific example for that would be usb_debug_root, which is created in the
> > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > 
> > The same is true for other cases where the debugfs "root" is created in the
> > module scope, but subsequent entries are created by driver instances. If a
> > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > device is unplugged (or unbound in general).
> 
> Where is the leak? If the file is still visible in the file system, then
> it's not a leak to still have the memory. If the file got removed by
> removing its parent, then my intent is that this should free the memory
> of the child object.

Well, take the case I described above, where the debugfs "root" is created in
the module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively the file / directory
(and subsequently also the corresponding memory) everytime a device is unplugged
(or unbound in general)."

If the module is built-in the directory from the module scope is *never*
removed, but the entries the driver e.g. creates in probe() for a particular
device with keep() will pile up endlessly, especially for hot-pluggable devices.

(It's getting even worse when there's data bound to such a leaked file, that
might even contain a vtable that is entered from any of the fops of the file.)

That'd be clearly a bug, but for the driver author calling keep() seems like a
valid thing to do -- to me that's clearly a built-in footgun.