[PATCH 0/4] scsi: pm8001: fix probe error-path resource leaks

[PATCH 0/4] scsi: pm8001: fix probe error-path resource leaks

[Peng Hao]

Following the fix for the incorrect IRQ index used while unwinding a
partial MSI-X setup, further review found several related resource
lifetime issues in the pm8001 initialization and probe error paths.

An allocation failure after IRQ registration can leave handlers active
after their HBA data has been freed. Other probe failures bypass parts of
the normal teardown sequence and leak tasklets, SAS host allocations,
CCB resources, or the devices array. In addition, falling back from MSI-X
to INT-X leaves the allocated MSI-X vectors and multi-queue state behind.

This series makes the error-path ownership and teardown ordering
consistent:

  - release registered IRQs before destroying their HBA and tasklet data;
  - route failures after controller initialization through chip reset and
    the complete probe cleanup chain;
  - centralize CCB teardown so both partial allocation failures and normal
    HBA destruction release all CCB-related resources;
  - release MSI-X vectors and restore single-queue state before requesting
    the legacy INT-X interrupt.

Peng Hao (4):
  scsi: pm8001: free IRQs when HBA allocation fails
  scsi: pm8001: clean up resources on probe failure
  scsi: pm8001: free CCB resources on allocation failure
  scsi: pm8001: free MSI-X vectors before INT-X fallback

 drivers/scsi/pm8001/pm8001_init.c | 88 ++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 33 deletions(-)

-- 
2.43.7

[PATCH 1/4] scsi: pm8001: free IRQs when HBA allocation fails

[Peng Hao]

pm8001_alloc() registers the interrupt handlers before allocating the MPI
memory regions and device array. If one of those later allocations fails,
pm8001_pci_alloc() frees the HBA while the IRQ handlers remain registered.
A subsequent interrupt can therefore dereference the freed HBA or SAS host
data.

Release the IRQs on every failure after successful registration. Let
pm8001_free() perform the memory-region cleanup, and kill the initialized
tasklets before freeing the HBA.

Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 54b35893261a..8ede1f1da415 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -312,7 +312,7 @@ static void pm8001_free_irq(struct pm8001_hba_info *pm8001_ha);
 static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
 			const struct pci_device_id *ent)
 {
-	int i, count = 0, rc = 0;
+	int i, count = 0;
 	u32 ci_offset, ib_offset, ob_offset, pi_offset;
 	struct inbound_queue_table *ibq;
 	struct outbound_queue_table *obq;
@@ -323,9 +323,8 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
 		   pm8001_ha->chip->n_phy);
 
 	/* Request Interrupt */
-	rc = pm8001_request_irq(pm8001_ha);
-	if (rc)
-		goto err_out;
+	if (pm8001_request_irq(pm8001_ha))
+		return 1;
 
 	count = pm8001_ha->max_q_num;
 	/* Queues are chosen based on the number of cores/msix availability */
@@ -446,27 +445,16 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
 	/* Memory region for devices*/
 	pm8001_ha->devices = kzalloc(PM8001_MAX_DEVICES
 				* sizeof(struct pm8001_device), GFP_KERNEL);
-	if (!pm8001_ha->devices) {
-		rc = -ENOMEM;
-		goto err_out_nodev;
-	}
+	if (!pm8001_ha->devices)
+		goto err_out;
 	for (i = 0; i < PM8001_MAX_DEVICES; i++) {
 		pm8001_ha->devices[i].dev_type = SAS_PHY_UNUSED;
 	}
 	pm8001_ha->flags = PM8001F_INIT_TIME;
 	return 0;
 
-err_out_nodev:
-	for (i = 0; i < pm8001_ha->max_memcnt; i++) {
-		if (pm8001_ha->memoryMap.region[i].virt_ptr != NULL) {
-			dma_free_coherent(&pm8001_ha->pdev->dev,
-				(pm8001_ha->memoryMap.region[i].total_len +
-				pm8001_ha->memoryMap.region[i].alignment),
-				pm8001_ha->memoryMap.region[i].virt_ptr,
-				pm8001_ha->memoryMap.region[i].phys_addr);
-		}
-	}
 err_out:
+	pm8001_free_irq(pm8001_ha);
 	return 1;
 }
 
@@ -575,6 +563,7 @@ static struct pm8001_hba_info *pm8001_pci_alloc(struct pci_dev *pdev,
 	if (!pm8001_alloc(pm8001_ha, ent))
 		return pm8001_ha;
 failed_pci_alloc:
+	pm8001_kill_tasklet(pm8001_ha);
 	pm8001_free(pm8001_ha);
 	return NULL;
 }
-- 
2.43.7

Re: [PATCH 1/4] scsi: pm8001: free IRQs when HBA allocation fails

[sashiko-bot]

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

