Re: [PATCHv3 05/15] dmaengine: fsldma: check dma_async_device_register() return value

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added error path for `dma_async_device_register()` jumps to the wrong cleanup label, failing to free requested IRQs and causing a potential Use-After-Free.

Pre-existing issues:
- [High] Missing `tasklet_kill()` during channel teardown leads to Use-After-Free if a tasklet is pending.
- [High] Synchronous device removal without waiting for an RCU grace period violates RCU teardown ordering and causes a Use-After-Free.
--

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

dmaengine: fsldma: check dma_async_device_register() return value

This patch checks the return value of dma_async_device_register() in the
probe path and propagates errors instead of silently returning success. It
addresses a potential NULL pointer dereference during device removal if
registration had failed.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..0927b386ae7f7 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
[ ... ]
> @@ -1292,7 +1292,11 @@ static int fsldma_of_probe(struct platform_device *op)
>  		goto out_free_fdev;
>  	}
>  
> -	dma_async_device_register(&fdev->common);
> +	err = dma_async_device_register(&fdev->common);
> +	if (err) {
> +		dev_err(fdev->dev, "unable to register DMA device\n");
> +		goto out_free_fdev;

[Severity: High]
Does this new error path introduce a regression by skipping the IRQ cleanup?

Looking at fsldma_of_probe(), we previously successfully called
fsldma_request_irqs(), which registers shared interrupt handlers. If
dma_async_device_register() fails, jumping directly to out_free_fdev skips
unregistering these IRQs.

Since the handlers are on a shared interrupt line, an incoming interrupt
from another device could trigger the handler. Will the handler then
dereference the freed fdev or chan pointers, resulting in a use-after-free
regression?

> +	}
>  	return 0;
>  
>  out_free_fdev:

[Severity: High]
This isn't a regression introduced by this patch, but I noticed a potential
issue in fsl_dma_chan_remove() during channel teardown.

drivers/dma/fsldma.c:fsl_dma_chan_remove() {
	irq_dispose_mapping(chan->irq);
	list_del(&chan->common.device_node);
	iounmap(chan->regs);
	kfree(chan);
}

The driver initializes a tasklet for each channel during probe. When
fsldma_of_remove() unregisters the hardware interrupt, it synchronizes with
hardirqs but not with softirqs.

If a tasklet was scheduled right before free_irq() finished, could it still
be pending? Without a call to tasklet_kill() before kfree(chan), could the
delayed tasklet execute dma_do_tasklet and dereference the freed chan
pointer?

[Severity: High]
This is a pre-existing issue, but does synchronous device removal in
fsldma_of_remove() violate RCU teardown ordering?

drivers/dma/fsldma.c:fsldma_of_remove() {
	dma_async_device_unregister(&fdev->common);
	fsldma_free_irqs(fdev);
	...
	kfree(fdev);
}

The call to dma_async_device_unregister() removes the device from the
RCU-protected dma_device_list. However, the memory for fdev is freed
synchronously afterwards.

Could concurrent RCU readers traversing dma_device_list access the device
after it has been freed, causing a use-after-free? Should this wait for an
RCU grace period before freeing?

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