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: <aCRdNJ2oq-REBotd@pollux>
Date: Wed, 14 May 2025 11:07:00 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <lossin@...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 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.

It either just adds complexity to the API (due to the need to distingish between
the "root" directory and sub-directories) or makes the API error prone by
providing a *valid looking* option to users to leak the "root" directory.

- Danilo