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

On 27-07-26, 20:56, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

This one seems valid.
We need to replace the callsites with dma_chan_kill_bh() here

-- 
~Vinod