Re: [PATCH v2 06/64] dmaengine: fsldma: migrate tasklet to dma_chan BH

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing dma_chan_kill_bh() during channel removal and resource freeing leaves pending bottom-half work queued, leading to use-after-free and NULL pointer dereference crashes.
- [High] Lock inversion deadlock when invoking client callbacks with the channel descriptor lock held.
--

--- Patch [6]: [PATCH v2 06/64] dmaengine: fsldma: migrate tasklet to dma_chan BH ---
commit eba223ea472b760cffa1e53fb0bb48d7fa6057d5
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: fsldma: migrate tasklet to dma_chan BH

Replace the per-channel tasklet with the shared dma_chan 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..1cb6ce3270782 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 does this cleanup path have a lock inversion
deadlock?

dma_do_tasklet() acquires chan->desc_lock before calling
fsldma_cleanup_descriptors(). While holding this lock, it invokes
fsldma_run_tx_complete_actions(), which executes the client callback via
dmaengine_desc_get_callback_invoke() and processes dependencies via
dma_run_dependencies().

If the client callback submits a new DMA transaction, or if a dependent
transaction is submitted for the same channel, it calls fsl_dma_tx_submit():

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

Since the non-recursive lock is already held on the same CPU, won't this
lead to a deadlock in softirq context?

> @@ -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);
> +	dma_chan_init_bh(&chan->common, dma_do_tasklet);

[Severity: High]
This is a pre-existing issue, but is it possible for pending bottom-half work
to run after the channel is removed?

If an IRQ fires and queues the BH right before unbinding,
fsl_dma_chan_remove() frees the channel structure and
fsl_dma_free_chan_resources() destroys the descriptor pool without calling
dma_chan_kill_bh() (or tasklet_kill() previously).

When the pending BH work later executes dma_do_tasklet(), could it dereference
the freed channel or pass a NULL desc_pool to dma_pool_free()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=6