Re: [PATCH v2 1/5] rust: pci: convert IrqVectorRegistration to a lifetime-managed owning type

[sashiko-bot@kernel.org]

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

New issues:
- [High] The lifetime bounds introduced on `IrqVectorRegistration` and `IrqVector` force an impossible self-referential struct, rendering the API unusable in safe Rust.

Pre-existing issues:
- [High] Concurrent calls to `alloc_irq_vectors` allow unsynchronized mutation of PCI device state, leading to data races and kernel memory corruption.
--

--- Patch [1]: [PATCH v2 1/5] rust: pci: convert IrqVectorRegistration to a lifetime-managed owning type ---
commit c70c8b630ade05d17f2339a929d25258bdd64514
Author: Danilo Krummrich <dakr@kernel.org>

rust: pci: convert IrqVectorRegistration to a lifetime-managed owning type

This commit converts IrqVectorRegistration to a lifetime-annotated type that
owns the PCI interrupt vector allocation. Dropping it frees the vectors, and
alloc_irq_vectors() is updated to return IrqVectorRegistration directly.

> diff --git a/rust/kernel/pci/irq.rs b/rust/kernel/pci/irq.rs
> index fea484dcf9cf2..8e06515878290 100644
> --- a/rust/kernel/pci/irq.rs
> +++ b/rust/kernel/pci/irq.rs
> @@ -78,6 +75,7 @@ const fn as_raw(self) -> u32 {
>  #[derive(Clone, Copy)]
>  pub struct IrqVector<'a> {
>      dev: &'a Device<Bound>,
> +    reg: &'a IrqVectorRegistration<'a>,
>      index: u32,
>  }

[Severity: High]
Will this lifetime bound prevent drivers from storing their PCI interrupts in a
device data structure?

If a driver calls alloc_irq_vectors() to get an IrqVectorRegistration, borrows
it via vector() to create an irq::Registration, and then tries to store both in
its driver data struct, the compiler will reject it. Safe Rust does not allow
the registration and the IRQ handler to be stored in the same struct due to
these self-referential lifetime constraints. 

This seems to force drivers to either leak memory or drop the registration
prematurely, which would instantly free the interrupts via
pci_free_irq_vectors().

[ ... ]

> @@ -256,7 +249,21 @@ pub fn alloc_irq_vectors(
>          &self,
>          min_vecs: u32,
>          max_vecs: u32,
>          irq_types: IrqTypes,
> -    ) -> Result<RangeInclusive<IrqVector<'_>>> {
> -        IrqVectorRegistration::register(self, min_vecs, max_vecs, irq_types)
> +    ) -> Result<IrqVectorRegistration<'_>> {

[Severity: High]
This is a pre-existing issue, but is it safe for alloc_irq_vectors to take an
immutable reference when it mutates PCI device state?

Because alloc_irq_vectors takes &self (which is Sync), a driver could expose
concurrent sysfs or ioctl endpoints that trigger reallocation on the same
Device. 

The underlying C function pci_alloc_irq_vectors checks and modifies
dev->msix_enabled without locking. This can cause a data race, leading to
double-allocation of MSI-X descriptors, leaking of ioremap mappings, and
corruption of internal PCI device state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811233952.3000968-1-dakr@kernel.org?part=1