Re: [PATCH v3] dmaengine: qcom: gpi: Fix resource leaks as part of channel clean up

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

New issues:
- [Medium] Inconsistent software state after channel rollback causes duplicate shutdown commands and timeouts during client teardown.

Pre-existing issues:
- [Critical] gpi_free_chan_resources() unconditionally frees ev_ring, causing a double-free crash when an initialization failure occurs.
- [Critical] Hardware event ring is not deallocated in the gpi_ch_init() error path, causing a permanent DMA Use-After-Free.
- [High] Event processing tasklet is not killed before freeing ev_ring in the error path, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v3] dmaengine: qcom: gpi: Fix resource leaks as part of channel clean up ---
commit f50a0a6a0fc075e17ad27e65c7651c6aa5bb6a12
Author: Aniket Randive <[email protected]>

dmaengine: qcom: gpi: Fix resource leaks as part of channel clean up

This commit fixes the gpi_ch_init() and gpi_alloc_chan_resources() error
handling paths to properly iterate over the sibling channels, release
the ch_ring when channel initialization fails, and restore the event ring
state after freeing the ring.

Link: https://patch.msgid.link/[email protected]

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..c41bfac65ddf3 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c

[ ... ]

> @@ -1965,16 +1965,19 @@ static int gpi_ch_init(struct gchan *gchan)
>  error_start_chan:
>  	for (i = i - 1; i >= 0; i--) {
>  		gpi_stop_chan(&gpii->gchan[i]);
> -		gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
> +		gpi_send_cmd(gpii, &gpii->gchan[i], GPI_CH_CMD_RESET);
>  	}

[Severity: Medium]
Does this sequence leave the sibling channel's software state inconsistent?

When gpi_start_chan() fails for the second channel, this loop correctly
rolls back the sibling channel in hardware by sending the STOP, RESET, and
DE_ALLOC commands.

However, it appears to leave the sibling's pm_state as ACTIVE_STATE.
During client teardown, gpi_free_chan_resources() observes ACTIVE_STATE
and incorrectly re-issues these commands to the hardware:

gpi_free_chan_resources() {
    ...
    if (cur_state == ACTIVE_STATE) {
        ...
        gpi_send_cmd(gpii, gchan, GPI_CH_CMD_STOP);
    }
}

Because the hardware is already deallocated, do the commands time out
in wait_for_completion_timeout() and cause a blocking delay regression?

>  	i = 2;
>  error_alloc_chan:
>  	for (i = i - 1; i >= 0; i--)
> -		gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
> +		gpi_reset_chan(&gpii->gchan[i], GPI_CH_CMD_DE_ALLOC);
>  error_alloc_ev_ring:
>  	gpi_disable_interrupts(gpii);

[Severity: High]
This is a pre-existing issue, but does this error path need to kill the
event processing tasklet before proceeding to free the ring?

If an error occurs after gpi_alloc_ev_chan() succeeds, a hardware
interrupt could fire and schedule ev_task. A concurrently executing
tasklet might read from the freed ev_ring DMA memory before pm_state is
updated. 

Would calling tasklet_kill(&gpii->ev_task) before freeing the ring prevent
this regression where memory is used after being freed?

>  error_config_int:
>  	gpi_free_ring(&gpii->ev_ring, gpii);
> +	write_lock_irq(&gpii->pm_lock);
> +	gpii->pm_state = DISABLE_STATE;
> +	write_unlock_irq(&gpii->pm_lock);

[Severity: Critical]
This is a pre-existing issue, but is the hardware event ring deallocated
before we free its memory here?

If the error path frees ev_ring without sending the GPI_EV_CMD_DEALLOC
command, the hardware might continue writing events to the freed memory.

Additionally, does setting pm_state to DISABLE_STATE bypass the fallback
cleanup in gpi_free_chan_resources()?

gpi_free_chan_resources() {
    ...
    if (cur_state == ACTIVE_STATE)
        gpi_send_cmd(gpii, NULL, GPI_EV_CMD_DEALLOC);
}

By forcing the state to DISABLE_STATE, it seems the fallback DEALLOC
is prevented when the sibling channel is cleaned up, potentially making what
was previously a race window into a permanent regression of the event ring.

[Severity: Critical]
This is another pre-existing issue, but does freeing the event ring here
cause a regression by freeing the resource twice later?

When gpi_free_ring() is called, it zeroes the ring structure:

gpi_free_ring() {
    dma_free_coherent(...);
    memset(ring, 0, sizeof(*ring));
}

Later, when the client handles the probe failure and releases the
previously initialized sibling channel, gpi_free_chan_resources()
observes no remaining active channels and unconditionally executes:

    gpi_free_ring(&gpii->ev_ring, gpii);

Since the ring properties are now 0 and NULL, does this cause the dma layer
to attempt to free an invalid page?

>  exit_gpi_init:
>  	return ret;
>  }

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