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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aC8uNmrLUSS8sxHU@google.com>
Date: Thu, 22 May 2025 14:01:26 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Benno Lossin <lossin@...nel.org>, 
	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>, 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 v5 4/4] rust: samples: Add debugfs sample

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.

But I also see your point. I don't think keep() is a critical part of
what I think the approach should be.

Alice