Re: [PATCHv2 9/9] dmaengine: mv_xor: add missing platform remove function

[sashiko-bot@kernel.org]

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

New issues:
- [High] Resource leaks in the newly introduced mv_xor_remove function: missing clk_disable_unprepare, clk_put, and irq_dispose_mapping.

Pre-existing issues:
- [High] Incorrect teardown sequence in mv_xor_channel_remove leads to Use-After-Free due to delayed free_irq and missing tasklet_kill.
--

commit 9ff45f7ddd6e175cd19bc1dcad7a0c33d39a5914
Author: Rosen Penev <rosenp@gmail.com>
dmaengine: mv_xor: add missing platform remove function

The driver was missing a remove callback, so channels, DMA devices, and IRQs
were never cleaned up on driver unbind. This patch implements mv_xor_remove
to undo probe operations.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..6065daec69ba9 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1460,8 +1460,19 @@ static int mv_xor_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void mv_xor_remove(struct platform_device *pdev)
> +{
> +	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
> +		if (xordev->channels[i])
> +			mv_xor_channel_remove(xordev->channels[i]);

[Severity: High]
Are we missing the teardown of the clock and IRQ mappings here?

Looking at the error path in mv_xor_probe(), we have explicit cleanup
for these resources:

drivers/dma/mv_xor.c:mv_xor_probe() {
...
err_channel_add:
    for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
        if (xordev->channels[i]) {
            mv_xor_channel_remove(xordev->channels[i]);
            if (pdev->dev.of_node)
                irq_dispose_mapping(xordev->channels[i]->irq);
        }

    if (!IS_ERR(xordev->clk)) {
        clk_disable_unprepare(xordev->clk);
        clk_put(xordev->clk);
    }
...
}

Does omitting clk_disable_unprepare(), clk_put(), and irq_dispose_mapping()
in mv_xor_remove() leak the clock reference and IRQ descriptors when the
driver is unbound?

[Severity: High]
This is a pre-existing issue, but calling mv_xor_channel_remove() here during
unbind exposes a teardown sequence problem.

In mv_xor_channel_remove(), dma_free_wc() is called before free_irq(), and
there is no tasklet_kill() for the irq_tasklet:

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
        ...
        dma_free_wc(dev, MV_XOR_POOL_SIZE,
                    mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
        ...
        free_irq(mv_chan->irq, mv_chan);

        return 0;
}

If an interrupt fires after dma_free_wc() but before free_irq(), the handler
can schedule the irq_tasklet which will access the freed descriptor pool.

Furthermore, without tasklet_kill(), a pending tasklet could execute after
mv_chan is freed by devres, resulting in a use-after-free.

Could this unbind sequence trigger these race conditions?

> +}
> +
>  static struct platform_driver mv_xor_driver = {
>  	.probe		= mv_xor_probe,
> +	.remove		= mv_xor_remove,

[ ... ]

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