Re: [PATCH v2 34/64] dmaengine: mtk-cqdma: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Incorrect teardown order in mtk_cqdma_remove() allows physical channel tasklets to reschedule virtual channel bottom halves after they have been killed, leading to a use-after-free.
- [High] Manual removal of channels from the DMA device channel list via list_del() before unregistration bypasses dmaengine cleanup, leaking memory and IDA indexes.
- [High] The DMA device is registered before physical channel tasklets and hardware are initialized, allowing early interrupts to schedule uninitialized tasklets.
--

--- Patch [34]: [PATCH v2 34/64] dmaengine: mtk-cqdma: kill vchan BH on teardown ---
commit 94dd78bb3e2fc0bc1ffe5fdde7322901f8e3ea2c
Author: Allen Pais <[email protected]>

dmaengine: mtk-cqdma: kill vchan BH on teardown
    
This patch updates the virt-dma channel cleanup in the MediaTek CQDMA driver
to use the standard dma_chan_kill_bh() helper rather than directly calling
tasklet_kill().

> diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
> index 80791e30aec24..3b0f1d8fa2055 100644
> --- a/drivers/dma/mediatek/mtk-cqdma.c
> +++ b/drivers/dma/mediatek/mtk-cqdma.c
> @@ -895,7 +895,7 @@ static void mtk_cqdma_remove(struct platform_device *pdev)
>  		vc = &cqdma->vc[i];
>  
>  		list_del(&vc->vc.chan.device_node);

[Severity: High]
This isn't a bug introduced by this patch, but does calling list_del() here
cause a memory and IDA index leak?

By manually removing the channel from the dmaengine device list before calling
dma_async_device_unregister(), the core subsystem's unregistration loop will
see an empty list.

This causes __dma_async_device_channel_unregister() to be skipped, which
means free_percpu(), sysfs cleanup, and ida_free() are never executed.

> -		tasklet_kill(&vc->vc.task);
> +		dma_chan_kill_bh(&vc->vc.chan);

[Severity: High]
This is a pre-existing issue, but does this teardown order leave a window for
a use-after-free?

If a physical channel interrupt fires after dma_chan_kill_bh() is called but
before the hardware interrupts are disabled and tasklets are killed below,
the interrupt handler will schedule the physical channel tasklet.

This tasklet processes completions and calls vchan_cookie_complete(), which
re-queues the virtual channel bottom-half work. After mtk_cqdma_remove()
returns, devres frees the cqdma device, leaving the re-queued work item
referencing freed memory.

Would it be safer to disable physical interrupts and kill tasklets before
killing the virtual channel bottom halves?

>  	}
>  
>  	/* disable interrupt */

[ ... ]

While reviewing this file, another pre-existing issue was noticed in
mtk_cqdma_probe():

[Severity: High]
This is a pre-existing issue, but is the DMA device registered before the
physical channel tasklets and hardware are fully initialized?

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_probe() {
    ...
    err = dma_async_device_register(dd);
    ...
    err = mtk_cqdma_hw_init(cqdma);
    ...
    for (i = 0; i < cqdma->dma_channels; ++i)
        tasklet_setup(&cqdma->pc[i]->tasklet, mtk_cqdma_tasklet_cb);
    ...
}

Once dma_async_device_register() is called, the controller is globally
visible. If a client probes asynchronously and immediately requests a channel,
it could trigger an early interrupt before tasklet_setup() completes,
potentially dereferencing a NULL pointer for the uninitialized tasklet
callback.

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