Re: [PATCH v2 4/5] 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] Skipping the deallocation of active DMA channels introduces a memory leak and causes a Use-After-Free of the devres-managed parent controller.
- [Medium] Missing `!omap_dma_legacy(od)` guard in probe error paths triggers WARN_ON(1) on OMAP1 platforms.

Pre-existing issues:
- [High] Virtual channel resources and descriptor pools are unconditionally freed before halting active hardware DMA channels, leading to memory corruption.
--

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

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

This commit addresses several bugs in the driver's remove path, including
incorrect IRQ disabling and missing locks. It also changes omap_dma_free()
to avoid freeing channel memory if clients still hold references. The goal
is to ensure safe driver removal and prevent resource leaks.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index dde270646bb94..8c32b7ab50f63 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c
> @@ -1516,13 +1516,21 @@ static int omap_dma_chan_init(struct omap_dmadev *od)
>  
>  static void omap_dma_free(struct omap_dmadev *od)
>  {
> +	struct omap_chan *c;
> +
>  	while (!list_empty(&od->ddev.channels)) {
> -		struct omap_chan *c = list_first_entry(&od->ddev.channels,
> -			struct omap_chan, vc.chan.device_node);
> +		c = list_first_entry(&od->ddev.channels,
> +				     struct omap_chan, vc.chan.device_node);
>  
>  		list_del(&c->vc.chan.device_node);
>  		tasklet_kill(&c->vc.task);
>  		vchan_free_chan_resources(&c->vc);

[Severity: High]
This is a pre-existing issue, but does this prematurely free resources while
the hardware is still active?

In omap_dma_free(), virtual channel resources and descriptors are
unconditionally torn down before verifying if the client is still using the
channel and before halting active hardware DMA channels.

If a channel is actively transferring data, its physical descriptors are
returned to the pool before the hardware is stopped. When dma_pool_destroy()
is later called in omap_dma_remove(), won't the still-running DMA hardware
continue accessing the freed descriptor memory, leading to memory corruption?

> +		if (c->vc.chan.client_count) {
> +			dev_warn(od->ddev.dev,
> +				 "chan%d freed with %u client(s)\n",
> +				 c->dma_ch, c->vc.chan.client_count);
> +			continue;
> +		}

[Severity: High]
Does skipping the channel deallocation here introduce a Use-After-Free for
the parent controller?

By using continue here in omap_dma_free() without freeing the channel, we
leave the channel exposed to the active client. When omap_dma_remove()
finishes, devres will automatically free the parent controller struct
omap_dmadev.

When the client later attempts to release the channel:

drivers/dma/dmaengine.c:dma_release_channel() {
    ...
    if (--chan->device->privatecnt == 0)
        dma_cap_clear(DMA_PRIVATE, chan->device->cap_mask);
    ...
}

Doesn't this inevitably dereference the freed chan->device (which points to
the now-freed omap_dmadev), causing a Use-After-Free?

>  		kfree(c);
>  	}
>  }

[ ... ]

> @@ -1870,16 +1878,20 @@ 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);
> +		spin_lock_irq(&od->irq_lock);
> +		od->irq_enable_mask = 0;
> +		omap_dma_glbl_write(od, IRQENABLE_L1, 0);
> +		spin_unlock_irq(&od->irq_lock);
> +		omap_dma_glbl_read(od, IRQENABLE_L1);
>  	}

[Severity: Medium]
While this patch correctly adds the !omap_dma_legacy(od) guard here in
omap_dma_remove(), earlier patches in this series added similar IRQENABLE_L1
writes to the omap_dma_probe() error paths without this guard.

On OMAP1 legacy platforms where IRQENABLE_L1 is not defined in the reg_map
array, passing it to omap_dma_glbl_write() and omap_dma_glbl_read()
unconditionally triggers WARN_ON(1) during probe failure.

Should the probe error paths be updated to include this same
!omap_dma_legacy(od) check?

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

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