Re: [PATCH 01/10] dmaengine: fsldma: kill tasklet before removing channel

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The commit message inaccurately claims the channel struct is 'no longer freed in the remove path' due to devm conversions, concluding this is not a use-after-free fix.

Pre-existing issues:
- [High] The driver holds a spinlock while executing client callbacks, causing a deadlock if the client submits new transactions.
- [High] The hardware DMA engine is not explicitly halted when the driver is unbound or when channel resources are freed, leading to DMA fetching from freed memory.
--

commit 2874e539ce862c4c0134a865315888712d6c8923
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: kill tasklet before removing channel

This commit adds tasklet_kill() to fsl_dma_chan_remove() to prevent a race
where the tasklet could run after the channel has been torn down.

> With the recent devm conversions
> the channel struct is no longer freed in the remove path, so
> this is not a use-after-free crash fix, but rather correct
> shutdown sequencing to avoid the tasklet operating on a
> logically-removed channel.

[Severity: Medium]
Is this description accurate for this exact point in the git history?

Looking at fsl_dma_chan_remove(), it still explicitly calls kfree() on the
channel struct:

drivers/dma/fsldma.c:fsl_dma_chan_remove() {
        tasklet_kill(&chan->tasklet);
        [ ... ]
        kfree(chan);
}

It appears the memory is indeed manually freed here, making this patch an
active prevention of a use-after-free crash. Could the commit message be
updated to reflect the current state of the code before the devm conversions
are applied?

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..0e2f84862261c 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1205,6 +1205,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
>  
>  static void fsl_dma_chan_remove(struct fsldma_chan *chan)
>  {
> +	tasklet_kill(&chan->tasklet);
>  	irq_dispose_mapping(chan->irq);
>  	list_del(&chan->common.device_node);
>  	iounmap(chan->regs);

[Severity: High]
This is a pre-existing issue, but does the hardware DMA engine need to be
explicitly halted when the driver is unbound or resources are freed?

During driver unbind in fsldma_of_remove(), the driver unmaps MMIO registers
and frees structures:

drivers/dma/fsldma.c:fsldma_of_remove() {
        [ ... ]
        for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
                if (fdev->chan[i])
                        fsl_dma_chan_remove(fdev->chan[i]);
        }
        irq_dispose_mapping(fdev->irq);
        iounmap(fdev->regs);
        kfree(fdev);
        [ ... ]
}

Similarly, fsl_dma_free_chan_resources() destroys the descriptor pool:

drivers/dma/fsldma.c:fsl_dma_free_chan_resources() {
        [ ... ]
        dma_pool_destroy(chan->desc_pool);
        [ ... ]
}

Since dma_halt() is never called on the channel, could an actively running
transfer continue executing in the background and fetch descriptors or write
data to physical memory pages that have already been freed?

[Severity: High]
This is another pre-existing issue, but can this code cause a recursive
spinlock deadlock if a client submits new transactions from a callback?

When the driver executes client callbacks, it holds the descriptor lock:

drivers/dma/fsldma.c:fsldma_cleanup_descriptors() {
        [ ... ]
        cookie = fsldma_run_tx_complete_actions(chan, desc, cookie);
        [ ... ]
}

If the client completion callback then invokes fsl_dma_tx_submit() to submit
a new DMA transaction, that function attempts to acquire the same lock:

drivers/dma/fsldma.c:fsl_dma_tx_submit() {
        [ ... ]
        spin_lock_bh(&chan->desc_lock);
        [ ... ]
}

Will this result in a deadlock on the same CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605220134.43295-1-rosenp@gmail.com?part=1