Re: [PATCH v2 18/64] dmaengine: xgene-dma: wire descriptor cleanup to dma_chan BH

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

Pre-existing issues:
- [High] During device teardown, the driver frees the IRQ before flushing the bottom half workqueue. This results in the bottom half handler calling `enable_irq()` on a freed IRQ descriptor, which violates the API and can cause warnings or oopses in the IRQ subsystem.
- [High] Potential spinlock recursion deadlock in `xgene_dma_cleanup_descriptors()` when called from process context via `xgene_dma_free_chan_resources()`.
- [High] Hardware interrupts are unmasked before the bottom halves are initialized in the probe sequence, allowing an early interrupt to permanently hang the channel.
- [High] In the probe error path, bottom halves are flushed before hardware interrupts are masked, leading to a race condition that can result in a Use-After-Free of driver structures when the workqueue later executes.
- [High] The driver returns ring slots to the hardware before clearing the descriptor completion signature, risking permanently lost completions.
--

--- Patch [18]: [PATCH v2 18/64] dmaengine: xgene-dma: wire descriptor cleanup to dma_chan BH ---
commit 3558adb2da1479ded074f1f3135c3903cdea9f57
Author: Allen Pais <[email protected]>

dmaengine: xgene-dma: wire descriptor cleanup to dma_chan BH

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/xgene-dma.c b/drivers/dma/xgene-dma.c
> index fa1173e499009..8d0428c2944ef 100644
> --- a/drivers/dma/xgene-dma.c
> +++ b/drivers/dma/xgene-dma.c
> @@ -975,9 +973,10 @@ static enum dma_status xgene_dma_tx_status(struct dma_chan *dchan,
>  	return dma_cookie_status(dchan, cookie, txstate);
>  }
>  
> -static void xgene_dma_tasklet_cb(struct tasklet_struct *t)
> +static void xgene_dma_tasklet_cb(struct dma_chan *c)
>  {
> -	struct xgene_dma_chan *chan = from_tasklet(chan, t, tasklet);
> +	struct xgene_dma_chan *chan = container_of(c, struct xgene_dma_chan,
> +						   dma_chan);
>  
>  	/* Run all cleanup for descriptors which have been completed */
>  	xgene_dma_cleanup_descriptors(chan);

[Severity: High]
This is a pre-existing issue, but can this cause a spinlock recursion
deadlock?

Looking at xgene_dma_free_chan_resources(), it runs in process context and
calls xgene_dma_cleanup_descriptors(), which acquires chan->lock using
spin_lock() without disabling softirqs.

If a hardware interrupt occurs on the same CPU, it schedules the bottom half.
On interrupt exit, the softirq executes xgene_dma_tasklet_cb(), which also
calls xgene_dma_cleanup_descriptors() and attempts to acquire the
already-held chan->lock.

[Severity: High]
This is a pre-existing issue, but does this out-of-order execution risk
permanently losing DMA completions?

In xgene_dma_cleanup_descriptors(), the driver writes to ring->cmd to notify
the hardware that a descriptor slot is free, and then clears the descriptor's
completion signature in memory:

xgene_dma_cleanup_descriptors() {
    ...
    iowrite32(-1, ring->cmd);
    desc_hw->m0 = cpu_to_le64(XGENE_DMA_DESC_EMPTY_SIGNATURE);
    ...
}

Since the hardware engine is active, could it immediately write a new
completion signature into that slot before the CPU executes the assignment,
causing the CPU to unconditionally overwrite the valid completion signature
with the empty signature?

[ ... ]

> @@ -1539,7 +1538,7 @@ static int xgene_dma_async_register(struct xgene_dma *pdma, int id)
>  	INIT_LIST_HEAD(&chan->ld_pending);
>  	INIT_LIST_HEAD(&chan->ld_running);
>  	INIT_LIST_HEAD(&chan->ld_completed);
> -	tasklet_setup(&chan->tasklet, xgene_dma_tasklet_cb);
> +	dma_chan_init_bh(&chan->dma_chan, xgene_dma_tasklet_cb);

[Severity: High]
This is a pre-existing issue, but can an early interrupt permanently hang
the channel during probe?

In xgene_dma_probe(), xgene_dma_init_hw() enables the DMA engine and unmasks
interrupts before xgene_dma_init_async() initializes the bottom halves.

If a pending or spurious interrupt fires, the ISR attempts to schedule the
uninitialized BH, which fails. Because the BH never runs, enable_irq() is
never called, leaving the channel's IRQ permanently disabled.

[ ... ]

> @@ -1579,7 +1578,7 @@ static int xgene_dma_init_async(struct xgene_dma *pdma)
>  		if (ret) {
>  			for (j = 0; j < i; j++) {
>  				dma_async_device_unregister(&pdma->dma_dev[j]);
> -				tasklet_kill(&pdma->chan[j].tasklet);
> +				dma_chan_kill_bh(&pdma->chan[j].dma_chan);
>  			}

[Severity: High]
This is a pre-existing issue, but could this error path lead to a
use-after-free of the driver structures?

If xgene_dma_init_async() fails, it calls dma_chan_kill_bh() to clean up
initialized channels, but does not mask hardware interrupts.

An interrupt could fire immediately after, re-queuing the BH. The probe
function then calls xgene_dma_free_irqs(), returns an error, and devm
frees the pdma memory.

Could the global workqueue subsequently execute the re-scheduled BH,
dereferencing the freed chan structure?

[ ... ]

> @@ -1790,7 +1789,7 @@ static void xgene_dma_remove(struct platform_device *pdev)
>  
>  	for (i = 0; i < XGENE_DMA_MAX_CHANNEL; i++) {
>  		chan = &pdma->chan[i];
> -		tasklet_kill(&chan->tasklet);
> +		dma_chan_kill_bh(&chan->dma_chan);
>  		xgene_dma_delete_chan_rings(chan);
>  	}

[Severity: High]
This is a pre-existing issue, but is there a risk of accessing a freed IRQ
descriptor during this teardown sequence?

xgene_dma_remove() calls xgene_dma_free_irqs() which frees chan->rx_irq, and
then calls dma_chan_kill_bh().

If the bottom half was already queued, it executes after the IRQ is freed
and unconditionally calls enable_irq(chan->rx_irq), which violates the API
and could cause an oops in the IRQ subsystem.

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