Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The driver invokes the DMA client callback while the transfer descriptor is still on the vchan `desc_issued` list, leading to a double-free and use-after-free list corruption if the channel is terminated concurrently. - [High] Missing dma_rmb() when reading event ring elements allows speculative reads of stale DMA memory. - [High] Missing dma_wmb() before writing to the channel doorbell register allows the device to fetch stale transfer ring elements (TREs). -- commit 69c40ab9cf4118765a807f48c1fc907c907c1667 Author: Praveen Talari <[email protected]> dmaengine: qcom: gpi: Convert dev_dbg() calls to tracepoints Replace the remaining dev_dbg() based debug logging in the GPI DMA driver with the qcom_gpi tracepoints, providing structured runtime visibility into GPI DMA behavior without requiring invasive debug patches. dev_err() calls are left untouched. > diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c > index a5055a6273af6..8e7d25a461475 100644 > --- a/drivers/dma/qcom/gpi.c > +++ b/drivers/dma/qcom/gpi.c [ ... ] > @@ -1043,6 +1039,8 @@ static void gpi_process_xfer_compl_event(struct gchan > *gchan, > smp_wmb(); > > chid = compl_event->chid; > + trace_gpi_ev_process(gpii->gpi_dev->dev, chid, XFER_COMPLETE_EV_TYPE, > + compl_event->code, compl_event->status, > compl_event->length); > if (compl_event->code == MSM_GPI_TCE_EOT && gpii->ieob_set) { > if (chid == GPI_RX_CHAN) > goto gpi_free_desc; > @@ -1054,11 +1052,10 @@ static void gpi_process_xfer_compl_event(struct gchan > *gchan, > dev_err(gpii->gpi_dev->dev, "Error in Transaction\n"); > result.result = DMA_TRANS_ABORTED; > } else { > - dev_dbg(gpii->gpi_dev->dev, "Transaction Success\n"); > result.result = DMA_TRANS_NOERROR; > } > result.residue = gpi_desc->len - compl_event->length; > - dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue); > + trace_gpi_xfer_result(gpii->gpi_dev->dev, chid, result.result, > result.residue); > > dma_cookie_complete(&vd->tx); > dmaengine_desc_get_callback_invoke(&vd->tx, &result); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] This isn't a bug introduced by this patch, but does invoking the DMA client callback here while the descriptor is still on the vchan desc_issued list create a use-after-free risk? If a client concurrently aborts the transfer (e.g., on a timeout) by calling dmaengine_terminate_all(), gpi_terminate_all() will extract all descriptors from the vchan lists (including this vd) and free them. When the tasklet resumes, it drops the lock, executes this callback, and then hits the gpi_free_desc path: gpi_free_desc: spin_lock_irqsave(&gchan->vc.lock, flags); list_del(&vd->node); spin_unlock_irqrestore(&gchan->vc.lock, flags); kfree(gpi_desc); Could this result in a double-free and list corruption? The identical vulnerable pattern also exists in gpi_process_imed_data_event(). [ ... ] > @@ -1090,11 +1087,8 @@ static void gpi_process_events(struct gpii *gpii) > chid = gpi_event->xfer_compl_event.chid; > type = gpi_event->xfer_compl_event.type; > > - dev_dbg(gpii->gpi_dev->dev, > - "Event: CHID:%u, type:%x %08x %08x %08x %08x\n", > - chid, type, gpi_event->gpi_ere.dword[0], > - gpi_event->gpi_ere.dword[1], > gpi_event->gpi_ere.dword[2], > - gpi_event->gpi_ere.dword[3]); > + trace_gpi_process_event(gpii->gpi_dev->dev, chid, type, > + gpi_event->gpi_ere.dword); [Severity: High] This is a pre-existing issue, but does gpi_process_events() need a dma_rmb() before accessing the event ring elements? The hardware event ring read pointer is read using gpi_read_reg() which wraps readl_relaxed(). Because readl_relaxed() doesn't provide memory ordering guarantees, could the CPU speculatively read the DMA memory for gpi_event before the register read completes, resulting in stale DMA memory being read? [ ... ] > @@ -1887,6 +1871,7 @@ static void gpi_issue_pending(struct dma_chan *chan) > } > > gpi_desc = to_gpi_desc(vd); > + trace_gpi_queue_xfer(gpii->gpi_dev->dev, gchan->chid, > gpi_desc->num_tre); > for (i = 0; i < gpi_desc->num_tre; i++) { > tre = &gpi_desc->tre[i]; > gpi_queue_xfer(gpii, gchan, tre, &wp); > } [Severity: High] This is a pre-existing issue, but is a dma_wmb() barrier needed here after copying the Transfer Ring Elements (TREs) and before writing to the channel doorbell register? Right after this loop, gpi_issue_pending() calls gpi_write_ch_db() which wraps writel_relaxed(). Without a dma_wmb() enforcing memory ordering, could the register write reach the device before the TRE memory writes are completely visible, causing the device to fetch stale descriptors? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260831-add-trace-support-gpio-v1-0-7b0fd0d0d...@oss.qualcomm.com?part=3
