Re: [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

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

Pre-existing issues:
- [High] Softirq deadlock due to using `spin_lock()` instead of `spin_lock_bh()` in `plx_dma_process_desc()`.
- [High] Missing check for surprise removal (0xffff) in interrupt handler causes IRQ storms on shared legacy interrupts.
--

--- Patch [6]: [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() ---
commit 80fb3e2fa6bbdc673ef54e009946f1184a4c94ca
Author: Logan Gunthorpe <[email protected]>

dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

plx_dma_create() registered the interrupt handler with request_irq()
before initializing plxdev->bar. This patch moves the bar assignment
ahead of request_irq() so everything is initialized before the handler
can run.

Link: https://lore.kernel.org/all/[email protected]/T/#u

> diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
> index 84941a918b01f..409898e92c320 100644
> --- a/drivers/dma/plx_dma.c
> +++ b/drivers/dma/plx_dma.c
> @@ -504,17 +504,17 @@ static int plx_dma_create(struct pci_dev *pdev)
>  	if (!plxdev)
>  		return -ENOMEM;
>  
> -	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
> -			 KBUILD_MODNAME, plxdev);
> -	if (rc)
> -		goto free_plx;
> -
>  	spin_lock_init(&plxdev->ring_lock);
>  	tasklet_setup(&plxdev->desc_task, plx_dma_desc_task);

[Severity: High]
This is a pre-existing issue, but could this lock and tasklet setup lead to a
softirq deadlock?

Looking at plx_dma_process_desc(), it acquires plxdev->ring_lock using
spin_lock() without disabling softirqs. If called from process context, and a
hardware interrupt occurs on the same CPU, the ISR will execute and schedule
this desc_task tasklet.

Upon returning from the ISR, the kernel will execute the softirq on the same
CPU, which attempts to acquire the same ring_lock in plx_dma_desc_task(),
deadlocking the CPU.

Should plx_dma_process_desc() be using spin_lock_bh() instead?

>  
>  	RCU_INIT_POINTER(plxdev->pdev, pdev);
>  	plxdev->bar = pcim_iomap_table(pdev)[0];
>  
> +	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
> +			 KBUILD_MODNAME, plxdev);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the ISR hooked
up here, could a surprise removal of the PCI device cause an IRQ storm?

If the device is removed, MMIO reads from plxdev->bar + PLX_REG_INTR_STATUS
in plx_dma_isr() will return 0xffff. Because 0xffff is non-zero, it bypasses
the !status check, writes 0xffff back, and returns IRQ_HANDLED.

Since IRQ_HANDLED is returned, the interrupt subsystem assumes the interrupt
was serviced. If the IRQ line is shared, it will immediately re-trigger,
causing an infinite loop.

Should the ISR explicitly check for 0xffff and return IRQ_NONE?

> +	if (rc)
> +		goto free_plx;
> +
>  	dma = &plxdev->dma_dev;

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