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

[sashiko-bot@kernel.org]

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