Re: [PATCH 4/7] s390/pci: fix MSI directed-mode teardown IRQ bit count

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

Pre-existing issues:
- [High] Double-addition logic error in `zpci_msi_clear_airq()` causes out-of-bounds memory writes and leaves dangling pointers.
- [High] Directed MSI teardown leaks uncleared hardware vector bits on CPUs > 0, causing spurious interrupts.
- [Medium] Missing RCU pointer assignment when publishing `zdev->aibv` to `zpci_ibv` array.
--

--- Patch [4]: [PATCH 4/7] s390/pci: fix MSI directed-mode teardown IRQ bit count ---
commit 061bcdd7920b9b7d42fc840ba0aa403eaf0255a5
Author: Tobias Schumacher <[email protected]>

s390/pci: fix MSI directed-mode teardown IRQ bit count

This commit updates zpci_msi_teardown_directed() to free the actual number of
allocated MSI bits (msi_nr_irqs) instead of the maximum supported by the
platform (max_msi). This prevents freeing bits that were not actually
allocated, which would corrupt the shared IRQ bitmap.

> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 5e934ac990ac1..e9eda846cb2d2 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c
> @@ -349,7 +349,7 @@ static struct airq_struct zpci_airq = {
>  
>  static void zpci_msi_teardown_directed(struct zpci_dev *zdev)
>  {
> -	airq_iv_free(zpci_ibv[0], zdev->msi_first_bit, zdev->max_msi);
> +	airq_iv_free(zpci_ibv[0], zdev->msi_first_bit, zdev->msi_nr_irqs);

[Severity: High]
This is a pre-existing issue, but does this leak hardware vector bits on CPUs
other than 0?

It looks like this only clears the vector bits for zpci_ibv[0]. Since CPUs
greater than 0 have zpci_ibv[cpu] created without AIRQ_IV_ALLOC, their avail
mask is NULL, and airq_iv_free() returns immediately without clearing the bits
in zpci_ibv[cpu]->vector.

If a pending hardware interrupt bit is left set on a CPU greater than 0 after
device teardown, could reallocating that bit index to a new device trigger a
spurious interrupt on that CPU?

>  	zdev->msi_first_bit = -1U;
>  	zdev->msi_nr_irqs = 0;
>  }

[Severity: High]
This is a pre-existing issue, but is there a double-addition logic error in
zpci_msi_clear_airq()?

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;

	if (irq_delivery == DIRECTED) {
		for_each_possible_cpu(cpu) {
			airq_iv_set_ptr(zpci_ibv[cpu], bit + i, 0);
			airq_iv_set_data(zpci_ibv[cpu], bit + i, 0);
...
}

During allocation, irq_domain_set_info() is passed hwirq + i, so d->hwirq
already incorporates the index i. zpci_decode_hwirq_msi_index(d->hwirq)
decodes this to desc->msi_index + i.

Since bit is calculated as zdev->msi_first_bit + msi_index (which already
includes i), clearing bit + i evaluates to base + 2*i.

Does this skip odd-indexed entries and write out of bounds when clearing,
leaving the intended MSI vector entries dangling?


[Severity: Medium]
This is a pre-existing issue, but is an RCU pointer assignment missing when
publishing zdev->aibv to the zpci_ibv array?

arch/s390/pci/pci_irq.c:__alloc_airq() {
	...
		/* Create adapter interrupt vector */
		zdev->aibv = airq_iv_create(msi_vecs,
					    AIRQ_IV_PTR | AIRQ_IV_DATA | AIRQ_IV_BITLOCK,
					    NULL);
	...
		/* Wire up shortcut pointer */
		zpci_ibv[*bit] = zdev->aibv;
}

The interrupt handler zpci_floating_irq_handler() correctly uses
rcu_dereference() to read zpci_ibv[si]. However, this plain assignment
publishes the pointer to readers without the required RCU compiler barriers.

Could compiler optimizations like LTO reorder the initialization of
zdev->aibv to occur after the pointer is published, allowing a concurrent
interrupt handler to read uninitialized structures?

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