Re: [PATCH 6/6] dmaengine: plx_dma: don't invoke client callbacks with ring_lock held

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: plx_dma: premature teardown in plx_dma_free_chan_resources()
- [Medium] dmaengine: plx_dma: race in plx_dma_process_desc() when dropping ring_lock
--

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

dmaengine: plx_dma: don't invoke client callbacks with ring_lock held

This commit updates plx_dma_process_desc() and plx_dma_abort_desc() to copy
and clear descriptor completion callbacks, advance plxdev->tail, and drop
plxdev->ring_lock before invoking the callback to avoid a deadlock when a
callback prepares a new transfer.

Link: https://lore.kernel.org/dmaengine/20260717223217.B6FBF1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
> index 8c4b19d681ad5..17745f7a5658d 100644
> --- a/drivers/dma/plx_dma.c
> +++ b/drivers/dma/plx_dma.c
> @@ -134,6 +134,7 @@ static struct plx_dma_desc *plx_dma_get_desc(struct plx_dma_dev *plxdev, int i)
>  
>  static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
>  {
> +	struct dmaengine_desc_callback cb;
>  	struct dmaengine_result res;
>  	struct plx_dma_desc *desc;
>  	u32 flags;
> @@ -165,11 +166,14 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
>  
>  		dma_cookie_complete(&desc->txd);
>  		dma_descriptor_unmap(&desc->txd);
> -		dmaengine_desc_get_callback_invoke(&desc->txd, &res);
> +		dmaengine_desc_get_callback(&desc->txd, &cb);
>  		desc->txd.callback = NULL;
>  		desc->txd.callback_result = NULL;
> -
>  		plxdev->tail++;
> +
> +		spin_unlock_bh(&plxdev->ring_lock);
> +		dmaengine_desc_callback_invoke(&cb, &res);
> +		spin_lock_bh(&plxdev->ring_lock);

[Severity: Medium]
Can dropping plxdev->ring_lock here after calling dma_cookie_complete() and
incrementing plxdev->tail cause descriptor callbacks to run out of order or
concurrently?

plx_dma_process_desc() is called both from the tasklet plx_dma_desc_task()
and from plx_dma_tx_status():

drivers/dma/plx_dma.c:plx_dma_tx_status() {
	ret = dma_cookie_status(chan, cookie, txstate);
	if (ret == DMA_COMPLETE)
		return ret;

	plx_dma_process_desc(plxdev);

	return dma_cookie_status(chan, cookie, txstate);
}

When plx_dma_tx_status() calls plx_dma_process_desc() in process context and
an interrupt schedules plxdev->desc_task while bottom halves are disabled
under spin_lock_bh(), calling spin_unlock_bh(&plxdev->ring_lock) re-enables
bottom halves and can immediately run plx_dma_desc_task() ->
plx_dma_process_desc() on the same CPU before dmaengine_desc_callback_invoke()
is called for the first descriptor. Because plxdev->tail was already
incremented, the tasklet will process subsequent descriptors and invoke
their callbacks ahead of the earlier descriptor's callback.

Similarly, on SMP, while one caller has dropped plxdev->ring_lock to execute
dmaengine_desc_callback_invoke(), a concurrent caller of plx_dma_tx_status()
on another CPU can enter plx_dma_process_desc(), observe the updated
plxdev->tail, and invoke subsequent callbacks concurrently, or observe
DMA_COMPLETE from dma_cookie_status() while the descriptor's callback is
still running.

>  	}
>  
>  	spin_unlock_bh(&plxdev->ring_lock);
> @@ -177,6 +181,7 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
>  
>  static void plx_dma_abort_desc(struct plx_dma_dev *plxdev)
>  {
> +	struct dmaengine_desc_callback cb;
>  	struct dmaengine_result res;
>  	struct plx_dma_desc *desc;
>  
> @@ -192,11 +197,14 @@ static void plx_dma_abort_desc(struct plx_dma_dev *plxdev)
>  
>  		dma_cookie_complete(&desc->txd);
>  		dma_descriptor_unmap(&desc->txd);
> -		dmaengine_desc_get_callback_invoke(&desc->txd, &res);
> +		dmaengine_desc_get_callback(&desc->txd, &cb);
>  		desc->txd.callback = NULL;
>  		desc->txd.callback_result = NULL;
> -
>  		plxdev->tail++;
> +
> +		spin_unlock_bh(&plxdev->ring_lock);
> +		dmaengine_desc_callback_invoke(&cb, &res);
> +		spin_lock_bh(&plxdev->ring_lock);

[Severity: High]
Does releasing plxdev->ring_lock after advancing plxdev->tail in
plx_dma_process_desc() and plx_dma_abort_desc() allow
plx_dma_free_chan_resources() to finish tearing down the channel while a
callback is still executing on another CPU?

Previously, holding plxdev->ring_lock across the entire loop ensured that
plx_dma_abort_desc() in plx_dma_free_chan_resources() would wait until any
concurrent plx_dma_process_desc() or plx_dma_abort_desc() call finished
invoking all callbacks.

With plxdev->ring_lock dropped around dmaengine_desc_callback_invoke(), if
CPU0 is in plx_dma_remove() -> plx_dma_abort_desc() or plx_dma_tx_status() ->
plx_dma_process_desc() and advances plxdev->tail to plxdev->head before
dropping plxdev->ring_lock, CPU1 can run dma_release_channel() ->
plx_dma_free_chan_resources():

drivers/dma/plx_dma.c:plx_dma_free_chan_resources() {
	...
	tasklet_kill(&plxdev->desc_task);

	plx_dma_abort_desc(plxdev);

	plx_dma_free_desc(plxdev);
	dma_free_coherent(plxdev->dma_dev.dev, ring_sz, plxdev->hw_ring,
			  plxdev->hw_ring_dma);
}

Because tasklet_kill() only waits for plxdev->desc_task, plx_dma_abort_desc()
on CPU1 can acquire plxdev->ring_lock, see plxdev->tail == plxdev->head, and
return immediately while CPU0 is still inside dmaengine_desc_callback_invoke().
plx_dma_free_chan_resources() and dma_release_channel() will then return and
allow the client (such as ntb_transport_free_queue() or ntb_perf) to free the
callback parameter data while the callback is still using it on CPU0.

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