Re: [PATCHv3] dmaengine: at_hdmac: use devm APIs in probe()

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

Pre-existing issues:
- [High] The driver fails to kill `vchan` tasklets during driver removal and probe error paths, risking a use-after-free.
- [High] Devres reverse-cleanup order destroys DMA pools before freeing the IRQ, risking a use-after-free with concurrent ISRs.
--

--- Patch [1]: [PATCHv3] dmaengine: at_hdmac: use devm APIs in probe() ---
commit 100916adb459d4781232f09cf49ed4747fa5f88a
Author: Rosen Penev <[email protected]>

dmaengine: at_hdmac: use devm APIs in probe()

Convert resource allocation and setup in probe() to use devm-managed APIs.
Simplify the error handling path and removes explicit cleanup code.

> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index e5b30a57c477a..f16cf29ca5487 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1980,40 +1980,34 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	atdma->dma_device.cap_mask = plat_dat->cap_mask;
>  	atdma->all_chan_mask = (1 << plat_dat->nr_channels) - 1;
>  
> -	atdma->clk = devm_clk_get(&pdev->dev, "dma_clk");
> +	atdma->clk = devm_clk_get_enabled(&pdev->dev, "dma_clk");
>  	if (IS_ERR(atdma->clk))
>  		return PTR_ERR(atdma->clk);
>  
> -	err = clk_prepare_enable(atdma->clk);
> -	if (err)
> -		return err;
> -
>  	/* force dma off, just in case */
>  	at_dma_off(atdma);
>  
> -	err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
> +	err = devm_request_irq(&pdev->dev, irq, at_dma_interrupt, 0, "at_hdmac", atdma);

[Severity: High]
This isn't a bug introduced by this patch, but requesting the IRQ here before
the DMA pools are created establishes an unsafe devres cleanup order.

Because devres destroys resources in reverse order, devm will destroy the
DMA pools before freeing the IRQ.

If an ISR is actively running on another CPU during driver unbind and enters
an error path, could it dereference the already-freed pool memory, leading to
a use-after-free?

>  	if (err)
> -		goto err_irq;
> +		return err;
>  
>  	platform_set_drvdata(pdev, atdma);
>  
>  	/* create a pool of consistent memory blocks for hardware descriptors */
> -	atdma->lli_pool = dma_pool_create("at_hdmac_lli_pool",
> +	atdma->lli_pool = dmam_pool_create("at_hdmac_lli_pool",
>  					  &pdev->dev, sizeof(struct at_lli),
>  					  4 /* word alignment */, 0);

[ ... ]

> @@ -2121,22 +2103,12 @@ static void at_dma_remove(struct platform_device *pdev)
>  	struct dma_chan		*chan, *_chan;
>  
>  	at_dma_off(atdma);
> -	if (pdev->dev.of_node)
> -		of_dma_controller_free(pdev->dev.of_node);
> -	dma_async_device_unregister(&atdma->dma_device);
> -
> -	dma_pool_destroy(atdma->memset_pool);
> -	dma_pool_destroy(atdma->lli_pool);
> -	free_irq(platform_get_irq(pdev, 0), atdma);
>  
>  	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
>  			device_node) {
>  		/* Disable interrupts */
>  		atc_disable_chan_irq(atdma, chan->chan_id);
> -		list_del(&chan->device_node);
>  	}

[Severity: High]
This is a pre-existing issue, but this driver unbind path does not synchronously
kill pending vchan tasklets before returning.

If an interrupt fires during or just before unbind, the IRQ handler might
schedule the channel's tasklet.

While at_dma_remove() masks interrupts, it doesn't wait for or kill already
scheduled tasklets via tasklet_kill(). Is there a risk that devres will free
the memory while the tasklet continues executing, resulting in a use-after-free?

> -
> -	clk_disable_unprepare(atdma->clk);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1