Message-Id: <D9VVOENW6H8P.32D4SGCFJ0LJU@kernel.org>
Date: Wed, 14 May 2025 14:21:41 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.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>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>, "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 Wed May 14, 2025 at 1:24 PM CEST, Danilo Krummrich wrote:
> On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
>> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
>> >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
>> >> > +impl kernel::Module for RustDebugFs {
>> >> > +    fn init(_this: &'static ThisModule) -> Result<Self> {
>> >> > +        // Create a debugfs directory in the root of the filesystem called "sample_debugfs".
>> >> > +        let debugfs = Dir::new(c_str!("sample_debugfs"));
>> >> > +
>> >> > +        {
>> >> > +            // Create a subdirectory, so "sample_debugfs/subdir" now exists.
>> >> > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
>> >> > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
>> >> > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
>> >> 
>> >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
>> >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
>> >> support of the wrapped type. But in this case, `Dir` is sometimes
>> >> intended to not be dropped, so I'd rather have a `.keep()` function I
>> >> saw mentioned somewhere.
>> >
>> > I agree, if we really want to "officially" support to forget() (sub-)directories
>> > and files in order to rely on the recursive cleanup of the "root" directory, it
>> > should be covered explicitly by the API. I.e. (sub-)directories and files should
>> > have some kind of keep() and / or forget() method, to make it clear that this is
>> > supported and by design and won't lead to any leaks.
>> >
>> > Consequently, this would mean that we'd need something like your proposed const
>> > generic on the Dir type, such that keep() / forget() cannot be called on the
>> > "root" directory.
>> >
>> > However, I really think we should keep the code as it is in this version and
>> > just don't provide an example that utilizes ManuallyDrop and forget().
>> >
>> > I don't see how the idea of "manually dropping" (sub-)directories and files
>> > provides any real value compared to just storing their instance in a driver
>> > structure as long as they should stay alive, which is much more intuitive
>> > anyways.
>> 
>> Yeah that's whats normally done in Rust anyways. But I think that
>> lifetimes bite us in this case:
>> 
>>     let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
>> 
>>     let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
>>     // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
>> 
>>     /* code for creating the file etc */
>> 
>>     Ok(Self { _debugfs: debugfs, _sub: sub })
>>     // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
>> 
>> This code won't compile, since we can't store the "root" dir in the same
>> struct that we want to store the subdir, because the subdir borrows from
>> the root dir.
>> 
>> Essentially this would require self-referential structs like the
>> `ouroboros` crate [1] from user-space Rust. We should rather have the
>> `.keep()` function in the API than use self-referential structs.
>
> Fair enough -- I think we should properly document those limitations, recommend
> using keep() for those cases and ensure that we can't call keep() on the "root"
> directory then.
>
> Unless, we can find a better solution, which, unfortunately, I can't think of
> one. The only thing I can think of is to reference count (parent) directories,
> which would be contrary to how the C API works and not desirable.

Yeah, I also don't have an idea, but if I find something, I'll let you
know.

>> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
>> 
>> Another problem that only affects complicated debugfs structures is that
>> you would have to store all subdirs & files somewhere. If the structure
>> is dynamic and changes over the lifetime of the driver, then you'll need
>> a `Vec` or store the dirs in `Arc` or similar, leading to extra
>> allocations.
>
> If it changes dynamically then it's pretty likely that we do not only want to
> add entries dynamically, but also remove them, which implies that we need to be
> able to drop them. So, I don't think that's a problem.

Yeah that's true.

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

Hmm that is unfortunate. But I don't see a problem with having:

    static USB_DEBUGFS: Dir<'static> = ...; // or some on-demand init process

Then users can store subdir that also is `Dir<'static>` and just borrow
the USB_DEBUGFS for `'static`.

The docs on `keep` should definitely warn about leaks.

---
Cheers,
Benno

Powered by blists - more mailing lists