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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > * 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.
> 
> I mean, for cases such as this, I could imagine that you use `keep()` on
> the files stored inside of the driver directory, but don't use it on the
> directory. That way, you only have to keep a single reference to an
> entire directory around, which may be more convenient.

No, sorry, but debugfs files are "create and forget" type of things.
The caller has NO reference back to the file at all in the C version,
let's not add that functionality back to the rust side after I spent a
long time removing it from the C code :)

If you really want to delete a debugfs file that you have created in the
past, then look it up and delete it with the call that is present for
that.

The only thing I think that might be worth "keeping" in some form, as an
object reference as discussed, is a debugfs directory.

thanks,

greg k-h