Re: [PATCH v4 02/10] dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing locking in `dw_edma_device_resume` leads to list corruption and UAF when racing with other dmaengine callbacks or interrupts.
- [High] `dw_edma_device_terminate_all` and `dw_edma_free_chan_resources` fail to reap and free pending descriptors, leading to memory leaks.
--

--- Patch [2]: [PATCH v4 02/10] dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk ---
commit c69420a16cf9075d2716bbeed5e9f214096073f7
Author: Frank Li <Frank.Li@nxp.com>
Subject: dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk

This commit adds a new xfer_sz field to struct dw_edma_chunk to explicitly
indicate the total transfer size of a chunk. This replaces the reuse of
ll_region.sz, which represents the memory size of the EDMA link list.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 1fec1b52e3d47..53469c8c8b82e 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -192,7 +192,7 @@ static int dw_edma_start_transfer(struct dw_edma_chan *chan)
>  		return 0;
>  
>  	dw_edma_core_start(dw, child, !desc->xfer_sz);
> -	desc->xfer_sz += child->ll_region.sz;
> +	desc->xfer_sz += child->xfer_sz;
>  	dw_edma_free_burst(child);
>  	list_del(&child->list);
>  	kfree(child);

[Severity: High]
This is a pre-existing issue, but could this sequence lead to a double-free
when racing with interrupts?

Looking at dw_edma_device_resume(), it calls dw_edma_start_transfer() without
holding the chan->vc.lock:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_resume() {
    ...
    } else {
        chan->status = EDMA_ST_BUSY;
        dw_edma_start_transfer(chan);
    }
    ...
}

If dw_edma_core_start() kicks the hardware and finishes immediately, an
interrupt could fire. The interrupt handler dw_edma_done_interrupt()
would take the lock and also call dw_edma_start_transfer(), potentially
freeing the exact same child entry concurrently.

While reviewing the broader code in this file, I also noticed another
potential leak in the channel termination code:

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

	return err;
}

[Severity: High]
This is a pre-existing issue, but do dw_edma_device_terminate_all() and
dw_edma_free_chan_resources() leak pending virt-dma descriptors?

dw_edma_device_terminate_all() only sets internal state flags but never
flushes the vc.desc_issued or vc.desc_submitted queues.

Additionally, dw_edma_free_chan_resources() waits for termination:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_free_chan_resources() {
    ...
	while (time_before(jiffies, timeout)) {
		ret = dw_edma_device_terminate_all(dchan);
		if (!ret)
			break;
    ...
}

However, it doesn't appear to call vchan_free_chan_resources() to free the
allocated virt-dma descriptors. Can this lead to memory leaks or the
execution of stale transfers if the channel is reused?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-edma_ll-v4-0-cc128f0afb61@nxp.com?part=2