Re: [PATCH v2] dmaengine: qcom: gpi: correct gchan in error paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `GPI_EV_CMD_DEALLOC` command in the `gpi_ch_init()` error path allows the hardware to write to freed event ring memory.
- [High] The transfer ring memory (`gchan->ch_ring`) is permanently leaked on the error path of `gpi_alloc_chan_resources()`.
- [High] The `gpi_ch_init` error path fails to kill the pending `ev_task` tasklet and reset `gpii->pm_state`, enabling a Use-After-Free of `ev_ring`.
- [High] Failure to reset `gchan->pm_state` on the error path corrupts the channel synchronization state machine, leading to unintended behavior.
--

--- Patch [1]: [PATCH v2] dmaengine: qcom: gpi: correct gchan in error paths ---
commit a95016733426dc2a9c83a8d6ed2c5dccd118b6bb
Author: Brian Masney <bmasney@redhat.com>

dmaengine: qcom: gpi: correct gchan in error paths

This commit corrects the gchan pointer passed in the error paths of
gpi_ch_init(). It ensures that the correct channel from the gpii array
is reset and deallocated, rather than reusing the initial gchan argument.

Link: https://lore.kernel.org/r/20260625-qcom-gpi-err-fix-v1-1-5ca3f00fe2e3@redhat.com

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..3f390b5821abb 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -1965,12 +1965,12 @@ static int gpi_ch_init(struct gchan *gchan)

[Severity: High]
This is a pre-existing issue, but since we are fixing the error path here,
should gchan->pm_state be reset on failure?

At the beginning of gpi_ch_init(), we set:
    gchan->pm_state = CONFIG_STATE;

If initialization fails and we take these error paths, gchan->pm_state is never
reverted back to DISABLE_STATE. If a client later requests this channel, could
gpi_ch_init() incorrectly assume the channel is already configured and break the
synchronization logic?

>  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:

[Severity: High]
This is a pre-existing issue, but does this error path leave the event ring
memory accessible to the hardware?

gpi_alloc_ev_chan() configures the hardware with the event ring's physical
address via GPI_EV_CMD_ALLOCATE. Since a GPI_EV_CMD_DEALLOC command is not sent
here before freeing the ring, can the device continue performing DMA writes to
the now-freed memory?

[Severity: High]
This is another pre-existing issue, but does this error path also leave a
tasklet potentially running after freeing the event ring?

If an error occurs after interrupts are enabled, an interrupt could fire and
schedule gpii->ev_task. The call to gpi_disable_interrupts() uses
devm_free_irq() which waits for IRQ handlers, but it does not cancel pending
tasklets.

Since tasklet_kill() is not called and gpii->pm_state is not reverted, could a
scheduled tasklet access the freed gpii->ev_ring?

[Severity: High]
This is a pre-existing issue in the caller of gpi_ch_init(), but does a failure
here cause a memory leak of the transfer ring?

Looking at gpi_alloc_chan_resources():

drivers/dma/qcom/gpi.c:gpi_alloc_chan_resources() {
    ...
    ret = gpi_alloc_ring(&gchan->ch_ring, elements,
                         sizeof(struct gpi_tre), gpii);
    if (ret)
            return ret;

    ret = gpi_ch_init(gchan);

    mutex_unlock(&gpii->ctrl_lock);

    return ret;
}

If gpi_ch_init() fails, it returns the error code without freeing gchan->ch_ring.

Since the DMA engine core does not automatically invoke the
device_free_chan_resources callback when device_alloc_chan_resources fails,
is this DMA-coherent transfer ring memory permanently leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-qcom-gpi-err-fix-v2-1-49c9be055501@redhat.com?part=1