Re: [PATCH] lib/scatterlist: fix sg_page_count and sg_dma_page_count

[Jason Gunthorpe <jgg@ziepe.ca>]

On Sun, Mar 08, 2026 at 02:55:27PM +0100, Julian Orth wrote:
> A user reported memory corruption in the Jay wayland compositor [1]. The
> corruption started when archlinux enabled
> CONFIG_TRANSPARENT_HUGEPAGE_SHMEM_HUGE_WITHIN_SIZE in kernel 6.19.5.
> 
> The compositor uses udmabuf to upload memory from memfds to the GPU.
> When running an affected kernel, the following warnings are logged:
> 
>     a - addrs >= max_entries
>     WARNING: drivers/gpu/drm/drm_prime.c:1089 at drm_prime_sg_to_dma_addr_array+0x86/0xc0, CPU#31: jay/1864
>     [...]
>     Call Trace:
>      <TASK>
>      amdgpu_bo_move+0x188/0x800 [amdgpu 3b451640234948027c09e9b39e6520bc7e5471cf]
> 
> Disabling the use of huge pages at runtime via
> /sys/kernel/mm/transparent_hugepage/shmem_enabled fixes the issue.
> 
> udmabuf allocates a scatterlist with buffer_size/PAGE_SIZE entries. Each
> entry has a length of PAGE_SIZE. With huge pages disabled, it appears
> that sg->offset is always 0. With huge pages enabled, sg->offset is
> incremented by PAGE_SIZE until the end of the huge page.

This was broken by 0c8b91ef5100 ("udmabuf: add back support for
mapping hugetlb pages") which switched from a working
sg_alloc_table_from_pages() to a messed up sg_set_pages loop:

+       for_each_sg(sg->sgl, sgl, ubuf->pagecount, i)
+               sg_set_page(sgl, ubuf->pages[i], PAGE_SIZE, ubuf->offsets[i]);
[..]
+               ubuf->offsets[*pgbuf] = subpgoff << PAGE_SHIFT;

Which is just the wrong way to use the scatterlist API.

This was later changed to sg_set_folio() which I'm also suspecting has
a bug, it should be setting page_link to the proper tail page because
as you observe page_offset must fall within 0 to PAGE_SIZE-1 to make
the iterator work.

I think the whole design here in udmabuf makes very little sense. It
starts out with an actual list of folios then expands them to a per-4K
double array of folio/offset. This is nonsensical, if it wants to
build a way to direct index the mapping for mmap it should just build
itself a page * array like the code used to do and continue to use
sg_alloc_table_from_pages() which builds properly formed scatterlists.

This would save memory, use the APIs properly and build a correct and
optimized scatterlist to boot. It uses vmf_insert_pfn() and
vm_map_ram() anyhow so it doesn't even use a folio :\

Here, a few mins of AI shows what I think udmabuf should look like. If
you wish to persue this please add my signed-off-by and handle testing
it and getting it merged. I reviewed it enough to see it was showing
what I wanted.

Jason

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)