Re: [PATCH 4/4] scsi: pm8001: free MSI-X vectors before INT-X fallback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Early registration of interrupt handler causes a race condition leading to a NULL pointer dereference.
- [High] The INT-X fallback path is unreachable dead code, and the driver fails to initialize if MSI-X allocation fails.
--

commit 41233b053e82eb6b313eebe72e0ecc8e816d565f
Author: Peng Hao <flyingpenghao@gmail.com>

scsi: pm8001: free MSI-X vectors before INT-X fallback

pm8001_setup_msix() may allocate MSI-X vectors before
pm8001_request_irq() decides to use INT-X. The fallback currently leaves
those vectors allocated. It also leaves max_q_num set to the MSI-X
vector count even though only one legacy interrupt is used.

Free allocated vectors before requesting INT-X, clear number_of_intr and
restore the single-queue configuration. Clear number_of_intr as well
when MSI-X IRQ registration fails after the vectors have been freed.

> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index a747de9a3ca0a..4bf6c747be8b8 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c

[ ... ]

> @@ -1087,6 +1088,12 @@ static u32 pm8001_request_irq(struct pm8001_hba_info *pm8001_ha)
>  	}
>  
>  use_intx:

[Severity: High]
This is a pre-existing issue, but is this INT-X fallback path actually
reachable?

Looking at pm8001_request_irq(), if pm8001_setup_msix() fails it immediately
returns the error rather than falling back to use_intx:

    rc = pm8001_setup_msix(pm8001_ha);
    if (rc) {
        ...
        return rc;
    }

    if (!pdev->msix_cap || !pci_msi_enabled())
        goto use_intx;

Since pm8001_setup_msix() only succeeds if MSI-X is supported and enabled,
the subsequent checks for !pdev->msix_cap and !pci_msi_enabled() will never
be true.

Does this leave the fallback as dead code, causing driver initialization to
fail completely instead of using legacy interrupts when MSI-X fails?

[Severity: Critical]
This is a pre-existing issue, but does setting up the INT-X interrupt here
introduce a race condition?

The interrupt handler is registered here during pm8001_alloc() via
pm8001_request_irq(), but hardware queues and pointers are not initialized
until later in pm8001_pci_probe() when calling chip_init().

If a shared INT-X line or pending hardware state causes an interrupt to fire
immediately, the handler will schedule a tasklet executing process_oq().
This function reads from the uninitialized queue pointers:

process_oq()
    circularQ->producer_index = cpu_to_le32(pm8001_read_32(circularQ->pi_virt));

Since pi_virt is still NULL before chip_init() is called, could this result
in a NULL pointer dereference?

> +	if (pm8001_ha->number_of_intr) {
> +		pci_free_irq_vectors(pdev);
> +		pm8001_ha->number_of_intr = 0;
> +	}
> +	pm8001_ha->max_q_num = 1;
> +
>  	/* Initialize the INT-X interrupt */
>  	pm8001_dbg(pm8001_ha, INIT, "MSIX not supported!!!\n");
>  	pm8001_ha->use_msix = false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911091011.94676-1-flyingpeng@tencent.com?part=4