Re: [PATCH v2 01/64] dmaengine: add tasklet-backed channel BH helpers

[sashiko-bot@kernel.org]

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

New issues:
- [Low] API breakage leading to widespread build failures in downstream DMA drivers due to the removal of the `task` field from `struct virt_dma_chan` without updating its consumers.

Pre-existing issues:
- [Critical] Race condition causing a Use-After-Free of a cyclic descriptor's `tx_result` pointer when a channel is concurrently terminated.
--

--- Patch [1]: [PATCH v2 01/64] dmaengine: add tasklet-backed channel BH helpers ---
commit 9141d715bfba5e26881326cc343ca063eaf8458a
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: add tasklet-backed channel BH helpers

This patch introduces helpers to initialize, schedule, and kill a channel
bottom half. It converts virt-dma to use the new API, replacing its private
tasklet while preserving the execution context.

> diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
> index 7961172a780dd..89fcd51ddcabe 100644
> --- a/drivers/dma/virt-dma.c
> +++ b/drivers/dma/virt-dma.c
[ ... ]
> @@ -98,7 +98,7 @@ static void vchan_complete(struct tasklet_struct *t)
>  	}
>  	spin_unlock_irq(&vc->lock);
>  
> -	dmaengine_desc_callback_invoke(&cb, &vd->tx_result);
> +	dmaengine_desc_callback_invoke(&cb, vd ? &vd->tx_result : NULL);

[Severity: Critical]
This is a pre-existing issue, but does dropping vc->lock just before
referencing vd->tx_result open a race window for a use-after-free?

If a concurrent dmaengine_terminate_sync() calls the driver's terminate_all
callback (like bcm2835_dma_terminate_all()), the driver could acquire the
lock and free vd via vchan_dma_desc_free_list():

CPU1
vchan_complete()
    spin_unlock_irq(&vc->lock);

CPU2
dmaengine_terminate_sync()
    bcm2835_dma_terminate_all()
        spin_lock_irqsave(&vc->lock, flags);
        vchan_dma_desc_free_list() // frees vd
        spin_unlock_irqrestore(&vc->lock, flags);

CPU1
    dmaengine_desc_callback_invoke(&cb, vd ? &vd->tx_result : NULL);

Could evaluating vd->tx_result here pass a dangling pointer to the client
callback if the descriptor was freed by the concurrent termination?

>  	list_for_each_entry_safe(vd, _vd, &head, node) {
>  		dmaengine_desc_get_callback(&vd->tx, &cb);
[ ... ]
> diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
> index 59d9eabc8b674..abe2a44757442 100644
> --- a/drivers/dma/virt-dma.h
> +++ b/drivers/dma/virt-dma.h
> @@ -21,7 +21,6 @@ struct virt_dma_desc {
>  
>  struct virt_dma_chan {
>  	struct dma_chan	chan;
> -	struct tasklet_struct task;

[Severity: Low]
Does removing the task field here cause build failures in downstream drivers?

Several DMA drivers like bcm2835-dma, hisi_dma, idma64, and tegra210-adma
directly access chan->vc.task during driver removal or error paths to kill
the tasklet. Without updating those consumers, they will fail to compile.

>  	void (*desc_free)(struct virt_dma_desc *);
>  
>  	spinlock_t lock;

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