* Re: [PATCH RFC] dmaengine: fsl-edma: check channel acquisition before use
2026-09-11 6:09 [PATCH RFC] dmaengine: fsl-edma: check channel acquisition before use Slavin Liu
@ 2026-09-11 6:26 ` sashiko-bot
2026-09-11 14:56 ` Frank Li
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-09-11 6:26 UTC (permalink / raw)
To: Slavin Liu; +Cc: vkoul, dmaengine, imx, Frank.Li
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH RFC] dmaengine: fsl-edma: check channel acquisition before use
2026-09-11 6:09 [PATCH RFC] dmaengine: fsl-edma: check channel acquisition before use Slavin Liu
2026-09-11 6:26 ` sashiko-bot
@ 2026-09-11 14:56 ` Frank Li
1 sibling, 0 replies; 3+ messages in thread
From: Frank Li @ 2026-09-11 14:56 UTC (permalink / raw)
To: Slavin Liu, Joy Zou; +Cc: frank.li, vkoul, imx, dmaengine, linux-kernel
On Fri, Sep 11, 2026 at 02:09:02PM +0800, Slavin Liu wrote:
Remove RFC
dmaengine: fsl-edma: check channel acquisition before mark it used.
>
> 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.
>
> Acquire and check the channel before publishing its source ID and
> request parameters. A NULL-only check after the existing source-ID
> assignment would leave the failed request marked as in use, causing
> fsl_edma_srcid_in_use() to reject a subsequent request for that source.
> The channel resource-allocation callback does not consume these request
> parameters, so set them only after acquisition succeeds. The existing
> scoped mutex release and successful-channel return are preserved.
>
> Detected by static analysis and reviewed with AI-assisted source auditing.
Add Joy Zou, who change this logic recently
Frank
>
> Fixes: 72f5801a4e2b ("dmaengine: fsl-edma: integrate v3 support")
> Assisted-by: LLM
> Signed-off-by: Slavin Liu <bolin.liu@seu.edu.cn>
> ---
> drivers/dma/fsl-edma-main.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index d9fb717b5b53..6934ac882697 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;
> +
> 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++;
> return chan;
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread