Re: [PATCH net-next 2/4] udmabuf: emit one sg entry per pinned folio

[sashiko-bot@kernel.org]

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 <bobbyeshleman@meta.com>

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/20260603-tcpdm-large-niovs-v1-0-f37a4ac6726c@meta.com?part=2