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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] API Contract Violation: `plx_dma_prep_memcpy` returns with a spinlock held.
- [High] Deadlock during client callback execution due to held spinlock in `plx_dma_process_desc`.
- [High] Memory leak of the descriptor ring in `plx_dma_alloc_chan_resources` on error path.
- [High] Deadlock due to missing bottom-half disable when taking spinlock in `plx_dma_process_desc`.
- [High] Hardware state corruption due to a 16-bit write to a 32-bit MMIO register (`PLX_REG_CTRL`).
- [Medium] Missing `dma_rmb()` memory barrier after reading the DMA completion flag.
--

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

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

This patch fixes a NULL pointer dereference in plx_dma_isr(). It moves the
bar assignment ahead of request_irq() to ensure the interrupt handler doesn't
run before plxdev->bar is initialized.

Link: https://lore.kernel.org/all/20260610112121.676561-1-jjy600901@snu.ac.kr/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);
>  
>  	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 driver, I noticed a few pre-existing issues in other
functions that might need attention:

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to invoke the
client callback while holding the spinlock in plx_dma_process_desc()?

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

	while (plxdev->tail != plxdev->head) {
		desc = plx_dma_get_desc(plxdev, plxdev->tail);
...
		dmaengine_desc_get_callback_invoke(&desc->txd, &res);
...
	spin_unlock(&plxdev->ring_lock);
}

If a DMA client prepares a new transfer from its completion callback using
dmaengine_prep_dma_memcpy(), it will attempt to re-acquire this same
ring_lock in plx_dma_prep_memcpy(), causing a deadlock. This also affects
plx_dma_abort_desc() which does the same.

[Severity: Critical]
This is a pre-existing issue, but does plx_dma_prep_memcpy() intentionally
return with the spinlock held?

drivers/dma/plx_dma.c:plx_dma_prep_memcpy() {
...
	spin_lock_bh(&plxdev->ring_lock);
...
	/* return with the lock held, it will be released in tx_submit */
	return &plxdesc->txd;
}

If a client prepares multiple descriptors in sequence before submitting them,
the second call to prep_memcpy() will deadlock trying to acquire the lock
that the first call left held. Furthermore, any client that sleeps, allocates
memory with GFP_KERNEL, or takes a mutex between prep and submit will trigger
a scheduling while atomic panic.

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
plx_dma_alloc_chan_resources() leak the descriptor ring?

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;
}

If rcu_dereference(plxdev->pdev) evaluates to false, the code jumps to
out_free_hw_ring. This frees hw_ring, but fails to free the newly allocated
desc_ring array and its descriptors, causing a memory leak.

[Severity: High]
This is a pre-existing issue, but should plx_dma_process_desc() use
spin_lock_bh() instead of spin_lock()?

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

This function can be called from process context via plx_dma_tx_status().
Because bottom halves are left enabled, a softirq can interrupt the lock
holder and schedule the driver's tasklet (plx_dma_desc_task) on the same CPU,
which will attempt to acquire the lock and deadlock.

[Severity: High]
This isn't a bug introduced by this patch, but should plx_dma_issue_pending()
use writel() instead of writew()?

drivers/dma/plx_dma.c:plx_dma_issue_pending() {
...
	writew(PLX_REG_CTRL_START_VAL, plxdev->bar + PLX_REG_CTRL);
...
}

PLX_REG_CTRL is treated as a 32-bit register elsewhere in the driver (e.g. in
__plx_dma_stop). Writing a 16-bit value to a 32-bit MMIO target might cause
partial register updates or undefined hardware behavior.

[Severity: Medium]
This is a pre-existing issue, but is a dma_rmb() memory barrier needed after
reading the descriptor status?

drivers/dma/plx_dma.c:plx_dma_process_desc() {
...
		flags = le32_to_cpu(READ_ONCE(desc->hw->flags_and_size));

		if (flags & PLX_DESC_FLAG_VALID)
			break;
...
}

On architectures with relaxed read ordering, reading the completion flag
without a subsequent dma_rmb() might allow the CPU to speculatively read
stale payload data from before the DMA transfer was actually completed by
the hardware.

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