Re: [PATCH v2 49/64] dmaengine: hisi: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Race condition during device teardown can lead to a use-after-free because the bottom-half work is killed before IRQs are synchronized, allowing a concurrent IRQ handler to reschedule the bottom-half work on freed memory.
- [High] The driver requests shared IRQs before fully initializing virtual channel structures, risking an uninitialized spinlock access and null pointer dereference if a shared or spurious interrupt occurs early.
--

--- Patch [49]: [PATCH v2 49/64] dmaengine: hisi: kill vchan BH on teardown ---
commit f8a5265e01e8ff281f13c0af8c64babbb3148800
Author: Allen Pais <[email protected]>

dmaengine: hisi: kill vchan BH on teardown
    
This commit replaces tasklet_kill() with dma_chan_kill_bh() for virt-dma 
cleanup during device teardown.

> diff --git a/drivers/dma/hisi_dma.c b/drivers/dma/hisi_dma.c
> index 28bf818f9aa63..39bc88c465138 100644
> --- a/drivers/dma/hisi_dma.c
> +++ b/drivers/dma/hisi_dma.c
> @@ -720,7 +720,7 @@ static void hisi_dma_disable_qps(struct hisi_dma_dev *hdma_dev)
>  
>  	for (i = 0; i < hdma_dev->chan_num; i++) {
>  		hisi_dma_disable_qp(hdma_dev, i);
> -		tasklet_kill(&hdma_dev->chan[i].vc.task);
> +		dma_chan_kill_bh(&hdma_dev->chan[i].vc.chan);

[Severity: High]
This is a pre-existing issue, but does this teardown sequence leave us open to
a use-after-free regression?

During device unbind, devres executes hisi_dma_disable_hw_channels() which
calls hisi_dma_disable_qps(). However, devm_free_irq() hasn't been called yet. 

If hisi_dma_irq() is concurrently executing on another CPU, it can call
vchan_cookie_complete():

drivers/dma/hisi_dma.c:hisi_dma_irq() {
	...
	vchan_cookie_complete(&desc->vd);
	...
}

This can schedule the bottom half after dma_chan_kill_bh() has already
flushed it. When devres subsequently frees the device memory, could the
scheduled bottom half execute on the freed channel structure?

>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a shared
interrupt to fire before the channel structures are fully initialized?

In hisi_dma_enable_hw_channels(), the driver requests shared IRQs before
calling hisi_dma_enable_qps(), which initializes the channels:

drivers/dma/hisi_dma.c:hisi_dma_enable_hw_channels() {
	...
	ret = hisi_dma_request_qps_irq(hdma_dev);
	...
}

If a shared or spurious interrupt fires immediately, hisi_dma_irq() would
execute and dereference an uninitialized pointer and lock:

drivers/dma/hisi_dma.c:hisi_dma_irq() {
	...
	struct hisi_dma_dev *hdma_dev = chan->hdma_dev;
	...
	spin_lock(&chan->vc.lock);
	...
}

Could this lead to a NULL pointer dereference or uninitialized lock access?

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