Re: [PATCH v2 58/64] dmaengine: qcom-adm: use dma_chan_kill_bh

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

Pre-existing issues:
- [High] Teardown race condition in adm_dma_remove() leading to use-after-free
- [High] Missing device_synchronize callback exposes DMA clients to use-after-free
--

--- Patch [58]: [PATCH v2 58/64] dmaengine: qcom-adm: use dma_chan_kill_bh ---
commit be25e9ce5563c30843636df639280bce697e48d5
Author: Allen Pais <[email protected]>

dmaengine: qcom-adm: 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/qcom/qcom_adm.c b/drivers/dma/qcom/qcom_adm.c
> index 07fbe32d31fa4..ea24e2de51544 100644
> --- a/drivers/dma/qcom/qcom_adm.c
> +++ b/drivers/dma/qcom/qcom_adm.c
> @@ -918,8 +918,8 @@ static void adm_dma_remove(struct platform_device *pdev)
>  		/* mask IRQs for this channel/EE pair */
>  		writel(0, adev->regs + ADM_CH_RSLT_CONF(achan->id, adev->ee));
>  
> -		tasklet_kill(&adev->channels[i].vc.task);
> -		adm_terminate_all(&adev->channels[i].vc.chan);
> +		dma_chan_kill_bh(&achan->vc.chan);

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

If adm_dma_remove() calls dma_chan_kill_bh() before devm_free_irq()
synchronizes with the interrupt handler, a concurrent interrupt can still
fire:

cpu1
adm_dma_remove()
    writel(0, ...)
    dma_chan_kill_bh()

cpu2
adm_dma_irq()
    vchan_cookie_complete()
        dma_chan_schedule_bh() 

Because cpu1 proceeds to devm_free_irq() which waits for the interrupt handler
to finish but does not flush the newly queued bottom-half work, the device
context is subsequently freed. The workqueue could then execute the scheduled
bottom-half work on the freed memory. 

Could devm_free_irq() be explicitly called before tearing down the channels?

[Severity: High]
This isn't a bug introduced by this patch, but does this driver also lack a
device_synchronize callback during its initialization in adm_dma_probe()?

When clients call dmaengine_terminate_sync() to abort transactions, it relies
on device_synchronize (usually mapped to vchan_synchronize) to wait for
callbacks to complete.

Without it, dmaengine_terminate_sync() returns immediately, and clients may
free their buffers while the virt-dma bottom-half work is still executing.

Can this lead to a use-after-free in the client's completion callback?

> +		adm_terminate_all(&achan->vc.chan);
>  	}
>  
>  	devm_free_irq(adev->dev, adev->irq, adev);

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