Re: [PATCH 2/7] dmaengine: dw-edma: Terminate STOP requests without callbacks

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete synchronization in dw_edma_device_synchronize leads to memory corruption and descriptor leaks.

Pre-existing issues:
- [High] dw_edma_device_terminate_all fails to clean up pending virt-dma descriptors.
- [High] Lockless state mutation in dw_edma_device_terminate_all causes permanent channel deadlock.
--

--- Patch [2]: [PATCH 2/7] dmaengine: dw-edma: Terminate STOP requests without callbacks ---
commit 8726b379b2e15e03e4a564512f4646c2031734e5
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Terminate STOP requests without callbacks

This commit fixes completion handling for stopped descriptors by moving
them to the terminated list instead of scheduling callbacks, avoiding
potential use-after-free bugs. It also introduces a synchronize callback
to allow virt-dma to release the terminated descriptors.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -673,8 +680,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  			break;
>  
>  		case EDMA_REQ_STOP:
> -			list_del(&vd->node);
> -			vchan_cookie_complete(vd);
> +			dw_edma_terminate_vdesc(vd);
>  			chan->request = EDMA_REQ_NONE;
>  			chan->status = EDMA_ST_IDLE;
>  			break;

[Severity: High]
This is a pre-existing issue, but does dw_edma_device_terminate_all() clean
up pending virt-dma descriptors properly?

If a client queues multiple descriptors and terminates the channel before
completion:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_terminate_all() {
    ...
    } else {
        chan->request = EDMA_REQ_STOP;
    }

The driver sets chan->request = EDMA_REQ_STOP, but the interrupt handler
processes EDMA_REQ_STOP for a single active descriptor via vchan_next_desc().

This appears to leave the remaining pending descriptors untouched in the
virt-dma queues, rather than clearing them out using
vchan_get_all_descriptors(). When the client later issues a new transfer,
won't these stale descriptors be executed unexpectedly and potentially
corrupt data?

[Severity: High]
This is also a pre-existing issue, but does dw_edma_device_terminate_all()
mutate channel state safely?

dw_edma_device_terminate_all() reads chan->status and writes chan->request
without holding chan->vc.lock:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_terminate_all() {
    ...
    } else if (chan->status == EDMA_ST_IDLE) {
        chan->configured = false;
    ...
    } else {
        chan->request = EDMA_REQ_STOP;
    }

If dw_edma_done_interrupt() concurrently finishes a transfer, acquires the
lock, and sets chan->status = EDMA_ST_IDLE, could the following race occur:

1. terminate_all reads chan->status as EDMA_ST_BUSY.
2. done_interrupt finishes, setting chan->status = EDMA_ST_IDLE and releasing
   the lock.
3. terminate_all writes chan->request = EDMA_REQ_STOP.

The channel is now left with status == EDMA_ST_IDLE and
request == EDMA_REQ_STOP.

Will subsequent calls to dw_edma_device_issue_pending() silently refuse
to start new transfers because it requires chan->request == EDMA_REQ_NONE?

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_issue_pending() {
    ...
    if (vchan_issue_pending(&chan->vc) && chan->request == EDMA_REQ_NONE &&
        chan->status == EDMA_ST_IDLE) {
        chan->status = EDMA_ST_BUSY;
        dw_edma_start_transfer(chan);
    }
    ...
}

[ ... ]
> @@ -856,6 +862,13 @@ static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
>  	return 0;
>  }
>  
> +static void dw_edma_device_synchronize(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	vchan_synchronize(&chan->vc);
> +}

[Severity: High]
Does vchan_synchronize() wait for the hardware transfer to actually stop?

If a client calls dmaengine_terminate_sync(), the driver asynchronously
requests a stop via chan->request = EDMA_REQ_STOP but doesn't halt the
hardware. 

If this function only calls vchan_synchronize(), it will return immediately
because the descriptor is still running. The client might then free or reuse
the DMA buffer while the hardware is still writing to it, which could corrupt
memory.

Additionally, when the interrupt later fires, it will terminate the active
descriptor and add it to the terminated list:

        case EDMA_REQ_STOP:
            dw_edma_terminate_vdesc(vd);

Since vchan_synchronize() has already returned, won't this descriptor be
permanently leaked on the terminated list?

Should this wait until chan->status == EDMA_ST_IDLE before calling
vchan_synchronize()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710080903.2392888-1-den@valinux.co.jp?part=2