linux-kernel - Re: [PATCH v4 4/6] rust: irq: add support for threaded IRQs and handlers

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: <DATCZMJJ1SQT.24OPC80MXN1E5@kernel.org>
Date: Sun, 22 Jun 2025 22:53:38 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>, "Daniel Almeida"
 <daniel.almeida@...labora.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>, "Thomas Gleixner" <tglx@...utronix.de>, "Bjorn
 Helgaas" <bhelgaas@...gle.com>, Krzysztof Wilczyński
 <kwilczynski@...nel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v4 4/6] rust: irq: add support for threaded IRQs and
 handlers

On Mon Jun 9, 2025 at 8:13 PM CEST, Danilo Krummrich wrote:
> On Mon, Jun 09, 2025 at 01:24:40PM -0300, Daniel Almeida wrote:
>> > On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@...nel.org> wrote:
>> >> +#[pin_data]
>> >> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
>> >> +    inner: Devres<RegistrationInner>,
>> >> +
>> >> +    #[pin]
>> >> +    handler: T,
>> >> +
>> >> +    /// Pinned because we need address stability so that we can pass a pointer
>> >> +    /// to the callback.
>> >> +    #[pin]
>> >> +    _pin: PhantomPinned,
>> >> +}
>> > 
>> > Most of the code in this file is a duplicate of the non-threaded registration.
>> > 
>> > I think this would greatly generalize with specialization and an HandlerInternal
>> > trait.
>> > 
>> > Without specialization I think we could use enums to generalize.
>> > 
>> > The most trivial solution would be to define the Handler trait as
>> > 
>> > trait Handler {
>> >   fn handle(&self);
>> >   fn handle_threaded(&self) {};
>> > }
>> > 
>> > but that's pretty dodgy.
>> 
>> A lot of the comments up until now have touched on somehow having threaded and
>> non-threaded versions implemented together. I personally see no problem in
>> having things duplicated here, because I think it's easier to reason about what
>> is going on this way. Alice has expressed a similar view in a previous iteration.
>> 
>> Can you expand a bit more on your suggestion? Perhaps there's a clean way to do
>> it (without macros and etc), but so far I don't see it.
>
> I think with specialization it'd be trivial to generalize, but this isn't
> stable yet. The enum approach is probably unnecessarily complicated, so I agree
> to leave it as it is.
>
> Maybe a comment that this can be generalized once we get specialization would be
> good?
>
>> >> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
>> >> +    /// Registers the IRQ handler with the system for the given IRQ number.
>> >> +    pub(crate) fn register<'a>(
>> >> +        dev: &'a Device<Bound>,
>> >> +        irq: u32,
>> >> +        flags: Flags,
>> >> +        name: &'static CStr,
>> >> +        handler: T,
>> >> +    ) -> impl PinInit<Self, Error> + 'a {
>> > 
>> > What happens if `dev`  does not match `irq`? The caller is responsible to only
>> > provide an IRQ number that was obtained from this device.
>> > 
>> > This should be a safety requirement and a type invariant.
>> 
>> This iteration converted register() from pub to pub(crate). The idea was to
>> force drivers to use the accessors. I assumed this was enough to make the API
>> safe, as the few users in the kernel crate (i.e.: so far platform and pci)
>> could be manually checked for correctness.
>> 
>> To summarize my point, there is still the possibility of misusing this from the
>> kernel crate itself, but that is no longer possible from a driver's
>> perspective.
>
> Correct, you made Registration::new() crate private, such that drivers can't
> access it anymore. But that doesn't make the function safe by itself. It's still
> unsafe to be used from platform::Device and pci::Device.
>
> While that's fine, we can't ignore it and still have to add the corresponding
> safety requirements to Registration::new().
>
> I think there is a way to make this interface safe as well -- this is also
> something that Benno would be great to have a look at.

Finally had some time to look through this thread, thought I needed a
whole lot of context, but turns out the question is simple :)

Your idea looks sound :)

---
Cheers,
Benno

> I'm thinking of something like
>
> 	/// # Invariant
> 	///
> 	/// `ìrq` is the number of an interrupt source of `dev`.
> 	struct IrqRequest<'a> {
> 	   dev: &'a Device<Bound>,
> 	   irq: u32,
> 	}
>
> and from the caller you could create an instance like this:
>
> 	// INVARIANT: [...]
> 	let req = IrqRequest { dev, irq };
>
> I'm not sure whether this needs an unsafe constructor though.