Re: [PATCH v3 2/5] rust: pci: resolve IRQ in index() and embed IrqRequest in IrqVector

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The API design forces a self-referential lifetime bound, making it impossible to safely store both the vector registration and the IRQ handler in a driver struct, naturally leading to Use-After-Free during teardown.

Pre-existing issues:
- [High] Multiple concurrent `IrqVectorRegistration` instances can be created, leading to a Use-After-Free of active IRQ vectors on teardown.
--

--- Patch [2]: [PATCH v3 2/5] rust: pci: resolve IRQ in index() and embed IrqRequest in IrqVector ---
commit 517346220f40fc029bc56afcf3a1427744cf6f09
Author: Danilo Krummrich <[email protected]>

rust: pci: resolve IRQ in index() and embed IrqRequest in IrqVector

This commit eagerly resolves the IRQ number by moving the pci_irq_vector()
call into IrqVectorRegistration::index(). It embeds the resolved IrqRequest
into IrqVector, making the conversion infallible. This removes the need for
pin_init_scope() in request_irq() and request_threaded_irq().

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/rust/kernel/pci/irq.rs b/rust/kernel/pci/irq.rs
> index daba86505cd2b..81b74c4c17d98 100644
> --- a/rust/kernel/pci/irq.rs
> +++ b/rust/kernel/pci/irq.rs
> @@ -68,32 +68,25 @@ const fn as_raw(self) -> u32 {
[ ... ]
>  pub struct IrqVector<'a> {
> -    dev: &'a Device<Bound>,
> +    request: IrqRequest<'a>,
>      reg: &'a IrqVectorRegistration<'a>,
> -    index: u32,
>  }

[Severity: High]
Does this struct definition force a self-referential lifetime bound that makes
it impossible to safely store both the vector registration and the IRQ handler
in a driver struct?

Because IrqVector<'a> borrows IrqVectorRegistration<'a> with the exact same
lifetime 'a as the device, request_irq() requires the resulting
irq::Registration to share this lifetime. A long-lived driver struct would
have to hold both the IrqVectorRegistration and the Registration that
borrows it.

This creates a self-referential struct forbidden by Safe Rust. If a developer
uses unsafe to bypass the restrictions to get it to compile, Rust drops
struct fields in declaration order. If IrqVectorRegistration is declared
before Registration, pci_free_irq_vectors() will execute before free_irq(),
destroying MSI vectors while the IRQ handler is still registered and active
in the system.

[ ... ]
> @@ -200,12 +183,8 @@ pub unsafe fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'a>(
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> impl PinInit<irq::ThreadedRegistration<'a, T>, Error> + 'a {
> -        pin_init::pin_init_scope(move || {
> -            let request = vector.try_into()?;
> -
> -            // SAFETY: Caller guarantees the Registration will not be leaked.
> -            Ok(unsafe { irq::ThreadedRegistration::<T>::new(request, flags, name, handler) })
> -        })
> +        // SAFETY: Caller guarantees the Registration will not be leaked.
> +        unsafe { irq::ThreadedRegistration::<T>::new(vector.into(), flags, name, handler) }
>      }
>  
>      /// Allocate IRQ vectors for this PCI device.

[Severity: High]
This is a pre-existing issue, but can multiple concurrent
IrqVectorRegistration instances be created, leading to a Use-After-Free of
active IRQ vectors on teardown?

Since Device::alloc_irq_vectors() takes an immutable reference (&self), a
driver can call it multiple times. If a driver allocates MSI vectors and
then falls back to allocating INTx vectors, the C API
(pci_alloc_irq_vectors_affinity) successfully processes the INTx request
without checking if MSI is already enabled. This returns a second
IrqVectorRegistration.

When this second registration goes out of scope and drops, its Drop
implementation calls pci_free_irq_vectors(), which disables and frees the
global MSI state. The first registration's active IRQ handlers are then
left operating on freed hardware state and descriptors.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2