Re: [PATCH v3 25/34] dmaengine: mv_xor_v2: use channel BH helpers

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

Pre-existing issues:
- [High] Incorrect teardown ordering in `mv_xor_v2_remove()` frees DMA memory before disabling interrupts and killing the bottom-half work.
- [High] The IRQ handler is registered before the bottom-half work is initialized and before descriptor memory is allocated, creating a race condition with early interrupts.
- [High] Probe error paths fail to kill the initialized bottom-half work, risking a use-after-free if an interrupt schedules the work before probe fails.
--

--- Patch [25]: [PATCH v3 25/34] dmaengine: mv_xor_v2: use channel BH helpers ---
commit 5bac7d61a8218a20b1f3b692adf51b25c890e0c1
Author: Allen Pais <[email protected]>

dmaengine: mv_xor_v2: use channel BH helpers
    
Replace the device tasklet with the per-channel BH helper so descriptor
callbacks run through dmaengine_*_bh.

> diff --git a/drivers/dma/mv_xor_v2.c b/drivers/dma/mv_xor_v2.c
> index cad4d4fb51ac3..14d563ec6bf0e 100644
> --- a/drivers/dma/mv_xor_v2.c
> +++ b/drivers/dma/mv_xor_v2.c

[ ... ]

> @@ -760,7 +758,7 @@ static int mv_xor_v2_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_msi_irqs;
>  
> -	tasklet_setup(&xor_dev->irq_tasklet, mv_xor_v2_tasklet);
> +	dmaengine_init_bh(&xor_dev->dmachan, mv_xor_v2_tasklet);
>  
>  	xor_dev->desc_size = mv_xor_v2_set_desc_size(xor_dev);
>  

[Severity: High]
This is a pre-existing issue, but registering the IRQ handler before
allocating the descriptor memory and initializing the bottom-half seems risky.

If the kernel is booted via kexec and there are pending hardware interrupts,
could the IRQ handler fire immediately upon registration and schedule the
bottom-half work?

If that happens, mv_xor_v2_tasklet() would execute and dereference 
xor_dev->hw_desq_virt before dma_alloc_coherent() has initialized it.
Would it be safer to request the IRQ after the structures are fully set up?

[Severity: High]
This is a pre-existing issue, but looking at the error handling in probe:

	xor_dev->hw_desq_virt =
		dma_alloc_coherent(&pdev->dev, ...);
	if (!xor_dev->hw_desq_virt) {
		ret = -ENOMEM;
		goto free_msi_irqs;
	}

In error paths like goto free_msi_irqs or goto free_hw_desq, the probe returns
without killing the initialized bottom-half work. If an early interrupt already
scheduled the bottom-half, could this lead to a use-after-free when devres
frees the device structures but the scheduled workqueue still executes?

[ ... ]

> @@ -869,7 +867,7 @@ static void mv_xor_v2_remove(struct platform_device *pdev)
>  
>  	platform_device_msi_free_irqs_all(&pdev->dev);
>  
> -	tasklet_kill(&xor_dev->irq_tasklet);
> +	dmaengine_kill_bh(&xor_dev->dmachan);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at the full
mv_xor_v2_remove() function:

static void mv_xor_v2_remove(struct platform_device *pdev)
{
	...
	dma_free_coherent(&pdev->dev,
			  xor_dev->desc_size * MV_XOR_V2_DESC_NUM,
			  xor_dev->hw_desq_virt, xor_dev->hw_desq);

	devm_free_irq(&pdev->dev, xor_dev->irq, xor_dev);
	platform_device_msi_free_irqs_all(&pdev->dev);
	dmaengine_kill_bh(&xor_dev->dmachan);
}

Since dma_free_coherent() frees the descriptor memory before the bottom-half
is killed, could an interrupt fire and schedule the bottom-half during this
window?

If mv_xor_v2_tasklet() runs after dma_free_coherent(), it would dereference
the freed xor_dev->hw_desq_virt memory. Should the IRQ be freed and
dmaengine_kill_bh() be called before tearing down the DMA memory?

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