Re: [PATCH v3 06/34] dmaengine: fsldma: migrate tasklet to dmaengine BH

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

Pre-existing issues:
- [High] Missing cancellation of the bottom-half work in the channel removal and error paths leads to a use-after-free.
- [High] The driver invokes client DMA completion callbacks and dependent transactions while holding the channel lock, which causes a recursive deadlock.
--

--- Patch [6]: [PATCH v3 06/34] dmaengine: fsldma: migrate tasklet to dmaengine BH ---
commit 9529606b1e089f3e7222105fc2792fecf8bf406b
Author: Allen Pais <[email protected]>

dmaengine: fsldma: migrate tasklet to dmaengine BH

This commit replaces the per-channel tasklet with the shared dmaengine
BH helper. The handler continues to run in softirq context while dmaengine
owns the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..f52547611db99 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
[ ... ]
> @@ -968,18 +968,18 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
[ ... ]
> -static void dma_do_tasklet(struct tasklet_struct *t)
> +static void dma_do_tasklet(struct dma_chan *c)
>  {
> -	struct fsldma_chan *chan = from_tasklet(chan, t, tasklet);
> +	struct fsldma_chan *chan = to_fsl_chan(c);
>  
>  	chan_dbg(chan, "tasklet entry\n");

[Severity: High]
This is a pre-existing issue, but since dma_do_tasklet invokes client
callbacks, is there a risk of a recursive spinlock deadlock?

Looking at dma_do_tasklet(), it acquires the channel descriptor lock and then
triggers cleanup:

dma_do_tasklet() {
    spin_lock(&chan->desc_lock);
    fsldma_cleanup_descriptors(chan);
    ...

Inside the cleanup routine, it calls fsldma_run_tx_complete_actions(), which
runs the client callback and dependencies with the lock still held:

fsldma_run_tx_complete_actions() {
    ...
    dmaengine_desc_get_callback_invoke(txd, NULL);
    dma_run_dependencies(txd);
}

If a client callback or a dependent transaction attempts to submit a new
transfer to the same channel, it will try to acquire the same lock:

fsl_dma_tx_submit() {
    spin_lock_bh(&chan->desc_lock);
    ...

Could this cause the subsystem to freeze when clients submit new transactions
from their completion callbacks?

[ ... ]
> @@ -1152,7 +1152,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
>  	}
>  
>  	fdev->chan[chan->id] = chan;
> -	tasklet_setup(&chan->tasklet, dma_do_tasklet);
> +	dmaengine_init_bh(&chan->common, dma_do_tasklet);
>  	snprintf(chan->name, sizeof(chan->name), "chan%d", chan->id);

[Severity: High]
This isn't a bug introduced by this patch, but missing teardown logic
might lead to a use-after-free during driver removal or probe errors.

The original code lacked a call to kill the tasklet, and the new code
similarly omits a call to shut down the bottom half work.

If an interrupt fires right before the channel is freed, the bottom half
could be scheduled and remain on the workqueue:

fsl_dma_chan_remove() {
    ...
    kfree(chan);
}

When the workqueue eventually processes the queued bottom half, it will
dereference the freed channel structure:

dma_do_tasklet() {
    ...
    spin_lock(&chan->desc_lock);

Should we add a call to dmaengine_kill_bh(&chan->common) before freeing the
channel structure to ensure the asynchronous work is safely flushed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6