Re: [PATCH] dmaengine: ste_dma40: fix autosuspend cleanup on probe failure

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

Pre-existing issues:
- [High] Unbalanced PM runtime state due to unconditional disable on probe failure.
- [High] IRQ resource leak and subsequent Use-After-Free on probe failure.
- [High] Incorrect order parameter passed to `__get_free_pages()` causes exponentially oversized allocations.
- [High] Calling `free_pages()` on a `devm_ioremap()` pointer causes a kernel panic.
- [High] Missing tasklet cleanup on probe failure leads to Use-After-Free.
- [High] Teardown asymmetry leaves active DMA engines exposed to users after backing resources are freed.
--

--- Patch [1]: [PATCH] dmaengine: ste_dma40: fix autosuspend cleanup on probe failure ---
commit 314875d0410790018090d6a5f3fa67bbac67585d
Author: Guangshuo Li <[email protected]>

dmaengine: ste_dma40: fix autosuspend cleanup on probe failure

d40_probe() calls pm_runtime_use_autosuspend(), but its error path does
not call the matching pm_runtime_dont_use_autosuspend() before
disabling runtime PM.

If the autosuspend delay is set to a negative value while autosuspend
is enabled, the runtime PM core increments usage_count to prevent
runtime suspend. Without calling pm_runtime_dont_use_autosuspend()
during cleanup, this reference is not dropped and usage_count remains
unbalanced.

Add the missing pm_runtime_dont_use_autosuspend() call to the probe
failure path before disabling runtime PM.

Fixes: 8d318a50b3d7 ("DMAENGINE: Support for ST-Ericssons DMA40 block v3")
Cc: [email protected]

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 0d9ffa3e26639..e2ad50e0fec48 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -3654,6 +3654,7 @@ static int __init d40_probe(struct platform_device *pdev)
>  		regulator_disable(base->lcpa_regulator);
>  		regulator_put(base->lcpa_regulator);
>  	}
> +	pm_runtime_dont_use_autosuspend(base->dev);
>  	pm_runtime_disable(base->dev);
>  

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
PM cleanup here, does this unconditional call to pm_runtime_disable() cause
an unbalanced runtime state if probe fails early?

If d40_probe() jumps to destroy_cache before pm_runtime_enable() is called
(e.g., during memory allocation or IRQ request), this will increment the
device's disable_depth from 1 to 2. If the device is probed again later,
the next pm_runtime_enable() will only decrement it to 1, leaving the
device permanently disabled.

[Severity: High]
This is a pre-existing issue, but looking at the surrounding error cleanup
path, is there an IRQ resource leak here?

The driver requests an unmanaged interrupt earlier:

    request_irq(base->irq, d40_handle_interrupt, 0, D40_NAME, base);

If a subsequent initialization step fails and we jump to destroy_cache,
free_irq() is omitted. Could this leave a dangling pointer registered as
the IRQ handler data, potentially causing a use-after-free if the hardware
fires an interrupt after devres frees the base structure?

[Severity: High]
This is a pre-existing issue, but does the cleanup in destroy_cache risk
calling free_pages() on an I/O mapped pointer?

When base->plat_data->use_esram_lcla is true, the base pointer is assigned
an I/O mapped address:

    base->lcla_pool.base = devm_ioremap(dev, res->start, resource_size(res));

If probe fails, destroy_cache evaluates if base_unaligned is NULL and
base->lcla_pool.base is true, leading to:

    free_pages((unsigned long)base->lcla_pool.base, base->lcla_pool.pages);

Passing an I/O memory address to free_pages() seems invalid.

[Severity: High]
This is a pre-existing issue, but is the pages parameter passed to
free_pages() and __get_free_pages() calculated correctly?

In d40_lcla_allocate(), the value is calculated as the raw number of pages:

    base->lcla_pool.pages = SZ_1K * base->num_phy_chans / PAGE_SIZE;

This raw count is then passed as the order parameter to __get_free_pages():

    __get_free_pages(GFP_KERNEL, base->lcla_pool.pages);

Would passing the raw count instead of an order (e.g., 8 instead of 3 for
32 channels) cause exponentially oversized allocations?

[Severity: High]
This is a pre-existing issue, but does the error path omit tasklet cleanup?

During channel initialization, tasklets are set up:

    tasklet_setup(&d40c->tasklet, dma_tasklet);

If probe fails afterwards, tasklet_kill() is not called in the cleanup
path. If an interrupt fires and schedules a tasklet before probe fails,
could the tasklet still execute after devres frees the channel structures?

[Severity: High]
This is a pre-existing issue, but is there a teardown asymmetry with the
DMA engine registration?

The DMA engine is exposed via a managed API:

    dmaenginem_async_device_register(&base->dma_slave);

If probe fails later (e.g., in of_dma_controller_register), the code
jumps to destroy_cache and manually destroys the lcla_pool, unmaps DMA
addresses, and disables regulators. However, the successfully registered
DMA engines remain active until devres unregisters them later.

Could concurrent clients acquire a channel and initiate transfers during
this window, accessing freed memory or operating with disabled regulators?

>   report_failure:

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