Re: [PATCH v2 04/64] dmaengine: at_xdmac: move irq bottom half to dma_chan BH

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but an initialization race condition exists: `request_irq()` is called before hardware clocks are enabled and DMA channels are initialized, which can cause a kernel panic or infinite loop.
- [High] This problem wasn't introduced by this patch, but there is a resource leak and Use-After-Free due to a bypassed cleanup loop in `at_xdmac_remove()`.
- [High] This problem wasn't introduced by this patch, but the teardown order is flawed: Hardware clocks and PM runtime are disabled before freeing the IRQ and stopping BH workers.
- [High] This problem wasn't introduced by this patch, but out-of-bounds memory accesses and list corruption occur due to an unchecked `list_first_entry()` in the BH worker.
- [High] This problem wasn't introduced by this patch, but there is a sleeping in atomic bug: The error handler calls `pm_runtime_resume_and_get()` while holding an IRQ-disabled spinlock.
--

--- Patch [4]: [PATCH v2 04/64] dmaengine: at_xdmac: move irq bottom half to dma_chan BH ---
commit 91d37fd9a6c41425b793e9ed1792794d52b35014
Author: Allen Pais <[email protected]>

dmaengine: at_xdmac: move irq bottom half 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/at_xdmac.c b/drivers/dma/at_xdmac.c
> index 901971e8bae69..aa918043e37ec 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c

[ ... ]

> @@ -1759,9 +1758,9 @@ static void at_xdmac_handle_error(struct at_xdmac_chan *atchan)
>  	/* Then continue with usual descriptor management */
>  }
>  
> -static void at_xdmac_tasklet(struct tasklet_struct *t)
> +static void at_xdmac_tasklet(struct dma_chan *chan)
>  {
> -	struct at_xdmac_chan	*atchan = from_tasklet(atchan, t, tasklet);
> +	struct at_xdmac_chan	*atchan = to_at_xdmac_chan(chan);
>  	struct at_xdmac		*atxdmac = to_at_xdmac(atchan->chan.device);
>  	struct at_xdmac_desc	*desc;

[Severity: High]
This is a pre-existing issue, but does this function lack a list empty check?
Later in at_xdmac_tasklet(), the code does:

desc = list_first_entry(&atchan->xfers_list, struct at_xdmac_desc,
                        xfer_node);
if (!desc->active_xfer) { ... }

If at_xdmac_device_terminate_all() runs concurrently and empties
xfers_list, resuming the BH worker might resolve an invalid pointer for
the empty list, read out-of-bounds for desc->active_xfer, and corrupt
the list.

[Severity: High]
This is also a pre-existing issue, but can this path sleep while atomic?
In at_xdmac_tasklet(), an IRQ-disabled spinlock is acquired:

spin_lock_irq(&atchan->lock);
...
if (atchan->irq_status & error_mask)
        at_xdmac_handle_error(atchan);

Inside at_xdmac_handle_error(), it calls pm_runtime_resume_and_get(),
which can sleep since the device isn't marked as pm_runtime_irq_safe().
Could this lead to a scheduling while atomic bug if the device's
runtime status is not RPM_ACTIVE?

[ ... ]

> @@ -2317,7 +2316,7 @@ static int at_xdmac_probe(struct platform_device *pdev)
>  		return PTR_ERR(atxdmac->clk);
>  	}
>  
> -	/* Do not use dev res to prevent races with tasklet */
> +	/* Do not use devm resources to prevent races with the BH worker */
>  	ret = request_irq(atxdmac->irq, at_xdmac_interrupt, 0, "at_xdmac", atxdmac);

[Severity: High]
This isn't a bug introduced by this patch, but an initialization race
condition appears to exist here. Is it safe to call request_irq() before
the hardware clocks are enabled via clk_prepare_enable() and before
dma_async_device_register() initializes the channel count?

If an early or pending interrupt fires immediately after request_irq(),
the handler at_xdmac_interrupt() will execute while the controller's
clock is gated, which could trigger a bus fault.

Even if clocks were enabled, atxdmac->dma.chancnt is still 0 at this point.
The handler would skip the channel check loop, fail to clear the interrupt,
and spin infinitely in its do-while(pending) loop.

[ ... ]

> @@ -2458,7 +2457,7 @@ static void at_xdmac_remove(struct platform_device *pdev)
>  	for (i = 0; i < atxdmac->dma.chancnt; i++) {
>  		struct at_xdmac_chan *atchan = &atxdmac->chan[i];
>  
> -		tasklet_kill(&atchan->tasklet);
> +		dma_chan_kill_bh(&atchan->chan);
>  		at_xdmac_free_chan_resources(&atchan->chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does the teardown order in
at_xdmac_remove() and the probe error path risk a bus fault?

In at_xdmac_remove(), clk_disable_unprepare() and pm_runtime_disable() are
called before free_irq(). If a pending interrupt fires before free_irq()
completes, the interrupt handler will attempt to read from the clock-gated
device registers, which could trigger a synchronous external abort.

[Severity: High]
This is another pre-existing issue, but will this cleanup loop ever execute?

Earlier in at_xdmac_remove(), dma_async_device_unregister() is called,
which iterates over all registered channels and decrements device->chancnt
to 0.

When this loop attempts to use atxdmac->dma.chancnt as its limit, it is
completely bypassed, meaning dma_chan_kill_bh() and
at_xdmac_free_chan_resources() are never called, potentially causing a
resource leak and use-after-free for pending BH workers.

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