Re: [PATCH v5 10/10] dmaengine: dw-edma: Remove struct dw_edma_chunk

[sashiko-bot@kernel.org]

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

New issues:
- [High] Replacing dynamically allocated small chunks with a single flat array creates a high risk of contiguous memory allocation failures for large scatter-gather lists.
- [High] If a channel is paused while the final burst is in flight, resuming the channel causes an out-of-bounds access on the descriptor's flexible array.
- [High] An integer overflow in calculating the transfer count for interleaved transfers leads to a zero-sized flexible array allocation, resulting in out-of-bounds memory accesses.
- [Medium] Failure to update the descriptor's `nburst` field when the scatter-gather loop terminates early causes uninitialized memory to be programmed into the hardware and incorrect residue calculations.
- [Low] Fields used to manage the removed chunk layer are left unused in `struct dw_edma_desc`.

Pre-existing issues:
- [High] Cyclic DMA transfers do not increment the memory address per period, causing the hardware to overwrite the same memory block repeatedly.
--

--- Patch [10]: [PATCH v5 10/10] dmaengine: dw-edma: Remove struct dw_edma_chunk ---
commit f95bbeacc03c2793d791c26855c3782bd0ea8bc2
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Remove struct dw_edma_chunk

This commit removes the dw_edma_chunk linked list layer from the DMA
descriptor layout. It embeds the burst array directly into struct
dw_edma_desc as a flexible array, aiming to reduce memory allocation
overhead by allocating descriptors and bursts together.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 01bee22fe3b3e..30b034a94a815 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> -static struct dw_edma_desc *dw_edma_alloc_desc(struct dw_edma_chan *chan)
> +static struct dw_edma_desc *
> +dw_edma_alloc_desc(struct dw_edma_chan *chan, u32 nburst)
>  {
>  	struct dw_edma_desc *desc;
>  
> -	desc = kzalloc_obj(*desc, GFP_NOWAIT);
> +	desc = kzalloc_flex(*desc, burst, nburst, GFP_NOWAIT);

[Severity: High]
Does allocating the entire burst array contiguously with GFP_NOWAIT introduce
a risk of allocation failures? Previously, chunking the allocations into sizes
bounded by chan->ll_max mitigated memory fragmentation. Now, large
scatter-gather lists (e.g., > 1000 entries) require large high-order
contiguous memory blocks, which might fail since reclaim and compaction
are disabled.

>  	if (unlikely(!desc))
>  		return NULL;
[ ... ]
> -static void dw_edma_core_start(struct dw_edma_chunk *chunk, bool first)
> +static void dw_edma_core_start(struct dw_edma_desc *desc, bool first)
>  {
> -	struct dw_edma_chan *chan = chunk->chan;
> +	struct dw_edma_chan *chan = desc->chan;
>  	u32 i = 0;
>  
>  	if (chan->non_ll) {
> -		if (chunk->nburst == 1)
> -			chan->dw->core->non_ll_start(chunk->chan, &chunk->burst[0]);
> +		chan->dw->core->non_ll_start(chan, &desc->burst[desc->start_burst]);

[Severity: High]
Can resuming a paused channel cause an out-of-bounds access here?

If a channel is paused (EDMA_REQ_PAUSE) right as its final burst completes in
dw_edma_done_interrupt(), the pause request skips the completion block,
leaving the fully completed descriptor on the active list. When resumed,
dw_edma_start_transfer() will retrieve the descriptor and call
dw_edma_core_start().

Because the chunk-list exhaustion check was removed, won't this blindly access
desc->burst[desc->start_burst] when start_burst equals nburst, reading past
the end of the flexible array?

> +		desc->done_burst = desc->start_burst;
> +		desc->start_burst += 1;
>  		return;
>  	}
[ ... ]
> @@ -452,19 +405,15 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer,
>  		fsz = xfer->xfer.il->frame_size;
>  	}
>  
> +	desc = dw_edma_alloc_desc(chan, cnt);
> +	if (unlikely(!desc))
> +		return NULL;
> +

[Severity: High]
Could this allocation create a zero-sized flexible array if an integer
overflow occurs?

The cnt variable is a u32, but it is calculated just above using size_t fields
from the interleaved transfer:

    } else if (xfer->type == EDMA_XFER_INTERLEAVED) {
        cnt = xfer->xfer.il->numf * xfer->xfer.il->frame_size;
        fsz = xfer->xfer.il->frame_size;
    }

If a client driver submits an interleaved transfer where numf * frame_size
equals a multiple of 4GB (e.g., 0x100000000), the product wraps to 0.
This would allocate a 0-length flexible array, leading to out-of-bounds
accesses later in dw_edma_start_transfer() and dw_hdma_set_callback_result().

>  	for (i = 0; i < cnt; i++) {
>  		if (xfer->type == EDMA_XFER_SCATTER_GATHER && !sg)
>  			break;

[Severity: Medium]
Does desc->nburst need to be updated if this scatter-gather loop breaks early?

If a client submits a scatter-gather list where len is greater than the actual
mapped entries, the loop exits early, but desc->nburst remains set to the
artificially large cnt value. dw_edma_core_start() will then loop up to
desc->nburst and submit uninitialized bursts to the DMA engine. Furthermore,
dw_hdma_set_callback_result() will access the uninitialized last element to
calculate residue, causing it to subtract a zero xfer_sz and report incorrect
results.

>  
> -		if (!(i % chan->ll_max)) {
> -			u32 n = min(cnt - i, chan->ll_max);
> -
> -			chunk = dw_edma_alloc_chunk(desc, n);
> -			if (unlikely(!chunk))
> -				goto err_alloc;
> -		}
> -
> -		burst = chunk->burst + (i % chan->ll_max);
> +		burst = desc->burst + i;
>  
>  		if (xfer->type == EDMA_XFER_CYCLIC)
>  			burst->sz = xfer->xfer.cyclic.len;
> @@ -473,8 +422,8 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer,
>  		else if (xfer->type == EDMA_XFER_INTERLEAVED)
>  			burst->sz = xfer->xfer.il->sgl[i % fsz].size;
>  
> -		chunk->xfer_sz += burst->sz;
>  		desc->alloc_sz += burst->sz;
> +		burst->xfer_sz = desc->alloc_sz;
>  
>  		if (dir == DMA_DEV_TO_MEM) {
>  			burst->sar = src_addr;
>  			if (xfer->type == EDMA_XFER_CYCLIC) {
>  				burst->dar = xfer->xfer.cyclic.paddr;

[Severity: High]
This is a pre-existing issue, but do cyclic DMA transfers fail to increment
the memory address per period here?

burst->dar (and similarly burst->sar for the opposite direction) is assigned
the fixed xfer->xfer.cyclic.paddr without adding any offset per period. Since
the addresses are not incremented by the period size across iterations, won't
the hardware repeatedly overwrite or read the exact same memory block?

[ ... ]
> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index 4950c57fca34f..7f2ec871f5bd5 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h
[ ... ]
> @@ -66,6 +59,12 @@ struct dw_edma_desc {
>  
>  	u32				alloc_sz;
>  	u32				xfer_sz;
> +
> +	u32				done_burst;

[Severity: Low]
Since the struct dw_edma_chunk layer was removed, are the chunk_list,
chunks_alloc, and xfer_sz fields still needed in struct dw_edma_desc?
Looking at the full struct definition, they appear to be unused now,
and leaving them in wastes memory for every descriptor allocation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-edma_ll-v5-0-e199053d4300@nxp.com?part=10