Re: [PATCHv4 02/15] dmaengine: fsldma: drop desc_lock before invoking client callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditionally setting `chan->idle = true` inside `fsldma_cleanup_descriptors()` clobbers the active hardware busy state, causing active transfers to be overwritten.
- [High] Dropping the lock without reentrancy protection allows concurrent execution of `fsldma_cleanup_descriptors()`, breaking callback serialization and corrupting `completed_cookie`.
- [High] Dropping the channel lock during descriptor cleanup allows concurrent channel teardown to destroy the descriptor pool, leading to a Use-After-Free.
--

commit 3e8446b448ad00b0206ad968720a0177b8f36855
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: drop desc_lock before invoking client callback

fsldma_run_tx_complete_actions() calls dmaengine_desc_get_callback_invoke()
while still holding chan->desc_lock.  If the client submits a new
transaction from their completion callback, fsl_dma_tx_submit()
tries to acquire the same non-recursive spinlock, causing a
self-deadlock.

Fix by extracting the callback info under the lock, removing the
descriptor from ld_running, dropping the lock, then invoking the
callback and running dependencies outside the lock.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..5c8a8fe25617d 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
[ ... ]
> @@ -635,22 +603,23 @@ static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
>  {
[ ... ]
> +	/*
> +	 * Take descriptors one at a time from the front of the running
> +	 * queue.  We re-read the list each iteration so that we don't
> +	 * chase a stale next pointer across the lock-drop below.
> +	 */
> +	while (!seen_current && !list_empty(&chan->ld_running)) {
[ ... ]
> +		/*
> +		 * stop the search if we reach the current descriptor and the
> +		 * channel is busy
> +		 */
> +		if (desc->async_tx.phys == curr_phys) {
> +			seen_current = 1;
> +			if (!dma_is_idle(chan))
> +				break;
> +		}
>  
[ ... ]
> +		/*
> +		 * Drop the lock before invoking the client callback, since
> +		 * the DMAengine API explicitly allows clients to submit new
> +		 * transactions from their completion callback.  Otherwise
> +		 * we self-deadlock on chan->desc_lock.
> +		 */
> +		spin_unlock(&chan->desc_lock);
> +		dmaengine_desc_callback_invoke(&cb, NULL);
> +		dma_run_dependencies(&desc->async_tx);
> +		spin_lock(&chan->desc_lock);

[Severity: High]
Does dropping the lock here without reentrancy protection allow concurrent
execution of fsldma_cleanup_descriptors()?

If another thread calls fsl_tx_status() concurrently, it could acquire
chan->desc_lock, enter fsldma_cleanup_descriptors(), and execute callbacks
simultaneously. This would break the DMAengine API's serialization guarantee.

> +
> +		chan->idle = true;

[Severity: High]
If the loop breaks out early because the current descriptor is busy
(!dma_is_idle(chan)), chan->idle is still set to true from the previous
iteration while the hardware is actively busy.

Will fsl_chan_xfer_ld_queue() called later in this function evaluate this
corrupted chan->idle state, skip its !chan->idle safety check, and forcefully
overwrite the active transaction by issuing a new DMA start command?

> +
> +		if (!async_tx_test_ack(&desc->async_tx))
> +			list_add_tail(&desc->node, &chan->ld_completed);
> +		else
> +			dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);

[Severity: High]
Could dropping the lock above allow fsl_dma_free_chan_resources() to execute
concurrently, destroying chan->desc_pool and setting it to NULL before we call
dma_pool_free() here?

>  	}
>  
>  	/*
>  	 * Start any pending transactions automatically
>  	 *
>  	 * In the ideal case, we keep the DMA controller busy while we go
>  	 * ahead and free the descriptors below.
>  	 */
>  	fsl_chan_xfer_ld_queue(chan);
>  
>  	if (cookie > 0)
>  		chan->common.completed_cookie = cookie;

[Severity: High]
If concurrent threads execute fsldma_cleanup_descriptors() due to the dropped
lock, can an older local cookie from one thread overwrite a newer one written
by another thread here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611035245.13439-3-rosenp@gmail.com?part=1