Re: [PATCH 4/4] dmaengine: ti: omap-dma: fix interrupt handling in remove

[sashiko-bot@kernel.org]

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

New issues:
- [High] Data race when modifying IRQ enable mask leaves interrupts vulnerable to storms.

Pre-existing issues:
- [Critical] Use-after-free of DMA channels during forced unbind.
- [High] Missing IO flush before freeing the shared IRQ handler allows unhandled interrupts.
--

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

dmaengine: ti: omap-dma: fix interrupt handling in remove

This commit aims to fix three interrupt handling bugs in the remove path.
It changes the interrupt disable logic to write to IRQENABLE_L1 instead
of IRQENABLE_L0, disables hardware interrupts before freeing the shared
IRQ handler, and adds a return value check for platform_get_irq().

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index fd1ad3b4268c9..ad90ca226db35 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c
> @@ -1859,16 +1859,17 @@ static void omap_dma_remove(struct platform_device *pdev)
>  	if (pdev->dev.of_node)
>  		of_dma_controller_free(pdev->dev.of_node);
>  
> -	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);
> +		od->irq_enable_mask = 0;
> +		omap_dma_glbl_write(od, IRQENABLE_L1, 0);

[Severity: High]
Does clearing od->irq_enable_mask and writing to IRQENABLE_L1 without
holding spin_lock_irq(&od->irq_lock) introduce a data race?

If an interrupt fires concurrently, the IRQ handler (omap_dma_irq) might
read the cleared mask, evaluate status &= 0, and return IRQ_NONE without
acknowledging the pending hardware interrupt.

Furthermore, if a client releases a channel concurrently:

omap_dma_free_chan_resources() {
    spin_lock_irqsave(&od->irq_lock, flags);
    ...
    omap_dma_glbl_write(od, IRQENABLE_L1, od->irq_enable_mask);
    spin_unlock_irqrestore(&od->irq_lock, flags);
}

This could write a non-zero mask to IRQENABLE_L1 immediately after this
remove path disabled it, leaving hardware interrupts active right before
devm_free_irq() removes the handler.

[Severity: High]
This isn't a bug introduced by this patch, but does the posted write to
IRQENABLE_L1 require a dummy read to flush the interconnect's write
buffers before freeing the IRQ?

Because there is no subsequent flush before devm_free_irq() is called,
the hardware disable command may not reach the DMA controller in time.

If the controller asserts an interrupt during this window, the CPU will
see an unhandled interrupt on a shared line, which can prompt the
generic IRQ subsystem to permanently disable the shared IRQ line.

>  	}
>  
> +	irq = platform_get_irq(pdev, 1);
> +	if (irq > 0)
> +		devm_free_irq(&pdev->dev, irq, od);
> +
>  	omap_dma_free(od);

[Severity: Critical]
This is a pre-existing issue, but can the call to omap_dma_free() result
in a use-after-free if a manual sysfs unbind occurs?

omap_dma_free() unconditionally frees all channel structures:

omap_dma_free() {
    while (!list_empty(&od->ddev.channels)) {
        ...
        kfree(c);
    }
}

Since the DMA subsystem does not automatically create strict device links
for consumers, a sysfs unbind of the DMA controller will not synchronously
unbind its consumers.

Active clients will thus continue to access their held channels and
eventually call dma_release_channel() on the freed memory.

>  
>  	if (od->ll123_supported)

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