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

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

[Aniket Randive]

The gpi_ch_init() error handling paths do not fully clean up resources
when channel initialization fails.

The unwind paths iterate over the original channel pointer instead of
the channels stored in gpii->gchan[], preventing previously initialized
sibling channels from being properly reset and deallocated.

In addition, gpi_alloc_chan_resources() allocates ch_ring before calling
gpi_ch_init(), but does not release it when initialization fails,
resulting in a memory leak.

The event ring cleanup path also leaves the PM state unchanged after
freeing the ring. As a result, subsequent cleanup may incorrectly assume
that the ring is still allocated, leading to a redundant EV_CMD_DEALLOC
command and an attempt to deallocate an already released ring.

Fix the unwind paths to operate on the correct channels, release
ch_ring when channel initialization fails, and restore the event ring
PM state after freeing the ring.

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---
Changes in v3:
- Updated the commit message as per Mukesh suggestion.
- Link to v2: https://patch.msgid.link/20260810-gpi_bug_fix-v2-1-60555394cbf9@oss.qualcomm.com

Changes in v2:
- Updated the commit message as per Mukesh suggestion.
---
 drivers/dma/qcom/gpi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..c41bfac65ddf 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);
 	}
 	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);
 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);
 exit_gpi_init:
 	return ret;
 }
@@ -2065,6 +2068,8 @@ static int gpi_alloc_chan_resources(struct dma_chan *chan)
 		goto xfer_alloc_err;
 
 	ret = gpi_ch_init(gchan);
+	if (ret)
+		gpi_free_ring(&gchan->ch_ring, gpii);
 
 	mutex_unlock(&gpii->ctrl_lock);
 

---
base-commit: 415606a7be939835db9b0d6b711887586646346d
change-id: 20260803-gpi_bug_fix-b0b80ef315b5

Best regards,
--  
Aniket Randive <aniket.randive@oss.qualcomm.com>

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

[sashiko-bot]

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 <aniket.randive@oss.qualcomm.com>

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/20260810-gpi_bug_fix-v2-1-60555394cbf9@oss.qualcomm.com

> 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/20260819-gpi_bug_fix-v3-1-4c97333e6f5b@oss.qualcomm.com?part=1