Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Scatterlist segment lengths generated from large folios exceed the importer device's maximum segment constraints, violating the DMA API. Pre-existing issues: - [High] Unsynchronized caching of `ubuf->sg` in `begin_cpu_udmabuf()` causes data races and memory leaks. - [High] Unmapping an sg_table with a hardcoded DMA_BIDIRECTIONAL direction mismatches the original mapped direction. -- commit 53ed4a59d4fc35b8f3cc16427a92f6e1ca44ddb7 Author: Bobby Eshleman <[email protected]> udmabuf: emit one sg entry per pinned folio This commit updates get_sg_table() to walk folios and emit one sg entry per folio, effectively coalescing contiguous runs into large scatterlist entries. This helps importers that expect dmabuf sg entries to be size and length aligned. > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 94b8ecb892bb1..9b751dd98b120 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c [ ... ] > +static unsigned int udmabuf_sg_nents(struct udmabuf *ubuf) > +{ > + unsigned int nents = 0; > + pgoff_t i; > + > + for (i = 0; i < ubuf->pagecount; i += udmabuf_folio_nr_pages(ubuf, i)) > + nents++; > + return nents; > +} > + > static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, > enum dma_data_direction direction) > { > struct udmabuf *ubuf = buf->priv; > - struct sg_table *sg; > struct scatterlist *sgl; > - unsigned int i = 0; > + struct sg_table *sg; > + pgoff_t i, run; > + unsigned int nents; > int ret; > > + nents = udmabuf_sg_nents(ubuf); > + > sg = kzalloc_obj(*sg); > if (!sg) > return ERR_PTR(-ENOMEM); > > - ret = sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL); > + ret = sg_alloc_table(sg, nents, GFP_KERNEL); > if (ret < 0) > goto err_alloc; > > - for_each_sg(sg->sgl, sgl, ubuf->pagecount, i) > - sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE, > + sgl = sg->sgl; > + for (i = 0; i < ubuf->pagecount; i += run) { > + run = udmabuf_folio_nr_pages(ubuf, i); > + sg_set_folio(sgl, ubuf->folios[i], run << PAGE_SHIFT, > ubuf->offsets[i]); [Severity: High] Does this code assign unconstrained lengths to scatterlist segments without respecting the importer device's max_seg_size? If the coalesced run of folios exceeds the importer device's maximum segment size (checked via dma_get_max_seg_size()), the DMA API mapping might fail or silently truncate, potentially causing hardware-level data corruption. Since dma_map_sgtable() cannot split overly large segments, should the scatterlist be correctly sized prior to mapping by splitting runs that exceed the device constraints? This would likely also require adjusting the entry count calculated in udmabuf_sg_nents() above. > + sgl = sg_next(sgl); > + } > > ret = dma_map_sgtable(dev, sg, direction, 0); > if (ret < 0) [Severity: High] This is a pre-existing issue, but I noticed a potential race condition in begin_cpu_udmabuf() in drivers/dma-buf/udmabuf.c. If the DMA_BUF_IOCTL_SYNC ioctl invokes begin_cpu_udmabuf() without acquiring the dmabuf->resv lock: begin_cpu_udmabuf() { ... if (!ubuf->sg) { ubuf->sg = get_sg_table(dev, buf, direction); ... } Can concurrent executions by multiple threads on the same file descriptor simultaneously allocate new sg_table instances and overwrite ubuf->sg? Could this permanently leak the unreferenced sg_tables and their associated DMA mappings? [Severity: High] This is also a pre-existing issue, but does release_udmabuf() unmap the sg_table using a hardcoded direction that mismatches the original mapped direction? release_udmabuf() { ... if (ubuf->sg) put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL); ... } Since begin_cpu_udmabuf() allocates and maps the sg_table using the DMA direction explicitly requested by userspace (e.g., DMA_FROM_DEVICE), unmapping it unconditionally with DMA_BIDIRECTIONAL violates the DMA API. Could this corrupt IOMMU state or cause kernel warnings on platforms with strict IOMMUs or bounce buffers? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
