Re: [PATCHv4 01/15] dmaengine: fsldma: kill tasklet before removing channel

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] fsldma_of_probe ignores the return value of dma_async_device_register.
- [High] Use-after-free read in fsl_dma_prep_memcpy error path due to freeing the list head during iteration.
- [High] Link descriptors are leaked if fsl_dma_tx_submit() is called while the device is suspended.
- [High] Deadlock caused by invoking client callbacks and dependencies with desc_lock held.
- [Medium] Incorrect loop unwind in fsldma_request_irqs leads to freeing an unallocated IRQ.
--

commit 45ba39c99b39bf6124bcca48128dda7ac68f9811
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);
>  	irq_dispose_mapping(chan->irq);
>  	list_del(&chan->common.device_node);
>  	iounmap(chan->regs);

[Severity: Medium]
This is a pre-existing issue, but I noticed the error handling in
fsldma_request_irqs() can free an unallocated interrupt:

drivers/dma/fsldma.c:fsldma_request_irqs() {
...
out_unwind:
	for (/* none */; i >= 0; i--) {
		chan = fdev->chan[i];
		if (!chan)
			continue;

		if (!chan->irq)
			continue;

		free_irq(chan->irq, chan);
	}
...
}

If request_irq() fails for a channel at index i, the driver does not own
the interrupt for that index. Does starting the unwind loop at the same
index i trigger a kernel warning for trying to free an already-free
interrupt?

[Severity: High]
This is a pre-existing issue, but the return value of
dma_async_device_register() appears to be discarded during probe:

drivers/dma/fsldma.c:fsldma_of_probe() {
...
	dma_async_device_register(&fdev->common);
	return 0;
...
}

If dma_async_device_register() returns an error code like -ENOMEM, the
driver will falsely report a successful probe.

Could this leave the device partially initialized and cause a
use-after-free or id allocator free on an uninitialized id when
dma_async_device_unregister() is called during module unbind?

[Severity: High]
This is a pre-existing issue, but there appears to be a use-after-free
read in the error path of fsl_dma_prep_memcpy():

drivers/dma/fsldma.c:fsl_dma_prep_memcpy() {
...
fail:
	if (!first)
		return NULL;

	fsldma_free_desc_list_reverse(chan, &first->tx_list);
	return NULL;
}

When fsldma_free_desc_list_reverse() is called, it iterates through
tx_list:

drivers/dma/fsldma.c:fsldma_free_desc_list_reverse() {
...
	list_for_each_entry_safe_reverse(desc, _desc, list, node)
		fsl_dma_free_descriptor(chan, desc);
...
}

Since all descriptors, including first, are linked into first->tx_list,
when the first descriptor is freed, its embedded tx_list head is also
freed.

Does the safe iterator macro then read a freed next pointer when
evaluating the next element, resulting in a use-after-free read?

[Severity: High]
This is a pre-existing issue, but link descriptors seem to leak if a
transaction is submitted while the device is suspended:

drivers/dma/fsldma.c:fsl_dma_tx_submit() {
...
#ifdef CONFIG_PM
	if (unlikely(chan->pm_state != RUNNING)) {
		chan_dbg(chan, "cannot submit due to suspend\n");
		spin_unlock_bh(&chan->desc_lock);
		return -1;
	}
#endif
...
}

Since the descriptors were allocated by fsl_dma_prep_memcpy() and
ownership was handed off for submission, returning here without queueing
them leaves them orphaned.

Are these descriptors permanently leaked from the dma pool?

[Severity: High]
This is a pre-existing issue, but could invoking client callbacks while
holding desc_lock lead to a deadlock?

drivers/dma/fsldma.c:dma_do_tasklet() {
...
	spin_lock(&chan->desc_lock);
...
	fsldma_cleanup_descriptors(chan);

	spin_unlock(&chan->desc_lock);
...
}

fsldma_cleanup_descriptors() iterates over running descriptors and calls
fsldma_run_tx_complete_actions(), which invokes callbacks:

drivers/dma/fsldma.c:fsldma_run_tx_complete_actions() {
...
		dmaengine_desc_get_callback_invoke(txd, NULL);
...
}

If the client callback calls dmaengine_submit() or a dependent
transaction executes on the same channel, it will call
fsl_dma_tx_submit(), which attempts to acquire
spin_lock_bh(&chan->desc_lock).

Will this cause a self-deadlock since the lock is already held by the
tasklet?

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