Re: [PATCH RFC] dmaengine: fsl-edma: check channel acquisition before use

[sashiko-bot@kernel.org]

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

New issues:
- [High] Premature abortion of channel multiplexing search when channel acquisition fails.

Pre-existing issues:
- [High] Double-increment of device privatecnt causes a reference leak and lockless data race.
- [High] Acquired DMA channel is permanently leaked on error path in fsl_edma_xlate.
- [High] Channel state flags like `srcid` and `is_remote` are updated locklessly during resource free, racing with channel allocation logic.
- [High] Failure to verify the return value of `dma_pool_create()` leads to a latent NULL pointer dereference.
--

commit fd5b3c44a24930a11481b00458a1c4c8b54e5433
Author: Slavin Liu <bolin.liu@seu.edu.cn>

    dmaengine: fsl-edma: check channel acquisition before use
    
    dma_get_slave_channel() can return NULL when acquiring a channel fails,
    for example if fsl_edma_alloc_chan_resources() cannot request an IRQ.
    fsl_edma3_xlate() dereferences that return value to update privatecnt.

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index d9fb717b5b53c..6934ac8826976 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c
> @@ -326,13 +326,16 @@ static struct dma_chan *fsl_edma3_xlate(struct of_phandle_args *dma_spec,
>  		if ((dma_spec->args[2] & FSL_EDMA_ODD_CH) && !(i & 0x1))
>  			continue;
>  
> +		chan = dma_get_slave_channel(chan);
> +		if (!chan)
> +			return NULL;

[Severity: High]
Does returning NULL here prematurely abort the channel multiplexing search?

If the first eligible channel fails dma_get_slave_channel() (e.g., due to an 
IRQ allocation error), returning NULL prevents the loop from checking other 
available channels. 

In multiplexed environments, should this use continue instead of return NULL to 
attempt acquiring the next available channel in the list?

> +
>  		fsl_chan->srcid = dma_spec->args[0];
>  		fsl_chan->priority = dma_spec->args[1];
>  		fsl_chan->is_rxchan = dma_spec->args[2] & FSL_EDMA_RX;
>  		fsl_chan->is_remote = dma_spec->args[2] & FSL_EDMA_REMOTE;
>  		fsl_chan->is_multi_fifo = dma_spec->args[2] & FSL_EDMA_MULTI_FIFO;
>  
> -		chan = dma_get_slave_channel(chan);
>  		chan->device->privatecnt++;

[Severity: High]
This is a pre-existing issue, but does this cause a reference leak and lockless
data race on the device privatecnt?

Since dma_get_slave_channel() safely increments privatecnt under 
dma_list_mutex, manually incrementing it again here without holding the lock 
appears redundant. This double-increment could permanently leak the private 
capability counter.

The same unprotected double-increment is also present in the legacy translation 
function fsl_edma_xlate().

[ ... ]

[Severity: High]
This is a pre-existing issue, but does fsl_edma_xlate() permanently leak the
acquired DMA channel on this error path?

drivers/dma/fsl-edma-main.c:fsl_edma_xlate() {
    ...
				if (!fsl_chan->srcid) {
					dev_err(&fsl_chan->pdev->dev, "Invalidate srcid %d\n",
						fsl_chan->srcid);
					return NULL;
				}
    ...
}

If a device tree specifies a 0 source ID, the function directly returns NULL
without unwinding and releasing the previously acquired dma_chan via
dma_release_channel().


[Severity: High]
This is a pre-existing issue, but do these lockless updates in 
fsl_edma_free_chan_resources() race with channel allocation logic?

drivers/dma/fsl-edma-common.c:fsl_edma_free_chan_resources() {
    ...
    fsl_chan->srcid = 0;
    fsl_chan->is_remote = false;
    ...
}

These shared state fields are updated without holding fsl_edma_mutex. If one 
device releases its DMA channel while another requests a channel for the same 
source ID, could fsl_edma3_xlate() observe a stale non-zero value? 

This could cause a spurious failure in fsl_edma_srcid_in_use():

drivers/dma/fsl-edma-main.c:fsl_edma_srcid_in_use() {
    ...
    if (fsl_chan->srcid && srcid == fsl_chan->srcid)
    ...
}


[Severity: High]
This is a pre-existing issue, but does fsl_edma_alloc_chan_resources() fail to 
verify the return value of dma_pool_create()?

drivers/dma/fsl-edma-common.c:fsl_edma_alloc_chan_resources() {
    ...
	fsl_chan->tcd_pool = dma_pool_create("tcd_pool", chan->device->dev,
				fsl_edma_drvflags(fsl_chan) & FSL_EDMA_DRV_TCD64 ?
				sizeof(struct fsl_edma_hw_tcd64) : sizeof(struct fsl_edma_hw_tcd),
				32, 0);

	if (fsl_chan->txirq)
    ...
}

If dma_pool_create() fails under memory pressure and returns NULL, the function
ignores the error and returns success to the DMA framework. 

Subsequent DMA transfer preparations would pass the NULL pool pointer to 
dma_pool_alloc(), causing a null pointer dereference when accessing the pool's 
internal spinlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911060902.94153-1-bolin.liu@seu.edu.cn?part=1