[PATCH] udmabuf: Do not create malformed scatterlists

[PATCH] udmabuf: Do not create malformed scatterlists

[Jason Gunthorpe]

Using a sg_set_folio() loop for every 4K results in a malformed scatterlist
because sg_set_folio() has an issue with offsets > PAGE_SIZE and because
scatterlist expects the creator to build a list which consolidates any
physical contiguity.

sg_alloc_table_from_pages() creates a valid scatterlist directly from a
struct page array, so go back to that.

Remove the offsets allocation and just store an array of tail pages as it
did before the below commit. Everything wants that anyhow.

Fixes: 0c8b91ef5100 ("udmabuf: add back support for mapping hugetlb pages")
Reported-by: Julian Orth <ju.orth@gmail.com>
Closes: https://lore.kernel.org/all/20260308-scatterlist-v1-1-39c4566b0bba@gmail.com/
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/dma-buf/udmabuf.c | 49 +++++++++++----------------------------
 1 file changed, 13 insertions(+), 36 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 94b8ecb892bb17..5d687860445137 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -26,10 +26,10 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in megabytes. Default is
 
 struct udmabuf {
 	pgoff_t pagecount;
-	struct folio **folios;
+	struct page **pages;
 
 	/**
-	 * Unlike folios, pinned_folios is only used for unpin.
+	 * Unlike pages, pinned_folios is only used for unpin.
 	 * So, nr_pinned is not the same to pagecount, the pinned_folios
 	 * only set each folio which already pinned when udmabuf_create.
 	 * Note that, since a folio may be pinned multiple times, each folio
@@ -41,7 +41,6 @@ struct udmabuf {
 
 	struct sg_table *sg;
 	struct miscdevice *device;
-	pgoff_t *offsets;
 };
 
 static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
@@ -55,8 +54,7 @@ static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
 	if (pgoff >= ubuf->pagecount)
 		return VM_FAULT_SIGBUS;
 
-	pfn = folio_pfn(ubuf->folios[pgoff]);
-	pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
+	pfn = page_to_pfn(ubuf->pages[pgoff]);
 
 	ret = vmf_insert_pfn(vma, vmf->address, pfn);
 	if (ret & VM_FAULT_ERROR)
@@ -73,8 +71,7 @@ static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
 		if (WARN_ON(pgoff >= ubuf->pagecount))
 			break;
 
-		pfn = folio_pfn(ubuf->folios[pgoff]);
-		pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
+		pfn = page_to_pfn(ubuf->pages[pgoff]);
 
 		/**
 		 * If the below vmf_insert_pfn() fails, we do not return an
@@ -109,22 +106,11 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
 static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
 {
 	struct udmabuf *ubuf = buf->priv;
-	struct page **pages;
 	void *vaddr;
-	pgoff_t pg;
 
 	dma_resv_assert_held(buf->resv);
 
-	pages = kvmalloc_objs(*pages, ubuf->pagecount);
-	if (!pages)
-		return -ENOMEM;
-
-	for (pg = 0; pg < ubuf->pagecount; pg++)
-		pages[pg] = folio_page(ubuf->folios[pg],
-				       ubuf->offsets[pg] >> PAGE_SHIFT);
-
-	vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
-	kvfree(pages);
+	vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
 	if (!vaddr)
 		return -EINVAL;
 
@@ -146,22 +132,18 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
 {
 	struct udmabuf *ubuf = buf->priv;
 	struct sg_table *sg;
-	struct scatterlist *sgl;
-	unsigned int i = 0;
 	int ret;
 
 	sg = kzalloc_obj(*sg);
 	if (!sg)
 		return ERR_PTR(-ENOMEM);
 
-	ret = sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL);
+	ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount, 0,
+					ubuf->pagecount << PAGE_SHIFT,
+					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,
-			     ubuf->offsets[i]);
-
 	ret = dma_map_sgtable(dev, sg, direction, 0);
 	if (ret < 0)
 		goto err_map;
@@ -207,12 +189,8 @@ static void unpin_all_folios(struct udmabuf *ubuf)
 
 static __always_inline int init_udmabuf(struct udmabuf *ubuf, pgoff_t pgcnt)
 {
-	ubuf->folios = kvmalloc_objs(*ubuf->folios, pgcnt);
-	if (!ubuf->folios)
-		return -ENOMEM;
-
-	ubuf->offsets = kvzalloc_objs(*ubuf->offsets, pgcnt);
-	if (!ubuf->offsets)
+	ubuf->pages = kvmalloc_objs(*ubuf->pages, pgcnt);
+	if (!ubuf->pages)
 		return -ENOMEM;
 
 	ubuf->pinned_folios = kvmalloc_objs(*ubuf->pinned_folios, pgcnt);
@@ -225,8 +203,7 @@ static __always_inline int init_udmabuf(struct udmabuf *ubuf, pgoff_t pgcnt)
 static __always_inline void deinit_udmabuf(struct udmabuf *ubuf)
 {
 	unpin_all_folios(ubuf);
-	kvfree(ubuf->offsets);
-	kvfree(ubuf->folios);
+	kvfree(ubuf->pages);
 }
 
 static void release_udmabuf(struct dma_buf *buf)
@@ -344,8 +321,8 @@ static long udmabuf_pin_folios(struct udmabuf *ubuf, struct file *memfd,
 		ubuf->pinned_folios[nr_pinned++] = folios[cur_folio];
 
 		for (; subpgoff < fsize; subpgoff += PAGE_SIZE) {
-			ubuf->folios[upgcnt] = folios[cur_folio];
-			ubuf->offsets[upgcnt] = subpgoff;
+			ubuf->pages[upgcnt] = folio_page(folios[cur_folio],
+						subpgoff >> PAGE_SHIFT);
 			++upgcnt;
 
 			if (++cur_pgcnt >= pgcnt)

base-commit: 1f318b96cc84d7c2ab792fcc0bfd42a7ca890681
-- 
2.43.0

Claude review: udmabuf: Do not create malformed scatterlists

[Claude Code Review Bot]

Overall Series Review

Subject: udmabuf: Do not create malformed scatterlists
Author: Jason Gunthorpe <jgg@nvidia.com>
Patches: 1
Reviewed: 2026-03-14T06:43:42.305622

---

This is a single-patch fix from Jason Gunthorpe that addresses a real bug in the udmabuf driver where `sg_set_folio()` was used in a per-4K-page loop, producing malformed scatterlists. The root cause is twofold: (1) `sg_set_folio()` doesn't handle offsets larger than `PAGE_SIZE` correctly, and (2) the code failed to coalesce physically contiguous pages into single scatterlist entries, which the scatterlist API expects.

The fix is clean and straightforward: revert to storing an array of `struct page *` (tail pages from the folios) instead of `struct folio **` plus a parallel offsets array, and use `sg_alloc_table_from_pages()` which naturally builds a correct, coalesced scatterlist from a page array.

The patch simplifies the code significantly (-36, +13 lines), removes the `offsets` array entirely, and eliminates a temporary page-array allocation in `vmap_udmabuf()` since the pages are now stored directly. The approach is sound and the resulting code is cleaner than what it replaces.

**Verdict: Looks good. This is a well-motivated bugfix with a clean implementation.**

---
Generated by Claude Code Patch Reviewer

Claude review: udmabuf: Do not create malformed scatterlists

[Claude Code Review Bot]

Patch Review

**Correctness of the core fix:**

The key change in `get_sg_table()` replaces:
```c
ret = sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL);
...
for_each_sg(sg->sgl, sgl, ubuf->pagecount, i)
    sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE, ubuf->offsets[i]);
```
with:
```c
ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount, 0,
                                ubuf->pagecount << PAGE_SHIFT, GFP_KERNEL);
```

This is correct. `sg_alloc_table_from_pages()` will merge contiguous pages into single scatterlist entries, which is the expected behavior. The old code allocated one SG entry per 4K page and set each individually, which produced a valid-looking but semantically broken scatterlist (no coalescing, and the `sg_set_folio()` offset bug on large folios).

**Data structure change:**

Replacing `struct folio **folios` + `pgoff_t *offsets` with `struct page **pages` is appropriate. The page pointers are obtained via `folio_page()` in `udmabuf_pin_folios()`:
```c
ubuf->pages[upgcnt] = folio_page(folios[cur_folio],
                    subpgoff >> PAGE_SHIFT);
```

This correctly extracts the tail page at the right sub-page offset within the folio. The folio pin tracking remains via the separate `pinned_folios` array, which is unchanged — this is fine since `unpin_folio()` works on the head folio.

**vmap_udmabuf simplification:**

The old code allocated a temporary `pages` array and populated it from `folios[]`+`offsets[]` just to call `vm_map_ram()`. Now it passes `ubuf->pages` directly. This is a nice cleanup that removes an allocation and avoids a potential `-ENOMEM` failure path.

**Comment nit (very minor):**

The comment update at line 31-32 says:
```c
 * Unlike pages, pinned_folios is only used for unpin.
```
The grammar "Unlike pages" is slightly awkward — it could be read as "pages are used for unpin too" when the intent is "pages is used for more than just unpin; pinned_folios is only for unpin." This is a pre-existing comment pattern that was just mechanically updated from "folios" to "pages", so not really an issue introduced by this patch.

**No issues found.** The patch is a clean, correct fix for a real bug with a good Fixes tag and proper attribution.

---
Generated by Claude Code Patch Reviewer