Re: [PATCH v3 01/34] dmaengine: add tasklet-backed channel BH helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free due to channel bottom-half (tasklet) being killed before hardware interrupts are disabled and synchronized during device removal.
--

--- Patch [1]: [PATCH v3 01/34] dmaengine: add tasklet-backed channel BH helpers ---
commit 552ba608049b66637e1d6db5b616db6c9c33204d
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: add tasklet-backed channel BH helpers

This commit introduces backend-independent helpers to initialize, schedule,
and kill channel bottom halves. It converts virt-dma and updates various
drivers to use the new API instead of directly overriding or killing
tasklets.

> diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
> --- a/drivers/dma/mediatek/mtk-cqdma.c
> +++ b/drivers/dma/mediatek/mtk-cqdma.c
> @@ -895,7 +895,7 @@ static void mtk_cqdma_remove(struct platform_device *pdev)
>  		vc = &cqdma->vc[i];
>  
>  		list_del(&vc->vc.chan.device_node);
> -		tasklet_kill(&vc->vc.task);
> +		dmaengine_kill_bh(&vc->vc.chan);
>  	}
>  
>  	/* disable interrupt */

[Severity: High]
This is a pre-existing issue, but could this teardown sequence result in a
use-after-free?

In mtk_cqdma_remove(), the channel bottom-halves are killed before the
physical channel interrupts are masked and synchronized. If an interrupt fires
within this window, the interrupt handler processes descriptors and can
reschedule the BH via dmaengine_schedule_bh(). 

Since the device is being removed, devres will subsequently free the channel
memory, potentially causing the recently scheduled BH to execute on freed
memory.

[ ... ]

> diff --git a/drivers/dma/mediatek/mtk-hsdma.c b/drivers/dma/mediatek/mtk-hsdma.c
> --- a/drivers/dma/mediatek/mtk-hsdma.c
> +++ b/drivers/dma/mediatek/mtk-hsdma.c
> @@ -1020,7 +1020,7 @@ static void mtk_hsdma_remove(struct platform_device *pdev)
>  		vc = &hsdma->vc[i];
>  
>  		list_del(&vc->vc.chan.device_node);
> -		tasklet_kill(&vc->vc.task);
> +		dmaengine_kill_bh(&vc->vc.chan);
>  	}
>  
>  	/* Disable DMA interrupt */

[Severity: High]
This is a pre-existing issue, but is it safe to kill the BH here before
disabling and synchronizing the DMA interrupts? 

Similar to mtk_cqdma_remove() above, this opens a window where an incoming
interrupt in mtk_hsdma_remove() can reschedule the BH just before the device
is torn down.

[ ... ]

> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -1215,7 +1215,7 @@ static void pxad_free_channels(struct dma_device *dmadev)
>  	list_for_each_entry_safe(c, cn, &dmadev->channels,
>  				 vc.chan.device_node) {
>  		list_del(&c->vc.chan.device_node);
> -		tasklet_kill(&c->vc.task);
> +		dmaengine_kill_bh(&c->vc.chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does pxad_free_channels() suffer from a
similar race? 

This function is called from pxad_remove(). The driver relies entirely on
devres to free the IRQ after the remove function returns, meaning interrupts
remain fully active during and after the BH is killed. 

A triggered interrupt could reschedule the BH right before devres tears down
the channel structures.

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