Re: [PATCH v2 4/7] s390/pci: Fix use-after-free race in zpci floating interrupt cleanup

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Mon, 2026-10-05 at 14:03 +0200, Tobias Schumacher wrote:
> zpci_clear_irq() stops the adapter from raising new interrupts for the
> function, but a zpci_floating_irq_handler() already running on another CPU
> can still be scanning zdev->aibv when zpci_msi_teardown_floating() releases
> it.
> 
> Clear the zpci_ibv[] entry so no further handler picks the vector up, then
> wait for a grace period before releasing it. The handler runs inside the
> rcu_read_lock() section that do_airq_interrupt() holds across
> airq->handler(), so synchronize_rcu() drains any handler still in flight.
> Free the summary bit only after the grace period, so it cannot be handed to
> another device while a reader still holds the old pointer.
> 
> zpci_ibv served both delivery modes, indexed by summary bit under

Nit: Maybe more precisely and matching the comment: "…, indexed by
function under FLOATING and …"?

> FLOATING and by cpu under DIRECTED. Only the floating vectors are
> published to and torn down under the interrupt handler, so split the
> directed vectors out into zpci_dibv and annotate zpci_ibv __rcu, which
> lets sparse check the accessors above.
> 
> Fixes: f770950a4709 ("s390/pci: Migrate s390 IRQ logic to IRQ domain API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tobias Schumacher <ts@linux.ibm.com>
> ---
>  arch/s390/pci/pci_irq.c | 60 +++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index d5763c5feb09..81a27bf756a3 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c
> @@ -22,12 +22,11 @@ static enum {FLOATING, DIRECTED} irq_delivery;
>   */
>  static struct airq_iv *zpci_sbv;
>  
> -/*
> - * interrupt bit vectors
> - * FLOATING - interrupt bit vector per function
> - * DIRECTED - interrupt bit vector per cpu
> - */
> -static struct airq_iv **zpci_ibv;
> +/* FLOATING - interrupt bit vector per function */
> +static struct airq_iv __rcu **zpci_ibv;
> +
> +/* DIRECTED - interrupt bit vector per cpu */
> +static struct airq_iv **zpci_dibv;
--- snip ---
>  
>  static void zpci_msi_teardown_floating(struct zpci_dev *zdev)
>  {
> +	rcu_assign_pointer(zpci_ibv[zdev->aisb], NULL);
> +	synchronize_rcu();
> +	airq_iv_free_bit(zpci_sbv, zdev->aisb);
> +
>  	airq_iv_release(zdev->aibv);
>  	zdev->aibv = NULL;
> -	airq_iv_free_bit(zpci_sbv, zdev->aisb);

Not sure why the aisb free moved to before the aibv release? The commit
message only explains why it is after the synchronize_rcu(). This way
it's also not in the opposite order of the allocation.

>  	zdev->aisb = -1UL;
>  	zdev->msi_first_bit = -1U;
>  	zdev->msi_nr_irqs = 0;
> @@ -428,9 +432,9 @@ static int zpci_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  
--- snip ---

Overall the change and the use of RCU looks good to me.

Thanks,
Niklas