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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Softirq preemption deadlock in `plx_dma_process_desc()` due to using `spin_lock()` instead of `spin_lock_bh()`.
- [High] Recursive deadlock in `plx_dma_process_desc()` when a client callback submits a new DMA transaction.
- [High] `plx_dma_prep_memcpy()` acquires a spinlock and returns without releasing it, violating the DMA API contract.
--

commit eba3f5594a9491de8ee08b096a4a7e95dd48da19
Author: Logan Gunthorpe <logang@deltatee.com>

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

This commit prevents a NULL pointer dereference in the interrupt handler.
It achieves this by moving the assignment of plxdev->bar ahead of
request_irq() during hardware initialization.

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
[ ... ]
> static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
> {
> 	struct dmaengine_result res;
> 	struct plx_dma_desc *desc;
> 	u32 flags;
> 
> 	spin_lock(&plxdev->ring_lock);

[Severity: High]
This is a pre-existing issue, but does using spin_lock() here instead of
spin_lock_bh() leave us vulnerable to a softirq preemption deadlock?

If plx_dma_tx_status() is called from process context, it will call
plx_dma_process_desc() and acquire this lock while leaving softirqs enabled.
If a hardware interrupt occurs on the same CPU while the lock is held,
plx_dma_isr() will schedule the plx_dma_desc_task tasklet.

Upon return from the interrupt, the softirq executes the tasklet, which calls
plx_dma_process_desc() again. Can this result in a recursive deadlock when it
attempts to acquire the already-held ring_lock?

[ ... ]
> 		dmaengine_desc_get_callback_invoke(&desc->txd, &res);

[Severity: High]
This isn't a bug introduced by this patch, but does invoking the client
callback while holding the internal spinlock expose us to a recursive deadlock?

It is a common DMA engine pattern for client callbacks to chain descriptor
submissions. If the callback submits a new DMA transaction, it will call:
plx_dma_prep_memcpy() -> spin_lock_bh(&plxdev->ring_lock)

Since the lock is already actively held by plx_dma_process_desc() on the same
CPU, will this cause an immediate deadlock?

[ ... ]
> 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)
> {
> 	struct plx_dma_dev *plxdev = chan_to_plx_dma_dev(c);
> 	struct plx_dma_desc *plxdesc;
> 
> 	spin_lock_bh(&plxdev->ring_lock);
[ ... ]
> 	/* return with the lock held, it will be released in tx_submit */
> 
> 	return &plxdesc->txd;

[Severity: High]
This is a pre-existing issue, but does returning to the caller with the
spinlock held and bottom halves disabled violate the DMA API contract?

If a client driver prepares multiple descriptors sequentially, such as calling
dmaengine_prep_dma_memcpy() twice before submitting, won't it deadlock on the
second call?

Additionally, if the client encounters an error between preparation and
submission and never calls tx_submit(), will the lock be permanently leaked,
leaving softirqs disabled on that CPU?

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