Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The scatterlist construction merges contiguous pages into a single segment without respecting the importing device's DMA maximum segment size constraints, which can also overflow the 32-bit SG length field for gigabyte-sized folios. --
commit 0cf8c9573968c851d2a1981909f322167f021ced 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 scatterlist entry per contiguous run of pages in a folio. It coalesces entries for large folios instead of emitting strictly one sg entry per PAGE_SIZE. > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 94b8ecb892bb1..f28dd3788adad 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c [ ... ] > +/* Count the contiguous folio runs in @ubuf, one sg entry per run. */ > +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++; [Severity: High] Does this count assume each contiguous run can always fit into a single scatterlist entry? If the importing device has a restrictive DMA maximum segment size, will this under-allocate the sg_table since large runs might need to be split across multiple entries? > + return nents; > +} > + > static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, > enum dma_data_direction direction) > { [ ... ] > - 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] Can run << PAGE_SHIFT exceed the importing device's maximum DMA segment size (dma_get_max_seg_size(dev))? If the hardware descriptors are limited (e.g. to 64KB), a large merged segment might get truncated during I/O by the importing driver. Also, if a system supports gigantic folios (e.g., 16GB HugeTLB), could run << PAGE_SHIFT exceed UINT_MAX? sg_set_folio() stores the length in the 32-bit sg->length field, which would silently truncate the value. > + sgl = sg_next(sgl); > + } > > ret = dma_map_sgtable(dev, sg, direction, 0); > if (ret < 0) -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
