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

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

Pre-existing issues:
- [High] Lock is held across DMA API boundaries (`prep` to `submit`), preventing descriptor batching and causing deadlocks.
- [High] Recursive deadlock when invoking client callbacks from `plx_dma_process_desc`.
- [High] Softirq preemption deadlock in `plx_dma_process_desc` due to missing `spin_lock_bh`.
- [High] 1-Second busy-wait loop inside an RCU read-side critical section.
- [Medium] Memory leak of `desc_ring` array and descriptors in `plx_dma_alloc_chan_resources` error path.
--

--- Patch [12]: [PATCH v4 12/12] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() ---
commit 255ac134d3a6a355b0f9fd6a9df9b4e6a75d09d7
Author: Logan Gunthorpe <[email protected]>
Subject: 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. If the device raised an interrupt in
that window, plx_dma_isr() would dereference the still-NULL bar.
This patch moves the bar assignment ahead of request_irq() so everything the
handler can touch is initialized before it can run.

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

> 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;
> +
>  	dma = &plxdev->dma_dev;

[ ... ]

While reviewing this patch, I noticed a few pre-existing issues in the same
file that might be worth addressing:

In drivers/dma/plx_dma.c:plx_dma_prep_memcpy():
> 	spin_lock_bh(&plxdev->ring_lock);
> ...
> 	return &plxdesc->txd;

In drivers/dma/plx_dma.c:plx_dma_tx_submit():
> 	spin_unlock_bh(&plxdev->ring_lock);
> 
> 	return cookie;

[Severity: High]
This is a pre-existing issue, but does holding ring_lock across DMA API
boundaries prevent descriptor batching?

If a client prepares two consecutive transfers before submitting them, wouldn't
the second plx_dma_prep_memcpy() call recursively attempt to acquire the
non-recursive ring_lock held by the first call, causing an immediate deadlock?

In drivers/dma/plx_dma.c:plx_dma_process_desc():
> 	spin_lock(&plxdev->ring_lock);
> 
> 	while (plxdev->tail != plxdev->head) {
> ...
> 		dmaengine_desc_get_callback_invoke(&desc->txd, &res);
> ...
> 	}
> 
> 	spin_unlock(&plxdev->ring_lock);

[Severity: High]
This is a pre-existing issue, but is it safe to invoke client callbacks while
holding ring_lock?

If a completion callback attempts to prepare a new transfer via
dmaengine_prep_dma_memcpy(), it will invoke plx_dma_prep_memcpy() which
attempts to acquire spin_lock_bh(&plxdev->ring_lock).

Would this cause an immediate recursive deadlock?

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

[Severity: High]
This is a pre-existing issue, but could acquiring ring_lock without disabling
softirqs lead to a preemption deadlock?

If plx_dma_process_desc() is called from process context via
plx_dma_tx_status(), a hardware interrupt could preempt the thread while the
lock is held and schedule the tasklet.

Would the softirq tasklet then spin forever in plx_dma_desc_task() trying
to acquire ring_lock?

In drivers/dma/plx_dma.c:plx_dma_stop():
> 	rcu_read_lock();
> 	if (!rcu_dereference(plxdev->pdev)) {
> 		rcu_read_unlock();
> 		return;
> 	}
> 
> 	__plx_dma_stop(plxdev);
> 
> 	rcu_read_unlock();

In drivers/dma/plx_dma.c:__plx_dma_stop():
> 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> ...
> 	while (!time_after(jiffies, timeout)) {
> 		val = readl(plxdev->bar + PLX_REG_CTRL);
> 		if (val & PLX_REG_CTRL_GRACEFUL_PAUSE_DONE)
> 			break;
> 
> 		cpu_relax();
> 	}

[Severity: High]
This is a pre-existing issue, but does __plx_dma_stop() spin for up to a second
inside an RCU read-side critical section?

Since plx_dma_stop() takes rcu_read_lock() and calls __plx_dma_stop(), waiting
for up to 1000 milliseconds using cpu_relax() could cause massive scheduling
latency spikes.

Could this explicitly block RCU grace periods and preemptions on
non-PREEMPT_RCU systems, potentially triggering RCU stall panics?

In drivers/dma/plx_dma.c:plx_dma_alloc_chan_resources():
> 	rc = plx_dma_alloc_desc(plxdev);
> 	if (rc)
> 		goto out_free_hw_ring;
> 
> 	rcu_read_lock();
> 	if (!rcu_dereference(plxdev->pdev)) {
> 		rcu_read_unlock();
> 		rc = -ENODEV;
> 		goto out_free_hw_ring;
> 	}
> ...
> out_free_hw_ring:
> 	dma_free_coherent(plxdev->dma_dev.dev, ring_sz, plxdev->hw_ring,
> 			  plxdev->hw_ring_dma);
> 	return rc;

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the desc_ring array
and its associated descriptors?

If the check for rcu_dereference(plxdev->pdev) fails, it jumps to
out_free_hw_ring.

Since this only frees the DMA coherent memory hw_ring, are the allocated
descriptors completely leaked?

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