[PATCHv2] dmaengine: fsldma: kill tasklet before removing channel

[PATCHv2] dmaengine: fsldma: kill tasklet before removing channel

[Rosen Penev]

Add tasklet_kill() in fsl_dma_chan_remove() to prevent a race where the
tasklet is scheduled by the IRQ handler and runs after the channel has been
freed.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 v2: split from series and wrap at 75 characters
 drivers/dma/fsldma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 6ce0e5668dc2..5eca8ab23564 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1207,6 +1207,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);
-- 
2.55.0

Re: [PATCHv2] dmaengine: fsldma: kill tasklet before removing channel

[sashiko-bot]

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

Pre-existing issues:
- [High] The error path in fsldma_request_irqs() calls free_irq() on an IRQ that just failed to be requested, triggering a kernel WARNING.
- [High] fsldma_of_probe() completely ignores the return value of dma_async_device_register(), leading to silent failures and potential list corruption.
- [High] Recursive deadlock when invoking client callbacks or resolving DMA dependencies in fsldma_run_tx_complete_actions().
- [High] Unbalanced spin_unlock_bh() on the error path in fsldma_suspend_late(), causing a double unlock.
- [High] DMA engine is not halted during channel removal, potentially causing memory corruption.
--

commit 6e64fb2982240feb5c9b3c6e8b290c5fad43ac6d
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: kill tasklet before removing channel

Add tasklet_kill() in fsl_dma_chan_remove() to prevent a race where the
tasklet is scheduled by the IRQ handler and runs after the channel has been
freed.

> 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);

[Severity: High]
This is a pre-existing issue, but does this function neglect to halt the
hardware DMA controller before tearing down the software channel? If the driver
is unbound while a DMA transfer is in progress, couldn't the hardware continue
executing transactions and corrupt memory after the descriptors and IO
registers are destroyed?

>  	irq_dispose_mapping(chan->irq);
>  	list_del(&chan->common.device_node);
>  	iounmap(chan->regs);

[ ... ]


drivers/dma/fsldma.c:fsldma_request_irqs() {
    ...
		ret = request_irq(chan->irq, fsldma_chan_irq, IRQF_SHARED,
				  "fsldma-chan", chan);
		if (ret) {
			chan_err(chan, "unable to request per-channel IRQ\n");
			goto out_unwind;
		}
	}

	return 0;

out_unwind:
	for (/* none */; i >= 0; i--) {
		chan = fdev->chan[i];
		if (!chan)
			continue;

		if (!chan->irq)
			continue;

		free_irq(chan->irq, chan);
	}
	return ret;
}

[Severity: High]
This is a pre-existing issue, but since the index 'i' is not decremented
before entering the out_unwind loop, could this call free_irq() on the channel
that just failed initialization? This might trigger a kernel warning for trying
to free an already-free IRQ.


drivers/dma/fsldma.c:fsldma_of_probe() {
    ...
	err = fsldma_request_irqs(fdev);
	if (err) {
		dev_err(fdev->dev, "unable to request IRQs\n");
		goto out_free_fdev;
	}

	dma_async_device_register(&fdev->common);
	return 0;
}

[Severity: High]
This is a pre-existing issue, but does fsldma_of_probe() ignore the return
value of dma_async_device_register()? If registration fails, returning success
could lead to a NULL pointer dereference later during device removal when
dma_async_device_unregister() is called on an uninitialized device.


drivers/dma/fsldma.c:fsldma_run_tx_complete_actions() {
    ...
	if (txd->cookie > 0) {
		ret = txd->cookie;

		dma_descriptor_unmap(txd);
		/* Run the link descriptor callback function */
		dmaengine_desc_get_callback_invoke(txd, NULL);
	}

	/* Run any dependencies */
	dma_run_dependencies(txd);
    ...
}

[Severity: High]
This is a pre-existing issue, but is there a potential recursive deadlock
when invoking client callbacks? fsldma_cleanup_descriptors() holds
chan->desc_lock when calling fsldma_run_tx_complete_actions(). If a client
callback or dependency submits a new transaction to the same channel,
fsl_dma_tx_submit() will try to acquire the same chan->desc_lock.


drivers/dma/fsldma.c:fsldma_suspend_late() {
    ...
	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
		chan = fdev->chan[i];
		if (!chan)
			continue;

		spin_lock_bh(&chan->desc_lock);
		if (unlikely(!chan->idle))
			goto out;
		chan->regs_save.mr = get_mr(chan);
		chan->pm_state = SUSPENDED;
		spin_unlock_bh(&chan->desc_lock);
	}
	return 0;

out:
	for (; i >= 0; i--) {
		chan = fdev->chan[i];
		if (!chan)
			continue;
		chan->pm_state = RUNNING;
		spin_unlock_bh(&chan->desc_lock);
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but does the error path in fsldma_suspend_late()
perform a double-unlock? For channels prior to the failing index 'i', the lock
was already correctly released in the first loop. Does the unconditional
spin_unlock_bh() in the out loop cause a double-unlock for those previously
processed channels?

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