Pre-existing issues:
- [High] The IRQ handler is registered before queue initialization is complete, causing a NULL pointer dereference on early/spurious interrupts.
- [High] Invalid offset virtual address passed to dma_free_coherent() when freeing MPI memory regions.
--

commit 9749644d500951b76d7ccb7050664ddfe58c9592
Author: Peng Hao <flyingpenghao@gmail.com>

scsi: pm8001: free IRQs when HBA allocation fails

pm8001_alloc() registers the interrupt handlers before allocating the MPI
memory regions and device array. If one of those later allocations fails,
pm8001_pci_alloc() frees the HBA while the IRQ handlers remain registered.
This patch releases the IRQs on every failure after successful registration
and kills the initialized tasklets before freeing the HBA.

> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -323,9 +323,8 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
>  		   pm8001_ha->chip->n_phy);
>  
>  	/* Request Interrupt */
> -	rc = pm8001_request_irq(pm8001_ha);
> -	if (rc)
> -		goto err_out;
> +	if (pm8001_request_irq(pm8001_ha))
> +		return 1;

[Severity: High]
This is a pre-existing issue, but since we are modifying the IRQ registration 
path here, is it safe to request the interrupt before the chip and memory 
queues are initialized?

If an interrupt fires early (e.g., from an unquiesced device via kexec, or a 
spurious interrupt on a shared INT-x line), the tasklet can be scheduled. 
The tasklet executes pm8001_chip_isr(), which calls process_oq().

In drivers/scsi/pm8001/pm8001_hwi.c:process_oq():

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

This unconditionally dereferences circularQ->pi_virt, but pi_virt is not 
populated until much later in the initialization sequence. Could this cause a 
NULL pointer dereference on early or spurious interrupts?

[ ... ]

> @@ -575,6 +563,7 @@ static struct pm8001_hba_info *pm8001_pci_alloc(struct pci_dev *pdev,
>  	if (!pm8001_alloc(pm8001_ha, ent))
>  		return pm8001_ha;
>  failed_pci_alloc:
> +	pm8001_kill_tasklet(pm8001_ha);
>  	pm8001_free(pm8001_ha);
>  	return NULL;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the memory-region cleanup 
in pm8001_free() violate the DMA API by passing an offset virtual address?

During allocation in pm8001_mem_alloc() (in drivers/scsi/pm8001/pm8001_sas.c),
the virtual pointer is manually aligned:

    *virt_addr = (void *)mem_virt_alloc + phys_align - *pphys_addr;

This modifies the virtual pointer away from the original value returned by 
dma_alloc_coherent(). When pm8001_free() iterates over the memoryMap.region 
array to free the memory, it unconditionally passes this shifted virt_ptr as 
the cpu_addr argument to dma_free_coherent().

The DMA API strictly requires the exact virtual address originally returned by 
dma_alloc_coherent() to be passed when freeing. Can passing this shifted 
pointer result in undefined behavior or a panic from the DMA allocator during 
driver unload or probe failure?

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

[PATCH 2/4] scsi: pm8001: clean up resources on probe failure

[Peng Hao]

When pm8001_init_ccb_tag() fails, pm8001_pci_probe() jumps directly to
the final return label. This bypasses teardown of the IRQs, tasklets,
HBA, SCSI host, SAS host data, PCI regions and PCI device.

Route the failure through the normal cleanup chain and reset the chip
before tearing down an initialized controller, including when
scsi_add_host() fails. Also release the separately allocated sas_phy and
sas_port arrays on failures after SAS host preparation.

Keep the earlier preparation-failure path separate because
pm8001_prep_sas_ha_init() already frees those arrays on failure.

Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 8ede1f1da415..412079b5652c 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -1154,7 +1154,7 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
 	pm8001_ha = pm8001_pci_alloc(pdev, ent, shost);
 	if (!pm8001_ha) {
 		rc = -ENOMEM;
-		goto err_out_free;
+		goto err_out_free_sas_ha;
 	}
 
 	PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha);
@@ -1167,7 +1167,7 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
 
 	rc = pm8001_init_ccb_tag(pm8001_ha);
 	if (rc)
-		goto err_out_enable;
+		goto err_out_chip;
 
 
 	PM8001_CHIP_DISP->chip_post_init(pm8001_ha);
@@ -1184,7 +1184,7 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
 
 	rc = scsi_add_host(shost, &pdev->dev);
 	if (rc)
-		goto err_out_ha_free;
+		goto err_out_chip;
 
 	PM8001_CHIP_DISP->interrupt_enable(pm8001_ha, 0);
 	if (pm8001_ha->chip_id != chip_8001) {
@@ -1216,8 +1216,16 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
 
 err_out_shost:
 	scsi_remove_host(pm8001_ha->shost);
+err_out_chip:
+	PM8001_CHIP_DISP->interrupt_disable(pm8001_ha, 0xFF);
+	PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha);
 err_out_ha_free:
