Re: [PATCH v2 52/64] dmaengine: tegra210-adma: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Incorrect ordering of bottom-half termination and IRQ freeing leaves a window for spurious execution and use-after-free.
- [High] Requesting the IRQ before powering up the device via Runtime PM causes a kernel panic if a pending or spurious interrupt fires.
--

--- Patch [52]: [PATCH v2 52/64] dmaengine: tegra210-adma: kill vchan BH on teardown ---
commit acf1e1ab4d7c08f6577663273485ed69a1d42e71
Author: Allen Pais <[email protected]>

dmaengine: tegra210-adma: kill vchan BH on teardown

Use dma_chan_kill_bh() for virt-dma cleanup.

> diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> index ceaee1e33e68a..598abcc4edaa1 100644
> --- a/drivers/dma/tegra210-adma.c
> +++ b/drivers/dma/tegra210-adma.c
> @@ -811,7 +811,7 @@ static void tegra_adma_free_chan_resources(struct dma_chan *dc)
>  
>  	tegra_adma_terminate_all(dc);
>  	vchan_free_chan_resources(&tdc->vc);
> -	tasklet_kill(&tdc->vc.task);
> +	dma_chan_kill_bh(&tdc->vc.chan);
>  	free_irq(tdc->irq, tdc);

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

If a delayed or spurious interrupt fires after dma_chan_kill_bh() flushes the
bottom-half, but before free_irq() unregisters the interrupt, couldn't the
interrupt handler execute and reschedule the bottom half via
vchan_cyclic_callback()?

Because free_irq() only waits for hardirq handlers and does not flush pending
bottom halves, this rescheduled bottom half could execute asynchronously after
the channel teardown completes and the client driver has freed its buffers.

Would it be safer to call free_irq() before dma_chan_kill_bh() to ensure
hardware events are disabled before flushing the work?

>  	pm_runtime_put(tdc2dev(tdc));
>  

[Severity: High]
This is also a pre-existing issue, but looking at the allocation side in
tegra_adma_alloc_chan_resources(), is it safe to request the IRQ before
powering up the device?

drivers/dma/tegra210-adma.c:tegra_adma_alloc_chan_resources() {
    ...
    ret = request_irq(tdc->irq, tegra_adma_isr, 0, dma_chan_name(dc), tdc);
    if (ret) {
        ...
    }

    ret = pm_runtime_resume_and_get(tdc2dev(tdc));
    ...
}

If the interrupt line is already active from a previous session or a spurious
hardware event, the CPU could immediately execute tegra_adma_isr() upon IRQ
registration.

Since tegra_adma_isr() unconditionally reads the ADMA_CH_INT_STATUS register,
and the device clock is disabled before pm_runtime_resume_and_get(), can
this access to unclocked hardware cause a synchronous external abort?

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