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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Deadlock in `plx_dma_process_desc()` because it uses `spin_lock()` instead of `spin_lock_bh()`, allowing preemption by softirqs/tasklets.
- [High] Recursive deadlock in `plx_dma_process_desc()` and `plx_dma_abort_desc()` due to invoking client callbacks while holding `ring_lock`.
--

--- Patch [11]: [PATCH v2 11/11] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() ---
commit eb45a79e5c7876e25dd34346cbd03538a3ed67e0
Author: Logan Gunthorpe <logang@deltatee.com>

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

This commit moves the plxdev->bar initialization before calling
request_irq(). This prevents a NULL pointer dereference that could occur if
an interrupt fires immediately and plx_dma_isr() attempts to access the
uninitialized bar.

Link: https://lore.kernel.org/all/20260610112121.676561-1-jjy600901@snu.ac.kr

> 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);
>  
>  	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);
> +	if (rc)
> +		goto free_plx;
> +

[Severity: High]
This is a pre-existing issue, but can this code cause a deadlock?

When plx_dma_tx_status() is called from process context with softirqs enabled,
it invokes plx_dma_process_desc() which acquires ring_lock using spin_lock():

drivers/dma/plx_dma.c:plx_dma_tx_status() {
    ...
    plx_dma_process_desc(plxdev);
    ...
}

drivers/dma/plx_dma.c:plx_dma_process_desc() {
    ...
    spin_lock(&plxdev->ring_lock);
    ...
}

If a hardware interrupt occurs on the same CPU while the lock is held, the IRQ
handler schedules the desc_task tasklet. When the IRQ returns, the softirq
handler executes the tasklet on the same CPU, which also calls
plx_dma_process_desc() and attempts to acquire the already-held lock.

Should plx_dma_process_desc() use spin_lock_bh() instead to disable softirqs?

[Severity: High]
This is a pre-existing issue, but does invoking the client callback while
holding ring_lock cause a recursive deadlock?

Looking at plx_dma_process_desc(), the client callback is invoked via
dmaengine_desc_get_callback_invoke() while the lock is still held:

drivers/dma/plx_dma.c:plx_dma_process_desc() {
    ...
    spin_lock(&plxdev->ring_lock);
    ...
    dmaengine_desc_get_callback_invoke(&desc->txd, &res);
    ...
}

This also happens during abort:

drivers/dma/plx_dma.c:plx_dma_abort_desc() {
    ...
    dmaengine_desc_get_callback_invoke(&desc->txd, &res);
    ...
}

If the DMA client's completion callback prepares and submits chained DMA
transactions by calling dmaengine_prep_dma_memcpy(), it re-enters the driver:

drivers/dma/plx_dma.c:plx_dma_prep_memcpy() {
    ...
    spin_lock_bh(&plxdev->ring_lock);
    ...
}

Could this attempt to re-acquire the non-recursive ring_lock on the same CPU
cause a hard lockup? Should the lock be dropped before invoking the callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721155739.62120-1-logang@deltatee.com?part=11