* Re: [PATCH] scsi: pm8001: unwind the probe on ccb tag allocation failure
2026-11-04 2:14 [PATCH] scsi: pm8001: unwind the probe on ccb tag allocation failure Wentao Liang
@ 2026-11-04 2:26 ` sashiko-bot
2026-11-04 8:33 ` Jinpu Wang
1 sibling, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-11-04 2:26 UTC (permalink / raw)
To: Wentao Liang; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: pm8001: use-after-free via active IRQs in pm8001_pci_probe()
- [High] scsi: pm8001: DMA regions freed while MPI is active in pm8001_pci_probe()
- [Medium] scsi: pm8001: memory leaks on ccb tag failure unwind in pm8001_pci_probe()
--
commit 06b680982a72c843fb06984b05c4494852d0cf98
Author: Wentao Liang <vulab@iscas.ac.cn>
scsi: pm8001: unwind the probe on ccb tag allocation failure
This commit changes the error handling path in pm8001_pci_probe() when
pm8001_init_ccb_tag() fails, jumping to err_out_ha_free instead of
err_out_enable to release previously allocated resources.
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 5af81c73a8f87..389649b36aabd 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1178,7 +1178,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_ha_free;
[Severity: High]
Can jumping to err_out_ha_free here leave active interrupt handlers
registered with dangling pointers into freed pm8001_ha and sha memory?
Before pm8001_init_ccb_tag() runs, pm8001_pci_probe() calls
pm8001_pci_alloc() -> pm8001_init_tasklet() and pm8001_alloc() ->
pm8001_request_irq().
In MSI-X mode, pm8001_request_msix() registers IRQ handlers pointing into
pm8001_ha:
drivers/scsi/pm8001/pm8001_init.c:pm8001_request_msix() {
...
rc = request_irq(pci_irq_vector(pm8001_ha->pdev, i),
pm8001_interrupt_handler_msix, flag,
pm8001_ha->intr_drvname[i],
&(pm8001_ha->irq_vector[i]));
...
}
In INT-X mode, pm8001_request_irq() registers a shared IRQ handler with
pm8001_ha->name and sha:
drivers/scsi/pm8001/pm8001_init.c:pm8001_request_irq() {
...
return request_irq(pdev->irq, pm8001_interrupt_handler_intx,
IRQF_SHARED, pm8001_ha->name,
SHOST_TO_SAS_HA(pm8001_ha->shost));
}
When pm8001_init_ccb_tag() fails, the err_out_ha_free path calls
pm8001_free(pm8001_ha) and kfree(sha) without calling
pm8001_free_irq(pm8001_ha) or pm8001_kill_tasklet(pm8001_ha) (unlike
pm8001_pci_remove()):
drivers/scsi/pm8001/pm8001_init.c:pm8001_pci_probe() {
...
err_out_ha_free:
pm8001_free(pm8001_ha);
err_out_free:
kfree(sha);
...
}
If a shared INT-X interrupt fires afterwards, pm8001_interrupt_handler_intx()
will dereference the freed sha and pm8001_ha structures and read unmapped MMIO
registers, and reading /proc/interrupts will dereference the freed devname
strings in pm8001_ha->intr_drvname[i] or pm8001_ha->name.
[Severity: High]
Also on the err_out_ha_free path, does pm8001_free() release coherent DMA
memory while the controller MPI engine is still active?
In pm8001_pci_probe(), PM8001_CHIP_DISP->chip_init(pm8001_ha) succeeds right
before pm8001_init_ccb_tag() is called, which programs the physical DMA
addresses of pm8001_ha->memoryMap.region[] (inbound/outbound queues,
producer/consumer indices, and firmware event logs) into the controller and
enables the firmware MPI state machine via mpi_init_check().
When pm8001_init_ccb_tag() fails and jumps to err_out_ha_free, pm8001_free()
immediately frees those DMA regions without first calling
PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha) to uninitialize the MPI engine and
reset the chip (as pm8001_pci_remove() does before calling pm8001_free()):
drivers/scsi/pm8001/pm8001_init.c:pm8001_free() {
...
for (i = 0; i < USI_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);
}
}
PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
...
}
Should chip_soft_rst() be called before pm8001_free() so the controller cannot
DMA into the freed memoryMap.region[] buffers?
[Severity: Medium]
Finally, does unwinding to err_out_ha_free on pm8001_init_ccb_tag() failure
still leak sha->sas_phy, sha->sas_port, pm8001_ha->devices,
pm8001_ha->ccb_info, and partially allocated pm8001_ha->ccb_info[i].buf_prd
DMA buffers?
Inside pm8001_init_ccb_tag(), if bitmap_zalloc() for pm8001_ha->rsvd_tags
fails, or if dma_alloc_coherent() for pm8001_ha->ccb_info[i].buf_prd fails at
iteration i, it jumps to err_out:
drivers/scsi/pm8001/pm8001_init.c:pm8001_init_ccb_tag() {
...
pm8001_ha->rsvd_tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
if (!pm8001_ha->rsvd_tags)
goto err_out;
...
for (i = 0; i < ccb_count; i++) {
pm8001_ha->ccb_info[i].buf_prd = dma_alloc_coherent(dev,
sizeof(struct pm8001_prd) * PM8001_MAX_DMA_SG,
&pm8001_ha->ccb_info[i].ccb_dma_handle,
GFP_KERNEL);
if (!pm8001_ha->ccb_info[i].buf_prd) {
pm8001_dbg(pm8001_ha, FAIL,
"ccb prd memory allocation error\n");
goto err_out;
}
...
}
return 0;
err_out_noccb:
kfree(pm8001_ha->devices);
err_out:
return -ENOMEM;
}
Jumping to err_out bypasses kfree(pm8001_ha->devices) (allocated in
pm8001_alloc()) and leaves pm8001_ha->ccb_info and any already allocated
pm8001_ha->ccb_info[0..i-1].buf_prd buffers unfreed. Neither pm8001_free() nor
err_out_free frees those allocations, nor do they free sha->sas_phy and
sha->sas_port (allocated in pm8001_prep_sas_ha_init()) before kfree(sha) is
called.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261104021411.1467970-1-vulab@iscas.ac.cn?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] scsi: pm8001: unwind the probe on ccb tag allocation failure
2026-11-04 2:14 [PATCH] scsi: pm8001: unwind the probe on ccb tag allocation failure Wentao Liang
2026-11-04 2:26 ` sashiko-bot
@ 2026-11-04 8:33 ` Jinpu Wang
2026-11-04 8:41 ` Krzysztof Kozlowski
1 sibling, 1 reply; 6+ messages in thread
From: Jinpu Wang @ 2026-11-04 8:33 UTC (permalink / raw)
To: Wentao Liang; +Cc: James E.J. Bottomley, Martin K. Petersen, linux-scsi
On Wed, Nov 4, 2026 at 3:14 AM Wentao Liang <vulab@iscas.ac.cn> wrote:
>
> pm8001_init_ccb_tag() is called after the Scsi_Host, the sas_ha and the
> driver's private data have been allocated, and after the PCI regions
> have been requested. When it fails, pm8001_pci_probe() jumps to
> err_out_enable, which only returns the error: the Scsi_Host, sas_ha and
> private data are leaked, the PCI regions stay requested and the device
> stays enabled.
>
> Jump to err_out_ha_free instead, which releases all of those resources.
>
> Fixes: 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding I/O supported to 1024")
> Cc: stable@vger.kernel.org
> Assisted-by: DeepSeek-V4.1-Flash and OpenAI Codex
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
Acked-by: Jack Wang <jinpu.wang@cloud.ionos.com>
Thx!
> ---
> drivers/scsi/pm8001/pm8001_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 5af81c73a8f8..389649b36aab 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1178,7 +1178,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_ha_free;
>
>
> PM8001_CHIP_DISP->chip_post_init(pm8001_ha);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread