Re: [PATCHv3 2/8] dmaengine: ti: omap-dma: synchronize CPU PM notifier removal

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the CPU PM notifier is not unregistered in omap_dma_remove() if the driver is configured with `needs_busy_check` but not `may_lose_context`, causing a use-after-free.
- [High] This problem wasn't introduced by this patch, but a missing `return rc;` in the `of_dma_controller_register()` error path causes the CPU PM notifier to be incorrectly registered before returning an error, leading to a use-after-free.
- [High] This problem wasn't introduced by this patch, but the hardware IRQ handler is prematurely freed before the DMA device is unregistered and before hardware interrupts are properly disabled.
--

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

dmaengine: ti: omap-dma: synchronize CPU PM notifier removal

cpu_pm_notify() walks the raw notifier chain under rcu_read_lock(),
while cpu_pm_unregister_notifier() only unlinks the notifier block.
This patch waits for an RCU grace period after unregistering the CPU PM
notifier to prevent concurrent readers from dereferencing a freed block.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 55ece7fd0d99a..6d1fb39956d7e 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c

[ ... ]

> @@ -1852,8 +1853,10 @@ static void omap_dma_remove(struct platform_device *pdev)
>  	struct omap_dmadev *od = platform_get_drvdata(pdev);
>  	int irq;
>  
> -	if (od->cfg->may_lose_context)
> +	if (od->cfg->may_lose_context) {
>  		cpu_pm_unregister_notifier(&od->nb);
> +		synchronize_rcu();
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does this leak the CPU PM 
notifier if the driver is configured with needs_busy_check but not 
may_lose_context?

In omap_dma_probe(), the notifier is registered if either needs_busy_check 
or may_lose_context is true:

drivers/dma/ti/omap-dma.c:omap_dma_probe() {
	...
	if (od->cfg->needs_busy_check) {
		od->nb.notifier_call = omap_dma_busy_notifier;
		cpu_pm_register_notifier(&od->nb);
	} else if (od->cfg->may_lose_context) {
	...
}

If a platform configuration sets only needs_busy_check, the notifier is 
never unregistered during remove. Because the od structure is devm-allocated,
will this leave a dangling pointer in the CPU PM notifier chain that causes 
a use-after-free on the next PM event?

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing return 
statement in the probe error path that could lead to a similar use-after-free?

drivers/dma/ti/omap-dma.c:omap_dma_probe() {
	...
		rc = of_dma_controller_register(pdev->dev.of_node,
				of_dma_simple_xlate, &omap_dma_info);
		if (rc) {
			pr_warn("OMAP-DMA: failed to register DMA controller\n");
			dma_async_device_unregister(&od->ddev);
			if (od->ll123_supported)
				dma_pool_destroy(od->desc_pool);
			omap_dma_free(od);
		}
	}
	
	if (od->cfg->needs_busy_check) {
		od->nb.notifier_call = omap_dma_busy_notifier;
		cpu_pm_register_notifier(&od->nb);
	...
	return rc;
}

If of_dma_controller_register() fails, execution falls through, potentially
registers the CPU PM notifier using the already-freed od structure, and then
returns the error code. 

When devres cleans up the failed probe, will the notifier remain registered 
and point to freed memory?

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
during teardown if an interrupt fires after devm_free_irq() is called?

Looking at the rest of omap_dma_remove():

drivers/dma/ti/omap-dma.c:omap_dma_remove() {
	...
	irq = platform_get_irq(pdev, 1);
	devm_free_irq(&pdev->dev, irq, od);

	dma_async_device_unregister(&od->ddev);

	if (!omap_dma_legacy(od)) {
		/* Disable all interrupts */
		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
	}
	...
}

Since devm_free_irq() is called before dma_async_device_unregister(), if a 
DMA transfer completes while waiting for device unregistration, will the 
hardware assert the IRQ line without a handler, causing an unhandled 
interrupt storm?

Additionally, the teardown logic disables IRQENABLE_L0 instead of 
IRQENABLE_L1. Could this leave the device capable of firing interrupts 
even after the driver is unloaded?

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