+	pm8001_free_irq(pm8001_ha);
+	pm8001_kill_tasklet(pm8001_ha);
 	pm8001_free(pm8001_ha);
+err_out_free_sas_ha:
+	kfree(sha->sas_phy);
+	kfree(sha->sas_port);
 err_out_free:
 	kfree(sha);
 err_out_free_host:
-- 
2.43.7

Re: [PATCH 2/4] scsi: pm8001: clean up resources on probe failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware soft reset is initiated before tearing down tasklets and IRQs, allowing concurrent MMIO accesses during reset.
- [High] Workqueue flushed after freeing DMA memory and unmapping MMIO registers, leading to use-after-free and use-after-unmap.
--

commit 701715eef41b511a0a6a9cbd9e7c8e9908bfcfc9
Author: Peng Hao <flyingpenghao@gmail.com>
scsi: pm8001: clean up resources on probe failure

This commit improves resource cleanup on probe failures in the pm8001 driver.
It routes failures through the standard teardown chain and correctly resets
the chip. It also adds logic to release SAS arrays when initialization fails.

> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index d0e91ed09bbe9..20b02e63be8c9 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> [ ... ]
> @@ -1216,8 +1216,16 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
>  
>  err_out_shost:
>  	scsi_remove_host(pm8001_ha->shost);
> +err_out_chip:
> +	PM8001_CHIP_DISP->interrupt_disable(pm8001_ha, 0xFF);
> +	PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha);
>  err_out_ha_free:
> +	pm8001_free_irq(pm8001_ha);
> +	pm8001_kill_tasklet(pm8001_ha);

[Severity: High]
This is a pre-existing issue, but does this ordering allow concurrent MMIO
accesses during the hardware soft reset?

In the newly introduced err_out_chip error path of pm8001_pci_probe(),
chip_soft_rst() is called before pm8001_free_irq() and pm8001_kill_tasklet().
If an interrupt fires immediately before interrupt_disable(), a tasklet can
be scheduled.

This tasklet could then execute concurrently on another CPU during the lengthy
soft reset window, accessing chip MMIO registers via pm8001_chip_isr() and
triggering PCI bus errors like a PCI Master Abort.

Could we tear down the tasklets and IRQs before initiating the soft reset?

>  	pm8001_free(pm8001_ha);

[Severity: High]
This is also a pre-existing issue, but can this lead to a use-after-free
or use-after-unmap if a delayed work item executes?

When pm8001_free() is called here, it unmaps MMIO and frees memory before
flushing the workqueue:

pm8001_free() {
    ...
    dma_free_coherent(...)
    PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
    flush_workqueue(pm8001_wq);
}

Because interrupts are enabled earlier in pm8001_pci_probe() prior to
pm8001_init_sas_add(), any work item queued by an interrupt during this
window could execute concurrently with or after the unmapping.

The delayed work item could then write to the now-freed DMA memory and access
unmapped MMIO.

Should the workqueue be flushed before freeing DMA memory and unmapping
the MMIO registers?

> +err_out_free_sas_ha:
> +	kfree(sha->sas_phy);
> +	kfree(sha->sas_port);
>  err_out_free:
>  	kfree(sha);

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

[PATCH 3/4] scsi: pm8001: free CCB resources on allocation failure

[Peng Hao]

If allocation of a CCB PRD buffer fails, pm8001_init_ccb_tag() leaks the
buffers allocated by earlier loop iterations, along with ccb_info and
the reserved-tag bitmap. The same resources, as well as the devices
array, are leaked when probe fails after CCB initialization because
pm8001_free() does not release them.

Add a common CCB cleanup helper and use it from both the partial
allocation error path and pm8001_free(). Move devices teardown into
pm8001_free() and remove the duplicate teardown from the remove path.

Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 42 ++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 412079b5652c..594f8c0595e9 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -176,6 +176,33 @@ static void pm8001_phy_init(struct pm8001_hba_info *pm8001_ha, int phy_id)
 	sas_phy->lldd_phy = phy;
 }
 
