Re: [PATCHv2 7/9] dmaengine: mv_xor: use devm for dma pool and irq

[sashiko-bot@kernel.org]

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

New issues:
- [High] Resource lifecycle violation: calling irq_dispose_mapping() before deferred devm_request_irq() cleanup leaks irqaction and risks interrupt storms.

Pre-existing issues:
- [High] Missing tasklet_kill() during channel teardown leads to potential Use-After-Free.
--

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

dmaengine: mv_xor: use devm for dma pool and irq
    
Replace dma_alloc_wc() with dmam_alloc_attrs() and request_irq() with
devm_request_irq(). This eliminates the need for manual cleanup of the dma
pool and irq in both the channel remove function and the channel add
error labels, removing the err_free_irq and err_free_dma labels entirely.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..2176be478c86b 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c

[ ... ]

> @@ -1025,8 +1023,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
>  		list_del(&chan->device_node);
>  	}
>  
> -	free_irq(mv_chan->irq, mv_chan);
> -
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the driver never calls
tasklet_kill(&mv_chan->irq_tasklet) during device teardown here in
mv_xor_channel_remove().

If an interrupt fires and schedules the tasklet during a probe failure,
could the tasklet execute after devres has freed the mv_chan structure,
resulting in a Use-After-Free?

This risk might be exacerbated by removing the synchronous free_irq() call
here, which previously helped silence interrupts earlier in the teardown
process.

[ ... ]

> @@ -1138,14 +1134,14 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  		ret = mv_chan_memcpy_self_test(mv_chan);
>  		dev_dbg(&pdev->dev, "memcpy self test returned %d\n", ret);
>  		if (ret)
> -			goto err_free_irq;
> +			goto err_unmap_dst;
>  	}

[Severity: High]
By deferring the interrupt teardown to devres, could this lead to an issue
where the IRQ mapping is disposed before devm can execute free_irq()?

Looking at mv_xor_probe() which calls mv_xor_channel_add(), if a channel
fails to add, it explicitly calls irq_dispose_mapping() on the error path:

drivers/dma/mv_xor.c:mv_xor_probe() {
    ...
    if (IS_ERR(chan)) {
        ret = PTR_ERR(chan);
        irq_dispose_mapping(irq);
        goto err_channel_add;
    }
    ...
}

It also disposes the mapping for previously successful channels during cleanup:

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);
        }
    }
    ...
}

Since devres runs after probe() completes, this destroys the IRQ descriptor
while the interrupt is still actively requested. When devres later invokes
free_irq(), it might fail to find the descriptor, resulting in a leak of
the irqaction structure.

Additionally, since the error path no longer calls free_irq() synchronously,
does this leave the interrupt unmasked during probe teardown, risking
interrupt storms?

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