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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Potential deadlock in `plx_dma_process_desc()` due to missing bottom-half disabling.
- [High] `plx_dma_prep_memcpy()` acquires a spinlock and returns with it held, breaking the DMA Engine API contract.
- [High] Unbounded busy-wait loop spanning up to 1000ms inside an RCU read-side critical section.
- [High] Missing `dma_rmb()` in `plx_dma_process_desc()` before processing completion data.
- [Medium] Memory leak of `desc_ring` and its inner descriptor objects in the `plx_dma_alloc_chan_resources()` error path.
--

commit dbf39b912829e6113fcfa314a5a4384936eccf12
Author: Logan Gunthorpe <logang@deltatee.com>
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 commit moves the bar assignment before request_irq() to fix the issue.

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;
> +
>  	dma = &plxdev->dma_dev;
>  	INIT_LIST_HEAD(&dma->channels);
>  	dma_cap_set(DMA_MEMCPY, dma->cap_mask);

While reviewing this file, I noticed a few pre-existing issues in other
functions that might need attention.

> static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
> {
[ ... ]
> 	spin_lock(&plxdev->ring_lock);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does using spin_lock() here instead of
spin_lock_bh() create a deadlock vulnerability? plx_dma_tx_status() calls
this function from process context. If an interrupt fires and triggers the
plx_dma_desc_task tasklet on the same CPU, wouldn't it attempt to acquire
this same lock, deadlocking the system?

[ ... ]
> 	while (plxdev->tail != plxdev->head) {
> 		desc = plx_dma_get_desc(plxdev, plxdev->tail);
> 
> 		flags = le32_to_cpu(READ_ONCE(desc->hw->flags_and_size));
> 
> 		if (flags & PLX_DESC_FLAG_VALID)
> 			break;
            ^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but should there be a dma_rmb()
inserted directly after this break statement? Without a read memory barrier,
weakly-ordered CPUs might speculatively read the destination buffer before
the status read completes, which could cause data corruption.

[ ... ]

> static struct dma_async_tx_descriptor *plx_dma_prep_memcpy(struct dma_chan *c,
> 		dma_addr_t dma_dst, dma_addr_t dma_src, size_t len,
> 		unsigned long flags)
> 	__acquires(plxdev->ring_lock)
> {
[ ... ]
> 	spin_lock_bh(&plxdev->ring_lock);
[ ... ]
> 	/* return with the lock held, it will be released in tx_submit */
> 
> 	return &plxdesc->txd;

[Severity: High]
This isn't a bug introduced by this patch, but does returning with a
spinlock held break the DMA Engine API contract? The API allows clients to
prepare multiple descriptors before submission. If a client prepares two
descriptors sequentially, won't the second call deadlock trying to acquire
ring_lock? Also, if a client prepares a descriptor but aborts without
calling submit, wouldn't the lock leak and permanently leave softirqs
disabled on that CPU?

[ ... ]

> static void plx_dma_stop(struct plx_dma_dev *plxdev)
> {
> 	rcu_read_lock();
[ ... ]
> 	__plx_dma_stop(plxdev);
[ ... ]
> }

and __plx_dma_stop() does:

> static void __plx_dma_stop(struct plx_dma_dev *plxdev)
> {
> 	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 this performs an unbounded delay of up to
1000ms inside the RCU read-side critical section established by
plx_dma_stop(). Could spinning for such a long duration block RCU grace
periods and severely degrade system stability?

[ ... ]

> static int plx_dma_alloc_chan_resources(struct dma_chan *chan)
> {
[ ... ]
> 	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;
                ^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
This isn't a bug introduced by this patch, but does this error path leak
the desc_ring array and its 2048 dynamically allocated descriptor objects?
It jumps to out_free_hw_ring, which frees hw_ring but skips the cleanup
loop needed to destroy desc_ring.

[ ... ]
> out_free_hw_ring:
> 	dma_free_coherent(plxdev->dma_dev.dev, ring_sz, plxdev->hw_ring,
> 			  plxdev->hw_ring_dma);
> 	return rc;
> }

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