Re: [PATCH v2 17/64] dmaengine: ste_dma40: convert per-channel tasklet to dma_chan BH

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

Pre-existing issues:
- [High] Missing `free_irq()` on `d40_probe` error paths leads to a leaked IRQ handler and a potential Use-After-Free of `base`.
- [High] Missing `device_synchronize` callback causes a race condition during channel release, potentially invoking callbacks in unloaded client modules.
- [High] Probe failure after exposing channels causes a Use-After-Free because devres forcefully frees memory while clients or scheduled BH work may be active.
--

--- Patch [17]: [PATCH v2 17/64] dmaengine: ste_dma40: convert per-channel tasklet to dma_chan BH ---
commit 7d6ec649e7f14a8affbdbde4ad9aa8498383284e
Author: Allen Pais <[email protected]>
Subject: dmaengine: ste_dma40: convert per-channel tasklet to dma_chan BH

This commit replaces the per-channel tasklet with the shared dma_chan BH
helper. The handler continues to run in softirq context while dmaengine
manages the scheduling and teardown mechanism.

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 0d9ffa3e26639..f279a093a81f6 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
[ ... ]
> @@ -1641,7 +1638,7 @@ static void dma_tasklet(struct tasklet_struct *t)
>  	d40c->pending_tx--;
>  
>  	if (d40c->pending_tx)
> -		tasklet_schedule(&d40c->tasklet);
> +		dma_chan_schedule_bh(&d40c->chan);
>  
>  	spin_unlock_irqrestore(&d40c->lock, flags);
>  

[Severity: High]
This is a pre-existing issue, but does this create a race condition during
channel release?

The driver doesn't seem to implement the device_synchronize callback, making
dmaengine_synchronize() a no-op. When a DMA client module unloads and
releases its channel, it relies on this synchronization to flush any pending
async work.

Could dma_release_channel() return and the client module unload while the
bh_work is still executing here? If the work is preempted right after
dropping the channel lock and before invoking the client callback:

	spin_unlock_irqrestore(&d40c->lock, flags);

	if (callback_active)
		dmaengine_desc_callback_invoke(&cb, NULL);

wouldn't it resume and attempt to execute the callback residing in the
now-freed module's .text segment?

Additionally, I noticed a couple of pre-existing issues in d40_probe():

[Severity: High]
This isn't a bug introduced by this patch, but does d40_probe() leak the
IRQ handler on error paths?

drivers/dma/ste_dma40.c:d40_probe() {
    ...
	ret = request_irq(base->irq, d40_handle_interrupt, 0, D40_NAME, base);
	if (ret) {
		d40_err(dev, "No IRQ defined\n");
		goto destroy_cache;
	}
    ...
	if (base->plat_data->use_esram_lcla) {

		base->lcpa_regulator = regulator_get(base->dev, "lcla_esram");
		if (IS_ERR(base->lcpa_regulator)) {
			d40_err(dev, "Failed to get lcpa_regulator\n");
			ret = PTR_ERR(base->lcpa_regulator);
			base->lcpa_regulator = NULL;
			goto destroy_cache;
		}
    ...
 destroy_cache:
	if (base->lcla_pool.dma_addr)
		dma_unmap_single(...)
    ...
 report_failure:
	d40_err(dev, "probe failed\n");
	return ret;
}

If d40_probe() fails after request_irq() (for example, in regulator_enable()
or of_dma_controller_register()), it branches to the destroy_cache label and
returns an error without freeing the registered IRQ.

Because base is automatically freed by the devres framework, if a spurious
or lingering interrupt fires afterwards, won't the still-registered
d40_handle_interrupt() execute and dereference the freed base pointer?


[Severity: High]
This isn't a bug introduced by this patch, but does a probe failure after
exposing channels cause a Use-After-Free?

drivers/dma/ste_dma40.c:d40_probe() {
    ...
	ret = d40_dmaengine_init(base, num_reserved_chans);
	if (ret)
		goto destroy_cache;
    ...
	ret = of_dma_controller_register(np, d40_xlate, NULL);
	if (ret) {
		dev_err(dev,
			"could not register of_dma_controller\n");
		goto destroy_cache;
	}
    ...
}

In d40_probe(), the driver exposes channels globally by calling
dmaenginem_async_device_register() midway through the function via
d40_dmaengine_init().

If a subsequent initialization step fails, like of_dma_controller_register(),
the function returns an error. The devres framework then automatically
unregisters the DMA device and frees the driver's memory.

In the window between registration and failure, could a concurrent client
driver request a channel and start a transfer, or an interrupt fire and
schedule the channel's BH work?

Since dma_async_device_unregister() doesn't wait for active clients to drop
references when called from devres, will the client or BH work access the
forcefully freed structures?

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