Re: [PATCH v2 1/4] dmaengine: dw: Prevent tx-status calling DMA-desc callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] dmaengine: dw: premature completion in dwc_scan_descriptors() on zero residue
- [High] dmaengine: dw: polled and non-interrupt transfers time out in dwc_tx_status()
--

commit 0cd2ee9537c620b0eff806ffd2af94c36f805f91
Author: Serge Semin <fancer.lancer@gmail.com>

dmaengine: dw: Prevent tx-status calling DMA-desc callback

This commit removes the dwc_scan_descriptors() call from dwc_tx_status() to
avoid invoking descriptor completion callbacks while holding consumer locks
such as the 8250 UART port lock. Instead, it introduces helper functions to
compute the residue directly for soft-LLP and hard-LLP transfers and uses
dwc_get_hard_llp_desc_residue() in both dwc_tx_status() and
dwc_scan_descriptors().

> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index dd75f97a33b3d..af1871646eb9e 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c

[ ... ]

> @@ -358,41 +358,16 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  		return;
>  	}
>  
> -	dev_vdbg(chan2dev(&dwc->chan), "%s: llp=%pad\n", __func__, &llp);
> +	dev_vdbg(chan2dev(&dwc->chan), "%s: hard LLP mode\n", __func__);
>  
>  	list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
> -		/* Initial residue value */
> -		desc->residue = desc->total_len;
> -
> -		/* Check first descriptors addr */
> -		if (desc->txd.phys == DWC_LLP_LOC(llp)) {
> -			spin_unlock_irqrestore(&dwc->lock, flags);
> -			return;
> -		}
> -
> -		/* Check first descriptors llp */
> -		if (lli_read(desc, llp) == llp) {
> -			/* This one is currently in progress */
> -			desc->residue -= dwc_get_sent(dwc);
> +		desc->residue = dwc_get_hard_llp_desc_residue(dwc, desc);
> +		if (desc->residue) {
>  			spin_unlock_irqrestore(&dwc->lock, flags);
>  			return;
>  		}
>  
> -		desc->residue -= desc->len;
> -		list_for_each_entry(child, &desc->tx_list, desc_node) {
> -			if (lli_read(child, llp) == llp) {
> -				/* Currently in progress */
> -				desc->residue -= dwc_get_sent(dwc);
> -				spin_unlock_irqrestore(&dwc->lock, flags);
> -				return;
> -			}
> -			desc->residue -= child->len;
> -		}
> -
> -		/*
> -		 * No descriptors so far seem to be in progress, i.e.
> -		 * this one must be done.
> -		 */
> +		/* No data left to be send. Finalize the transfer then */
>  		spin_unlock_irqrestore(&dwc->lock, flags);
>  		dwc_descriptor_complete(dwc, desc, true);
>  		spin_lock_irqsave(&dwc->lock, flags);

[Severity: Critical]
Can checking only if (desc->residue) here cause an in-progress descriptor to
be completed and freed prematurely while still linked on dwc->active_list?

Previously, dwc_scan_descriptors() returned immediately whenever an LLI
matched the channel LLP register (desc->txd.phys == DWC_LLP_LOC(llp) or
lli_read(..., llp) == llp), even if desc->residue -= dwc_get_sent(dwc)
evaluated to 0.

In the DesignWare AHB DMAC, dwc_get_sent(dwc) reads CTL_HI (BLOCK_TS), which
counts bytes fetched from the source into the channel FIFO. On a single-block
descriptor or the final LLI of a multi-block descriptor (where lli.llp == 0
and hardware LLP == 0), once all source bytes have been read into the FIFO,
dwc_get_sent(dwc) equals the block length and dwc_get_hard_llp_desc_residue()
returns 0 while destination writes are still draining from the FIFO
(RAW.XFER == 0) or when a destination bus error (RAW.ERROR) halts the channel
with RAW.XFER == 0.

When dwc_get_hard_llp_desc_residue() returns 0, dwc_scan_descriptors() now
falls through to dwc_descriptor_complete(dwc, desc, true), which sets
desc->txd.cookie = 0 via dma_cookie_complete() and frees desc (and its
tx_list children) to dw->desc_pool via dwc_desc_put() without removing
desc->desc_node from dwc->active_list. It then disables the channel via
dwc_chan_disable() and calls dwc_dostart_first_queued(dwc) with the freed
descriptor still on dwc->active_list.

In particular, when reached via dw_dma_tasklet()->dwc_handle_error():

drivers/dma/dw/core.c:dwc_handle_error() {
	dwc_scan_descriptors(dw, dwc);

	spin_lock_irqsave(&dwc->lock, flags);

	bad_desc = dwc_first_active(dwc);
	list_del_init(&bad_desc->desc_node);
	list_move(dwc->queue.next, dwc->active_list.prev);
	...
	dwc_descriptor_complete(dwc, bad_desc, true);
}

dwc_handle_error() will pop the already-freed bad_desc from dwc->active_list,
dereference it in dwc_dump_lli() (use-after-free), and pass it to
dwc_descriptor_complete() a second time, triggering
BUG_ON(tx->cookie < DMA_MIN_COOKIE) in dma_cookie_complete() and
double-freeing bad_desc to dw->desc_pool.

Note that while a later commit in this series ("dmaengine: dw: serialize
error recovery with channel termination") removes the dwc_scan_descriptors()
call from dwc_handle_error(), the if (desc->residue) check and missing
list_del(&desc->desc_node) in dwc_scan_descriptors() remain and can still be
reached from dw_dma_tasklet()->dwc_scan_descriptors() if a concurrent
dwc_terminate_all() and transfer restart clear RAW.XFER before
dwc_scan_descriptors() acquires dwc->lock.

[ ... ]

> @@ -1012,12 +1028,6 @@ dwc_tx_status(struct dma_chan *chan,
>  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
>  	enum dma_status		ret;
>  
> -	ret = dma_cookie_status(chan, cookie, txstate);
> -	if (ret == DMA_COMPLETE)
> -		return ret;
> -
> -	dwc_scan_descriptors(to_dw_dma(chan->device), dwc);
> -
>  	ret = dma_cookie_status(chan, cookie, txstate);
>  	if (ret == DMA_COMPLETE)
>  		return ret;

[Severity: High]
What happens for polled transfers or descriptors prepared without
DMA_PREP_INTERRUPT now that dwc_tx_status() no longer calls
dwc_scan_descriptors()?

In drivers/dma/dw/core.c, both dwc_prep_dma_memcpy() and dwc_prep_slave_sg()
only enable the descriptor completion interrupt when DMA_PREP_INTERRUPT is
set:

	if (flags & DMA_PREP_INTERRUPT)
		/* Trigger interrupt after last block */
		lli_set(prev, ctllo, DWC_CTLL_INT_EN);

When DMA_PREP_INTERRUPT is not set in hard-LLP mode (or when DMAC interrupts
are masked at the wrapper level), the controller sets RAW.XFER when the
transfer finishes, but never raises an interrupt to run dw_dma_interrupt()
and dw_dma_tasklet(). Without dwc_scan_descriptors() in dwc_tx_status(),
chan->completed_cookie is never updated, completed descriptors remain on
dwc->active_list, and dwc_tx_status() continues to return DMA_IN_PROGRESS
even after the residue reaches 0.

Several in-tree callers rely on polling completion without
DMA_PREP_INTERRUPT:

1. sound/soc/intel/catpt/dsp.c:catpt_dma_memcpy() prepares memcpy transfers
with DMA_CTRL_ACK (and with DMAC interrupts masked in CATPT_SHIM_IMC) and
waits via dma_wait_for_async_tx():

	desc = dmaengine_prep_dma_memcpy(chan, dst_addr, src_addr, size,
					 DMA_CTRL_ACK);
	...
	status = dma_wait_for_async_tx(desc);
	ret = (status == DMA_COMPLETE) ? 0 : -EPROTO;

When dma_wait_for_async_tx() times out and returns -EPROTO to
catpt_request_dma_load_firmware(), releasing the channel via
dma_release_channel()->dwc_free_chan_resources() then hits
BUG_ON(!list_empty(&dwc->active_list)).

2. drivers/dma/dmatest.c:dmatest_func() in polled mode prepares transfers
with flags = DMA_CTRL_ACK and waits via dma_sync_wait().

3. crypto/async_tx/async_memcpy.c:async_memcpy() omits DMA_PREP_INTERRUPT
when submit->cb_fn is NULL.

Could dwc_tx_status() still finalize descriptors that did not request an
interrupt (or when no callback is registered), or otherwise update completion
state for polled transfers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-submit-dw-dma-synchronize-v1-v2-0-5244e94dca8c@gmail.com?part=1