+/**
+ * pm8001_free_ccb - free CCB resources
+ * @pm8001_ha:	our hba structure.
+ */
+static void pm8001_free_ccb(struct pm8001_hba_info *pm8001_ha)
+{
+	int i;
+
+	if (pm8001_ha->ccb_info) {
+		for (i = 0; i < pm8001_ha->ccb_count; i++) {
+			if (!pm8001_ha->ccb_info[i].buf_prd)
+				continue;
+
+			dma_free_coherent(pm8001_ha->dev,
+					  sizeof(struct pm8001_prd) *
+					  PM8001_MAX_DMA_SG,
+					  pm8001_ha->ccb_info[i].buf_prd,
+					  pm8001_ha->ccb_info[i].ccb_dma_handle);
+		}
+	}
+	kfree(pm8001_ha->ccb_info);
+	pm8001_ha->ccb_info = NULL;
+	pm8001_ha->ccb_count = 0;
+	bitmap_free(pm8001_ha->rsvd_tags);
+	pm8001_ha->rsvd_tags = NULL;
+}
+
 /**
  * pm8001_free - free hba
  * @pm8001_ha:	our hba structure.
@@ -198,7 +225,8 @@ 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);
+	pm8001_free_ccb(pm8001_ha);
+	kfree(pm8001_ha->devices);
 	kfree(pm8001_ha);
 }
 
@@ -1285,8 +1313,8 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
 	return 0;
 
 err_out_noccb:
-	kfree(pm8001_ha->devices);
 err_out:
+	pm8001_free_ccb(pm8001_ha);
 	return -ENOMEM;
 }
 
@@ -1294,7 +1322,6 @@ static void pm8001_pci_remove(struct pci_dev *pdev)
 {
 	struct sas_ha_struct *sha = pci_get_drvdata(pdev);
 	struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
-	int i;
 
 	sas_unregister_ha(sha);
 	sas_remove_host(pm8001_ha->shost);
@@ -1306,15 +1333,6 @@ static void pm8001_pci_remove(struct pci_dev *pdev)
 	pm8001_kill_tasklet(pm8001_ha);
 	scsi_host_put(pm8001_ha->shost);
 
-	for (i = 0; i < pm8001_ha->ccb_count; i++) {
-		dma_free_coherent(&pm8001_ha->pdev->dev,
-			sizeof(struct pm8001_prd) * PM8001_MAX_DMA_SG,
-			pm8001_ha->ccb_info[i].buf_prd,
-			pm8001_ha->ccb_info[i].ccb_dma_handle);
-	}
-	kfree(pm8001_ha->ccb_info);
-	kfree(pm8001_ha->devices);
-
 	pm8001_free(pm8001_ha);
 	kfree(sha->sas_phy);
 	kfree(sha->sas_port);
-- 
2.43.7

Re: [PATCH 3/4] scsi: pm8001: free CCB resources on allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Out-of-bounds array accesses and integer underflow due to unvalidated device-supplied `max_out_io` limit.
--

commit 54b6fce7eebff69bdb11ab411c836b69e2bfd3d1
Author: Peng Hao <flyingpenghao@gmail.com>

scsi: pm8001: free CCB resources on allocation failure

This commit adds a common CCB cleanup helper to avoid leaking buffers
when CCB PRD allocation fails. It also moves device teardown into
pm8001_free() and removes duplicate teardown logic from the remove path.

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

[ ... ]

> @@ -1285,8 +1313,8 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>  	return 0;
>  
>  err_out_noccb:
> -	kfree(pm8001_ha->devices);
>  err_out:
> +	pm8001_free_ccb(pm8001_ha);
>  	return -ENOMEM;
>  }

[Severity: High]
This is a pre-existing issue, but does deriving ccb_count directly from the
untrusted device configuration without a safe lower bound cause integer
underflow and out-of-bounds accesses?

Earlier in pm8001_init_ccb_tag(), the driver reads max_out_io directly
from the device's MMIO:

	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);

	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;

If the device provides a max_out_io value smaller than PM8001_RESERVE_SLOT,
will shost->can_queue underflow to a negative value, potentially corrupting
block layer limits?

Furthermore, since the ccb_info array is allocated based on this restricted
ccb_count, could this cause out-of-bounds memory accesses during driver
operation? For example, in pm8001_hwi.c:pm8001_work_fn(), the code
unconditionally iterates up to PM8001_MAX_CCB instead of the dynamically
allocated ccb_count:

	for (i = 0; ccb = NULL, i < PM8001_MAX_CCB; i++) {
		ccb = &pm8001_ha->ccb_info[i];
		if ((ccb->ccb_tag != PM8001_INVALID_TAG) &&
		    (ccb->task == t))
			break;
	}

Could an untrusted or compromised PCIe device trigger a kernel panic by
providing a small max_out_io value, causing these loops to iterate past
the bounds of pm8001_ha->ccb_info?

[ ... ]

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

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

[Peng Hao]

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.

Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 594f8c0595e9..65bf10caa327 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -1050,6 +1050,7 @@ static u32 pm8001_request_msix(struct pm8001_hba_info *pm8001_ha)
 					 &pm8001_ha->irq_vector[j]);
 			}
 			pci_free_irq_vectors(pm8001_ha->pdev);
+			pm8001_ha->number_of_intr = 0;
 			break;
 		}
 	}
@@ -1087,6 +1088,12 @@ static u32 pm8001_request_irq(struct pm8001_hba_info *pm8001_ha)
 	}
 
 use_intx:
+	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;
-- 
2.43.7

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

[sashiko-bot]

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