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

[sashiko-bot@kernel.org]

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

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/20260612-tcpdm-large-niovs-v3-0-a3b693e76fcb@meta.com?part=2