Re: [PATCH v3 12/34] dmaengine: mv_xor: convert irq tasklet to dmaengine BH

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

Pre-existing issues:
- [High] Missing `dmaengine_kill_bh()` on error paths and device removal, leading to potential use-after-free of device resources.
- [High] The IRQ is requested and unmasked before critical locks and lists are initialized, leading to a NULL pointer dereference.
--

--- Patch [12]: [PATCH v3 12/34] dmaengine: mv_xor: convert irq tasklet to dmaengine BH ---
commit 59f9ab1622f4d6943ebdad668684fff90fe01b57
Author: Allen Pais <[email protected]>

dmaengine: mv_xor: convert irq tasklet to dmaengine BH

Replace the per-channel tasklet with the shared dmaengine BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

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

[ ... ]

> @@ -1107,7 +1107,7 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  
>  	mv_chan->mmr_base = xordev->xor_base;
>  	mv_chan->mmr_high_base = xordev->xor_high_base;
> -	tasklet_setup(&mv_chan->irq_tasklet, mv_xor_tasklet);
> +	dmaengine_init_bh(&mv_chan->dmachan, mv_xor_tasklet);

[Severity: High]
This isn't a bug introduced by this patch, but does the error path miss a
teardown for this BH work?

If the probe fails (e.g., during self-test timeouts) or the driver is unbound,
free_irq() is called in mv_xor_channel_remove() and the err_free_irq path of
mv_xor_channel_add(), but the scheduled bottom-half work is never canceled
via dmaengine_kill_bh(&mv_chan->dmachan).

Since mv_chan is subsequently freed, couldn't this lead to a use-after-free
if a scheduled bottom-half executes after the memory is freed?

>  
>  	/* clear errors before enabling interrupts */
>  	mv_chan_clear_err_status(mv_chan);

[Severity: High]
This is a pre-existing issue, but is it safe to request and unmask the IRQ
here before initializing the lock and lists?

A few lines down from here in mv_xor_channel_add():

	ret = request_irq(mv_chan->irq, mv_xor_interrupt_handler,
			  0, dev_name(&pdev->dev), mv_chan);
	if (ret)
		goto err_free_dma;

	mv_chan_unmask_interrupts(mv_chan);
	...
	spin_lock_init(&mv_chan->lock);
	INIT_LIST_HEAD(&mv_chan->chain);

If a bootloader left a pending End-Of-Chain (EOC) interrupt, couldn't it
fire immediately upon unmasking? This would schedule the BH, which takes the
uninitialized lock and checks list_empty(&mv_chan->chain).

Since devm_kzalloc zeroed the memory, list_empty() would return false,
causing a NULL pointer dereference in the list_for_each_entry_safe() loop.

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