Re: [PATCH v2 63/64] dmaengine: qcom-gpi: defer callbacks via vchan

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect teardown sequence in gpi_free_chan_resources(): dma_chan_kill_bh() is called after gpi_free_ring(), exposing a freed ring buffer to concurrent client callbacks, leading to a divide-by-zero crash.
- [High] The driver defers callbacks to a virt-dma BH but fails to implement the device_synchronize callback, breaking dmaengine_terminate_sync().
--

--- Patch [63]: [PATCH v2 63/64] dmaengine: qcom-gpi: defer callbacks via vchan ---
commit e55f43a67f69dbf222b5c7eba067eedde92ac98b
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: qcom-gpi: defer callbacks via vchan

Complete descriptors through virt-dma instead of invoking callbacks
directly from event processing. This preserves the transfer result for
the shared vchan BH, which invokes the callback and frees the descriptor;
drain that BH before releasing channel resources.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..4cf68cf8eb2e5 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
[ ... ]
> @@ -1060,8 +1064,12 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
>  	result.residue = gpi_desc->len - compl_event->length;
>  	dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue);
>  
> -	dma_cookie_complete(&vd->tx);
> -	dmaengine_desc_get_callback_invoke(&vd->tx, &result);
> +	spin_lock_irqsave(&gchan->vc.lock, flags);
> +	list_del(&vd->node);
> +	vd->tx_result = result;
> +	vchan_cookie_complete(vd);

[Severity: High]
Since this patch defers callbacks to the virt-dma bottom-half, does the driver
now need to implement the device_synchronize callback?

When a client driver calls dmaengine_terminate_sync() to ensure no callbacks
are running before freeing its DMA buffers, the absence of device_synchronize
causes it to fall back to dmaengine_terminate_all(), which merely stops the
hardware.

Could the newly introduced deferred BH execute a completion callback after
dmaengine_terminate_sync() has already returned, potentially leading to a
use-after-free in the client driver?

Should gpi_probe() be updated to set gpi_dev->dma_device.device_synchronize
to a function that wraps vchan_synchronize()?

> +	spin_unlock_irqrestore(&gchan->vc.lock, flags);
> +	return;
>  
>  gpi_free_desc:
>  	spin_lock_irqsave(&gchan->vc.lock, flags);
[ ... ]
> @@ -2009,6 +2017,7 @@ static void gpi_free_chan_resources(struct dma_chan *chan)
>  
>  	/* free all allocated memory */
>  	gpi_free_ring(&gchan->ch_ring, gpii);
> +	dma_chan_kill_bh(&gchan->vc.chan);

[Severity: High]
Does this cleanup sequence introduce a race condition that could result in a
divide-by-zero crash?

Because dma_chan_kill_bh() is called after gpi_free_ring(), the bottom-half
is still permitted to run after the channel's ring has been freed and
zero-initialized.

If a deferred callback executes in the BH and calls dmaengine_prep_slave_sg()
to queue the next transfer, it will eventually call gpi_prep_slave_sg(). That
function calls gpi_ring_num_elements_avail(), which blindly accesses the now
zeroed ch_ring and divides by ring->el_size.

Could dma_chan_kill_bh() be moved before gpi_free_ring() to ensure the BH is
drained before tearing down the hardware ring resources?

>  	vchan_free_chan_resources(&gchan->vc);
>  	kfree(gchan->config);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=63