Re: [PATCH v2 57/64] dmaengine: edma: use dma_chan_kill_bh

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Channel unregistration bypass leading to memory leak and sysfs Use-After-Free.
- [High] Missing BH cancellation for `memcpy` channels causing Use-After-Free during teardown.
--

--- Patch [57]: [PATCH v2 57/64] dmaengine: edma: use dma_chan_kill_bh ---
commit 03c05180f9212195192cc327d33610888eef85fe
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: edma: use dma_chan_kill_bh

virt-dma now dispatches completion callbacks through per-channel BH work
instead of its tasklet. Cancel that work during teardown before channel
storage is released.

> diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c
> index d97db5af35555..c0b16cd247525 100644
> --- a/drivers/dma/ti/edma.c
> +++ b/drivers/dma/ti/edma.c
> @@ -2560,7 +2560,7 @@ static void edma_cleanupp_vchan(struct dma_device *dmadev)
>  	list_for_each_entry_safe(echan, _echan,
>  			&dmadev->channels, vchan.chan.device_node) {
>  		list_del(&echan->vchan.chan.device_node);

[Severity: High]
This is a pre-existing issue, but does removing the channel from the
device list here cause problems during device unregistration?

When a user unbinds the driver, edma_remove() calls this function which
unconditionally removes all slave channels from dmadev->channels. When
dma_async_device_unregister() subsequently runs, it iterates over the
now-empty list and entirely skips unregistering the channels.

Can this cause sysfs device nodes, IDAs, and per-cpu variables to leak?
If the orphaned sysfs attributes (such as in_use or bytes_transferred)
are read after the driver is unbound, could it trigger a use-after-free
since the underlying chan structures are freed by devres?

> -		tasklet_kill(&echan->vchan.task);
> +		dma_chan_kill_bh(&echan->vchan.chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but are we missing a cleanup step for the
memcpy channels?

Looking at edma_remove(), edma_cleanupp_vchan() is explicitly called for
ecc->dma_slave, but it appears to be completely omitted for ecc->dma_memcpy:

drivers/dma/ti/edma.c:edma_remove() {
    ...
    edma_cleanupp_vchan(&ecc->dma_slave);
    ...
    dma_async_device_unregister(&ecc->dma_slave);
    if (ecc->dma_memcpy)
        dma_async_device_unregister(ecc->dma_memcpy);
    ...
}

Because the cleanup is skipped, the BH cancellation is never performed
for dma_memcpy channels. If a background transfer completes and schedules
the BH just before the driver is unbound, could the pending BH execute
after devres has freed the channel structures and cause a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=57