Re: [PATCH v3 33/34] dmaengine: qcom-gpi: defer callbacks via vchan

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

New issues:
- [High] The driver defers completion callbacks to a bottom-half (virt-dma BH) but fails to implement the `device_synchronize` callback. This breaks `dmaengine_terminate_sync()`, allowing client callbacks to execute concurrently with client resource freeing, leading to potential use-after-free bugs.
- [High] dmaengine_kill_bh() is called after gpi_free_ring(), allowing the virt-dma bottom half to execute client callbacks while the channel's hardware ring buffer is already freed and zeroed, leading to a divide-by-zero kernel panic.

Pre-existing issues:
- [High] If a client submits multiple descriptors before calling `dma_async_issue_pending()`, `gpi_issue_pending` drops all but the last descriptor.
--

--- Patch [33]: [PATCH v3 33/34] dmaengine: qcom-gpi: defer callbacks via vchan ---
commit b8f9701fb8c3295335854c36b7913f03c0929404
Author: Allen Pais <[email protected]>

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..2bc3a07ef0920 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
[ ... ]
> @@ -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);
> +	dmaengine_kill_bh(&gchan->vc.chan);

[Severity: High]
Does this sequence leave a race window where the bottom half tasklet
can access freed structures?

If dmaengine_kill_bh() is called after gpi_free_ring(), the virt-dma
bottom half could execute client callbacks while the channel's hardware
ring buffer is already freed and zeroed. If the client callback submits
a new transfer, gpi_prep_slave_sg() will invoke
gpi_ring_num_elements_avail(), which divides by ring->el_size. Since
the ring is zeroed, would this cause a divide-by-zero kernel panic?

Could dmaengine_kill_bh() be called before tearing down hardware
resources?


[Severity: High]
Since this patch defers completion callbacks to a virt-dma bottom half,
how does the driver synchronize these callbacks during channel teardown?

drivers/dma/qcom/gpi.c:gpi_probe() {
    ...
	gpi_dev->dma_device.device_free_chan_resources = gpi_free_chan_resources;
	gpi_dev->dma_device.device_tx_status = dma_cookie_status;
    ...
}

Without implementing the device_synchronize callback, does
dmaengine_terminate_sync() operate as a no-op regarding synchronization
of deferred callbacks? If a client terminates a channel and immediately
frees its buffers, could the still-running bottom-half callback access
freed memory?

Before this patch, gpi_terminate_all() acquired gpii->pm_lock, which
natively synchronized with the event tasklet. Is it necessary to assign
vchan_synchronize to gpi_dev->dma_device.device_synchronize to restore
this safety guarantee?


[Severity: High]
This isn't a bug introduced by this patch, but looking at batched
submissions in gpi_issue_pending():

drivers/dma/qcom/gpi.c:gpi_issue_pending() {
    ...
	/* move all submitted descriptors to issued list */
	spin_lock_irqsave(&gchan->vc.lock, flags);
	if (vchan_issue_pending(&gchan->vc))
		vd = list_last_entry(&gchan->vc.desc_issued,
				     struct virt_dma_desc, node);
    ...
}

If a client submits multiple descriptors before calling
dma_async_issue_pending(), does this logic drop all but the last
descriptor?

Extracting only the last descriptor and queueing its TREs to the
hardware ring would mean when the hardware completes this single
transfer, the driver's event handler pops the first descriptor from
the issued list. Could this mismatch between hardware transfers and
software completions leave the remaining descriptors permanently
uncompleted and corrupt data?

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