Re: [PATCH] scsi: pm8001: use rollback index when freeing MSI-X vectors

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

Pre-existing issues:
- [High] Probe and allocation error paths fail to unregister IRQ handlers, kill tasklets, and free PCI IRQ vectors.
- [High] Incorrect error jump label in pm8001_pci_probe() bypasses all cleanup.
- [High] Broken fallback to legacy INT-X interrupts when MSI-X vector allocation fails.
- [Medium] Missing teardown of specific memory allocations (pm8001_ha->devices, sha->sas_phy, sha->sas_port) in probe error paths.
--

--- Patch [1]: [PATCH] scsi: pm8001: use rollback index when freeing MSI-X vectors ---
commit 961717b6ef49f9e3b852de4ab73c7119e69629df
Author: Runyu Xiao <[email protected]>

scsi: pm8001: use rollback index when freeing MSI-X vectors

pm8001_request_msix() unwinds previously registered handlers with
free_irq() when request_irq() fails. The rollback loop uses the
failing index i for every iteration instead of the already
registered vector index j. This patch uses j for both
pci_irq_vector() and the matching irq_vector entry in the
rollback loop.

Cc: [email protected]

> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index e93ea76b565e5..54b35893261a6 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1029,8 +1029,8 @@ static u32 pm8001_request_msix(struct pm8001_hba_info *pm8001_ha)
>  			&(pm8001_ha->irq_vector[i]));
>  		if (rc) {
>  			for (j = 0; j < i; j++) {
> -				free_irq(pci_irq_vector(pm8001_ha->pdev, i),
> -					&(pm8001_ha->irq_vector[i]));
> +				free_irq(pci_irq_vector(pm8001_ha->pdev, j),
> +					 &pm8001_ha->irq_vector[j]);
>  			}
>  			pci_free_irq_vectors(pm8001_ha->pdev);
>  			break;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that probe and
allocation error paths fail to unregister IRQ handlers, kill tasklets, and
free PCI IRQ vectors.

Looking at pm8001_alloc() in drivers/scsi/pm8001/pm8001_init.c:

	rc = pm8001_request_irq(pm8001_ha);
	if (rc)
		goto err_out;
...
err_out:
	return 1;

Does the internal error path here return failure after pm8001_request_irq()
succeeded, bypassing IRQ teardown?

Additionally, the primary teardown function used in probe error paths does
not call pm8001_free_irq(), pm8001_kill_tasklet(), or
pci_free_irq_vectors():

static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
{
...
	PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
	flush_workqueue(pm8001_wq);
	bitmap_free(pm8001_ha->rsvd_tags);
	kfree(pm8001_ha);
}

When probe error paths invoke pm8001_free() and kfree(sha):

err_out_ha_free:
	pm8001_free(pm8001_ha);
err_out_free:
	kfree(sha);

Doesn't this leave dangling IRQ handlers that still point to these freed
structures?

If probe fails after IRQs are requested, it appears the driver leaks the
IRQ handlers, tasklets, and MSI-X vectors. Furthermore, since the handler's
dev_id pointer (pm8001_ha) is freed by pm8001_free(), could a subsequent
interrupt on the shared line or an unmasked device interrupt cause a kernel
panic when the ISR dereferences pm8001_ha?

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but I noticed the incorrect error jump label in pm8001_pci_probe()
bypasses all cleanup.

In pm8001_pci_probe():

	rc = pm8001_init_ccb_tag(pm8001_ha);
	if (rc)
		goto err_out_enable;
...
err_out_regions:
	pci_release_regions(pdev);
err_out_disable:
	pci_disable_device(pdev);
err_out_enable:
	return rc;

Does this failure path wrongly jump to the end of the function, bypassing
all preceding teardown labels?

If pm8001_init_ccb_tag() fails during probe, the code jumps directly to the
err_out_enable label. This skips all necessary rollback steps, leaking the
Scsi_Host, PCI regions, sas_ha_struct, and pm8001_ha structures, and
leaving the PCI device enabled.

[Severity: Medium]
This isn't a bug introduced by this patch, but there seems to be missing
teardown of specific memory allocations in probe error paths.

In pm8001_free():

static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
{
...
	bitmap_free(pm8001_ha->rsvd_tags);
	kfree(pm8001_ha);
}

Does this function omit kfree(pm8001_ha->devices), unlike its counterpart
pm8001_pci_remove()?

Also, in the probe error path:

err_out_free:
	kfree(sha);

Is it safe for the probe error path to drop the sha structure without
freeing its embedded dynamically allocated arrays (sas_phy and sas_port)?

The structures allocated in pm8001_alloc() (pm8001_ha->devices) and
pm8001_prep_sas_ha_init() (sha->sas_phy and sha->sas_port) are properly
freed during device removal in pm8001_pci_remove(), but seem completely
ignored in the standard probe error paths. Can this lead to memory leaks
on cold paths when initialization fails?

[Severity: High]
This is a pre-existing issue, but I noticed a broken fallback to legacy
INT-X interrupts when MSI-X vector allocation fails.

Looking at pm8001_request_irq():

	if (pm8001_use_msix && pci_find_capability(pdev, PCI_CAP_ID_MSIX)) {
		rc = pm8001_setup_msix(pm8001_ha);
		if (rc) {
			pm8001_dbg(pm8001_ha, FAIL,
				   "pm8001_setup_irq failed [ret: %d]\n", rc);
			return rc;
		}

Does this exact block break the fallback logic by unconditionally returning
rather than branching to the use_intx label when allocation fails?

If pm8001_setup_msix() fails, the function immediately returns the error.
This entirely skips the intended fallback to legacy shared interrupts at
the use_intx label. Could this unnecessarily fail to initialize the device
on systems where MSI-X is unavailable?

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