Re: [PATCH v2 45/64] dmaengine: mtk-uart-apdma: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Resource leak on driver unbind due to prematurely emptying the DMA channels list.
- [High] Use-After-Free of channel context due to orphaned IRQ handler on unbind.
- [High] Double decrement of PM runtime usage counter on failure in channel allocation.
--

--- Patch [45]: [PATCH v2 45/64] dmaengine: mtk-uart-apdma: kill vchan BH on teardown ---
commit 58283f2545c03ebd5b55eff5e9a5d8fdde273b4d
Author: Allen Pais <[email protected]>

dmaengine: mtk-uart-apdma: kill vchan BH on teardown

Use dma_chan_kill_bh() for virt-dma cleanup.

> diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
> index c269d84d7bd2b..4d89927922f52 100644
> --- a/drivers/dma/mediatek/mtk-uart-apdma.c
> +++ b/drivers/dma/mediatek/mtk-uart-apdma.c
> @@ -312,7 +312,7 @@ static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
>  
>  	free_irq(c->irq, chan);

[Severity: High]
This is a pre-existing issue, but is it possible for a use-after-free
to occur on unbind?

The IRQ is requested without devres tracking in
mtk_uart_apdma_alloc_chan_resources(). It is only freed here in
mtk_uart_apdma_free_chan_resources().

If the driver is unbound while a client still actively holds a channel,
mtk_uart_apdma_remove() completes without forcing the channel to be
freed and without disabling the active IRQ. Since the mtk_chan context
is managed by devres, it is automatically freed upon remove() completion.
If a hardware interrupt fires after this, could the orphaned handler
dereference the already-freed mtk_chan structure?

> -	tasklet_kill(&c->vc.task);
> +	dma_chan_kill_bh(&c->vc.chan);
>  
>  	vchan_free_chan_resources(&c->vc);
>  
> @@ -463,7 +463,7 @@ static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
>  			struct mtk_chan, vc.chan.device_node);
>  
>  		list_del(&c->vc.chan.device_node);

[Severity: High]
This isn't a bug introduced by this patch, but does this list removal
cause a resource leak during driver unbind?

In mtk_uart_apdma_remove(), the driver calls mtk_uart_apdma_free()
before dma_async_device_unregister(). This loop explicitly removes
each channel via list_del(), leaving the mtkd->ddev.channels list empty.

Immediately after, dma_async_device_unregister() iterates over the
now-empty device->channels list to tear down the channels. Because the
list is empty, does this cause the teardown to be entirely skipped,
permanently leaking the per-CPU chan->local memory, IDR slots, and
leaving orphaned sysfs nodes?

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

[Severity: High]
This is a pre-existing issue, but does mtk_uart_apdma_alloc_chan_resources()
double decrement the PM runtime usage counter on failure?

drivers/dma/mediatek/mtk-uart-apdma.c:mtk_uart_apdma_alloc_chan_resources() {
    ...
    ret = pm_runtime_resume_and_get(mtkd->ddev.dev);
    if (ret < 0) {
        pm_runtime_put_noidle(chan->device->dev);
        return ret;
    }
    ...
}

Since pm_runtime_resume_and_get() intrinsically drops the PM runtime
usage counter if it fails (returns < 0), does calling pm_runtime_put_noidle()
again in the error path result in a double decrement and unbalanced
runtime PM state?

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