Re: [PATCH 3/7] s390/pci: fix resource leak in zpci MSI setup

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

Pre-existing issues:
- [High] zpci_msi_teardown_directed() unconditionally frees `zdev->max_msi` vectors instead of the dynamically allocated `zdev->msi_nr_irqs`, potentially freeing vectors owned by unrelated PCI devices.
- [High] Missing `rcu_assign_pointer()` when publishing the newly created adapter interrupt vector to the globally visible `zpci_ibv` array.
- [High] zpci_msi_clear_airq() double-counts the iteration index, resulting in skipped elements and out-of-bounds memory corruption.
--

--- Patch [3]: [PATCH 3/7] s390/pci: fix resource leak in zpci MSI setup ---
commit ac232ab92e1500cdc9f0a1a221f03f9be96fffe4
Author: Tobias Schumacher <[email protected]>

s390/pci: fix resource leak in zpci MSI setup

If airq_iv_create() fails in __alloc_airq(), the zpci_sbv bit allocated
by airq_iv_alloc_bit() is never freed. This permanently leaks one of the
ZPCI_NR_DEVICES summary bits (~128 total), reducing system capacity with
each failed device hotplug.

> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 94b03d16006ba..5e934ac990ac1 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c
> @@ -320,8 +320,11 @@ static int __alloc_airq(struct zpci_dev *zdev, int msi_vecs,
>  		zdev->aibv = airq_iv_create(msi_vecs,
>  					    AIRQ_IV_PTR | AIRQ_IV_DATA | AIRQ_IV_BITLOCK,
>  					    NULL);
> -		if (!zdev->aibv)
> +		if (!zdev->aibv) {
> +			airq_iv_free_bit(zpci_sbv, *bit);
> +			zdev->aisb = -1UL;
>  			return -ENOMEM;
> +		}
>  
>  		/* Wire up shortcut pointer */
>  		zpci_ibv[*bit] = zdev->aibv;

[Severity: High]
This isn't a bug introduced by this patch, but does this direct assignment
need a memory barrier?

This pointer is published to the globally visible zpci_ibv array.
Concurrently, the asynchronous hardware interrupt handler
zpci_floating_irq_handler() locklessly reads this array using
rcu_dereference(zpci_ibv[si]).

Without an rcu_assign_pointer() or equivalent release barrier, could the
reader observe the aibv pointer before its internal memory regions are
globally visible, leading to an invalid pointer dereference?

[Severity: High]
This is a pre-existing issue, but does zpci_msi_teardown_directed() free
vectors owned by unrelated PCI devices?

When a driver requests fewer IRQs than its maximum capability, __alloc_airq()
dynamically allocates exactly msi_vecs bits. However, the teardown blindly
frees max_msi bits:

arch/s390/pci/pci_irq.c:zpci_msi_teardown_directed() {
    ...
	airq_iv_free(zpci_ibv[0], zdev->msi_first_bit, zdev->max_msi);
    ...
}

Will this cause memory corruption by freeing trailing vectors that might have
been allocated to another device? Should this free zdev->msi_nr_irqs instead?

[Severity: High]
This is a pre-existing issue, but does zpci_msi_clear_airq() double-count
the iteration index and write out of bounds?

In zpci_msi_domain_free(), it iterates over nr_irqs and passes the loop index
i to zpci_msi_clear_airq():

arch/s390/pci/pci_irq.c:zpci_msi_domain_free() {
    ...
	for (i = 0; i < nr_irqs; i++) {
		d = irq_domain_get_irq_data(domain, virq + i);
		zpci_msi_clear_airq(d, i);
    ...
}

Inside zpci_msi_clear_airq(), the hwirq already reflects the correct index.
The bit is calculated, but then i is added again:

arch/s390/pci/pci_irq.c:zpci_msi_clear_airq() {
    ...
	msi_index = zpci_decode_hwirq_msi_index(d->hwirq);
	bit = zdev->msi_first_bit + msi_index;
    ...
		airq_iv_set_ptr(zdev->aibv, bit + i, 0);
    ...
}

If nr_irqs is greater than 1, doesn't this skip the intended elements and
corrupt pointers in unrelated active IRQs while failing to clear the
target vectors?

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