linux-kernel - Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

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: <aGLUl7ZtuQBPoCuv@pollux>
Date: Mon, 30 Jun 2025 20:16:55 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Matthew Maurer <mmaurer@...gle.com>
Cc: 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>,
	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>, Benno Lossin <lossin@...nel.org>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	Dirk Behme <dirk.behme@...bosch.com>
Subject: Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing
 for File

On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@...nel.org> wrote:
> >
> > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@...nel.org> wrote:
> > >>
> > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > >>> +    where
> > >>> +        for<'a> D::Borrowed<'a>: Display,
> > >>> +    {
> > >>> +        File {
> > >>> +            _foreign: ForeignHolder::new(data),
> > >>> +        }
> > >>>        }
> > >>
> > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > >> File<D> and store data directly?
> > >
> > > 1. A `File<D>` can't be held in collection data structures as easily
> > > unless all your files contain the *same* backing type.
> >
> > That sounds reasonable.
> >
> > > 2. None of the APIs or potential APIs for `File` care about which type
> > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > `File` is different depending on the backing type, making it
> > > polymorphic is just needlessly confusing.
> >
> > What if I want to access file.data() and do something with the data? Then I'd
> > necessarily need to put my data in an Arc and reference count it to still be
> > able to access it.
> >
> > That doesn't seem like a reasonable requirement to be able to access data
> > exposed via debugfs.
> 
> `pub fn data(&self) -> D` would go against my understanding of Greg's
> request for DebugFS files to not really support anything other than
> delete. I was even considering making `D` not be retained in the
> disabled debugfs case, but left it in for now for so that the
> lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has
no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file()
without any implication of ownership. debugfs has a pointer, the driver has a
pointer. The question of the ownership semantics is not answered by the API, but
by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you
expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference
counted, it:

  1) Is additional overhead imposed on users.

  2) It has implications on the ownership design of your driver. Once something
     is reference counted, you loose the guarantee the something can't out-live
     some event.

I don't want that people have to stuff their data structures into Arc (i.e.
reference count them), even though that's not necessary. It makes it easy to
make mistakes. Things like:

	let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a
KBox to an Arc is much harder to miss.

> If you want a `.data()` function, I can add it in,

I think it could even be an implementation of Deref.

> but I don't think
> it'll improve flexibility in most cases. If you want to do something
> with the data and it's not in an `Arc` / behind a handle of some kind,
> you'll need something providing threadsafe interior mutability in the
> data structure. If that's a lock, then I have a hard time believing
> that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> is why I added that in the stack) is so much more expensive than
> `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> `Box<AtomicU8>` over that, but it still seems so slim that I think the
> simpler "`File` is just a handle to how long the file stays alive, it
> doesn't let you do anything else" API makes sense.

I don't really see what is complicated about File<T> -- it's a File and it owns
data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in
terms of clarity about ownership and lifetime of an object as explained above
clearly is.

- Danilo