Re: [PATCH v2 47/64] dmaengine: loongson1-apb: kill vchan BH on teardown

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

New issues:
- [High] The patch series breaks bisectability and leaves the tree unbuildable by prematurely removing the `task` member from `struct virt_dma_chan` before updating all consuming drivers.

Pre-existing issues:
- [High] Premature removal of DMA channels from the dma_device channels list bypasses dmaengine core unregistration, causing massive resource leaks (sysfs nodes, IDA entries, per-CPU memory) and leading to a Use-After-Free vulnerability.
- [High] Flushing the bottom-half work without stopping the hardware and masking interrupts leaves a race window where an active IRQ can reschedule the bottom-half on freed memory, leading to a Use-After-Free.
--

--- Patch [47]: [PATCH v2 47/64] dmaengine: loongson1-apb: kill vchan BH on teardown ---
commit 3e0d38c61679a38b8f14c3f667abc7411f9f4c83
Author: Allen Pais <[email protected]>

dmaengine: loongson1-apb: kill vchan BH on teardown
    
Use dma_chan_kill_bh() for virt-dma cleanup.

[Severity: High]
Does this patch series introduce a build regression that breaks
bisectability?

If the task member was removed from struct virt_dma_chan earlier in this
series, unmodified drivers that still rely on it, such as bcm2835-dma.c in
bcm2835_dma_free_chan_resources(), will fail to build.

Should all consuming drivers be updated to the new API before the task
member is removed from the core virt_dma_chan structure?

> --- a/drivers/dma/loongson/loongson1-apb-dma.c
> +++ b/drivers/dma/loongson/loongson1-apb-dma.c
> @@ -552,7 +552,7 @@ static void ls1x_dma_chan_remove(struct ls1x_dma *dma)
>  
>  		if (chan->vc.chan.device == &dma->ddev) {
>  			list_del(&chan->vc.chan.device_node);

[Severity: High]
This is a pre-existing issue, but does removing the channel from the
device_node list here bypass the dmaengine core unregistration?

When the devres-managed dma_async_device_unregister() later runs, the
channels list will be empty, causing it to silently skip unregistering the
channel devices. This leaks the channel device structures, their IDA
entries, and per-CPU memory.

Furthermore, because the channel device is never unregistered, the IRQ
handler registered via devm_request_irq() in ls1x_dma_alloc_chan_resources()
is never freed and remains active indefinitely. Since the platform device's
devres will eventually free the channel memory, can an active IRQ trigger a
use-after-free on that freed memory?

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

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition here
since the hardware isn't stopped first?

In ls1x_dma_remove(), the hardware DMA controller is not stopped and its
interrupts are not masked before teardown. Because the IRQ is still active,
a hardware interrupt can fire immediately after dma_chan_kill_bh() returns.

If the interrupt handler calls vchan_cookie_complete(), which then calls
dma_chan_schedule_bh(), it will requeue the work. When the ls1x_dma
structure containing the work item is later freed by devres, could the
workqueue process freed memory and cause a use-after-free or corrupt the
workqueue linked list?

>  		}
>  	}
>  }

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