linux-kernel - Re: [PATCH v2] rust/kernel: Add faux device bindings

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: <2025020904-defile-juror-67ab@gregkh>
Date: Sun, 9 Feb 2025 16:43:23 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Lyude Paul <lyude@...hat.com>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
	rust-for-linux@...r.kernel.org,
	Maíra Canal <mairacanal@...eup.net>,
	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>,
	Danilo Krummrich <dakr@...nel.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Xiangfei Ding <dingxiangfei2009@...il.com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] rust/kernel: Add faux device bindings

On Fri, Feb 07, 2025 at 05:10:29PM -0500, Lyude Paul wrote:
> On Fri, 2025-02-07 at 13:16 +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 07, 2025 at 12:42:41PM +0100, Miguel Ojeda wrote:
> > > On Fri, Feb 7, 2025 at 1:42 AM Lyude Paul <lyude@...hat.com> wrote:
> > > > 
> > > > This introduces a crate for working with faux devices in rust, along with
> > > 
> > > s/crate/module
> > > 
> > > (also in the module description)
> > > 
> > > > +//! C header: [`include/linux/device/faux.h`]
> > > > +use crate::{bindings, device, error::code::*, prelude::*};
> > > 
> > > Newline between.
> > > 
> > > > +        // SAFETY: self.0 is a valid registered faux_device via our type invariants.
> > > 
> > > Markdown.
> > > 
> > > > +// SAFETY: The faux device API is thread-safe
> > > > +unsafe impl Send for Registration {}
> > > > +
> > > > +// SAFETY: The faux device API is thread-safe
> > > > +unsafe impl Sync for Registration {}
> > > 
> > > Perhaps some extra notes here would be useful, e.g. is it documented
> > > to be so? Especially since faux is being added now, it may make sense
> > > to e.g. take the chance to work on mentioning this on the C side.
> > 
> > How can or should I mention this on the C side?
> 
> This is a very good question :), especially because it turns out I actually
> think this function is not thread-safe! Though I don't think that's actually
> much of a problem for Send/Sync here:
> 
> So - my original assumption was that since faux_device_destroy() just wraps
> around device_del() and put_device() we'd get thread safety. put_device() is
> thread-safe, but on closer inspection I don't see that device_del() is. It
> _can_ be called from any thread, but only so long as there is a guarantee it's
> called exactly once. I think that's fine both for C and rust, but it
> definitely warrants a more descriptive SAFETY comment from me.
> 
> So for the C side of things I might actually add documentation to device_del()
> for this that would look something like this:
> 
>    device_del() can be called from any thread, but provides no protection
>    against multiple calls. It is up to the caller to ensure this function may
>    only be called once for a given device.

You are correct in that device_del() has to be called only once, but the
idea of "thread safe" really isn't a thing in the kernel because we have
processes entering/exiting the code from all different places so things
better be "thread safe" almost everywhere, what matters to us as you
know is "context" due to sleep/locking issues.

So how about this wording instead:
	device_del() must be called from process context, not interrupt
	context, and also provides no protection against multiple calls
	for the same device.  It MUST be up to the caller to ensure that
	this function can only be called once for a given device.


> And then I suppose we could refer back to device_del() in faux_device_destroy()'s
> documentation if we want. For the rust side of thing the safety comment could
> just be like this:
> 
>    // SAFETY: Our bindings ensure only one `Registration` can exist at a time,
>    meaning faux_device_destroy() can only be called once for a given
>    registration - fulfilling the driver core's requirements for thread-safety.

Sure, but I think we are going to start getting "thread" confusion over
time here, is this something we want to start thinking about or am I
just running into it for the first time here?  I know it's a userspace
thing but not normally thought of in the kernel subsystems I'm familiar
with.

thanks,

greg k-h