* Re: [PATCH] drm/ttm: clamp the prefault window to the buffer object
2026-08-06 3:43 [PATCH] drm/ttm: clamp the prefault window to the buffer object Baul Lee
@ 2026-08-06 3:54 ` sashiko-bot
2026-08-06 4:04 ` Matthew Brost
2026-08-06 11:38 ` Christian König
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-08-06 3:54 UTC (permalink / raw)
To: Baul Lee; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The error handling in the speculative prefault loop is inverted, hiding errors on the primary faulting page and propagating errors on prefaulted pages.
- [High] Unbounded kernel memory leak via repeated dummy page allocations when the DRM device is unplugged.
--
--- Patch [1]: [PATCH] drm/ttm: clamp the prefault window to the buffer object ---
commit 9b271449ffc1a09950a3de8a10df5f54125914a6
Author: Baul Lee <baul.lee@xbow.com>
drm/ttm: clamp the prefault window to the buffer object
ttm_bo_vm_fault_reserved() derives two page indices from the caller's
mmap(2) arguments and bounds only one of them:
[ ... ]
Fixes: ba4e7d973dd0 ("drm: Add the TTM GPU memory manager subsystem.")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c456..14ebf6ee3c47e 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -212,6 +212,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
> page_last = vma_pages(vma) + vma->vm_pgoff -
> drm_vma_node_start(&bo->base.vma_node);
> + page_last = min_t(unsigned long, page_last, PFN_UP(bo->base.size));
>
> if (unlikely(page_offset >= PFN_UP(bo->base.size)))
> return VM_FAULT_SIGBUS;
[ ... ]
> ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
>
> /* Never error on prefaulted PTEs */
> if (unlikely((ret & VM_FAULT_ERROR))) {
> if (i == 0)
> return VM_FAULT_NOPAGE;
> else
> break;
> }
[Severity: High]
This is a pre-existing issue, but does this error handling logic invert the
intended behavior? The comment states "Never error on prefaulted PTEs", but
if vmf_insert_pfn_prot() returns an error for the primary faulting page
(i == 0), it returns VM_FAULT_NOPAGE. This signals the MM core that the
fault was handled without a page, which can cause the CPU to retry the
instruction and immediately fault again, potentially leading to a local
soft lockup.
Conversely, if an error occurs on a prefaulted page (i > 0), it breaks the
loop and returns the error code, which might inappropriately force a
SIGBUS or SIGKILL on the application even though the primary fault succeeded.
Could this logic be backwards?
[ ... ]
> static vm_fault_t ttm_bo_vm_dummy_page(struct vm_fault *vmf, pgprot_t prot)
> {
[ ... ]
> /* Allocate new dummy page to map all the VA range in this VMA to it*/
> page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> if (!page)
> return VM_FAULT_OOM;
>
> /* Set the page to be freed using drmm release action */
> if (drmm_add_action_or_reset(bo->base.dev, ttm_bo_release_dummy_page,
> page))
[Severity: High]
This is a pre-existing issue, but does this code cause an unbounded kernel
memory leak when the DRM device is unplugged?
When a device is hot-unplugged, the fault handler falls back to this
function, which allocates a new dummy page and attaches it to the
drm_device lifecycle via drmm_add_action_or_reset().
If userspace holds the device file descriptor open (keeping the drm_device
alive) and repeatedly calls madvise(MADV_DONTNEED) followed by a memory
read on the mapping, each read triggers a new page fault. This could allocate
a new page and a new drmm action on every fault, which are never freed
until the device is completely destroyed. Could this lead to an
Out-Of-Memory (OOM) condition?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260806034356.43681-1-baul.lee@xbow.com?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/ttm: clamp the prefault window to the buffer object
2026-08-06 3:43 [PATCH] drm/ttm: clamp the prefault window to the buffer object Baul Lee
2026-08-06 3:54 ` sashiko-bot
@ 2026-08-06 4:04 ` Matthew Brost
2026-08-06 11:38 ` Christian König
2 siblings, 0 replies; 4+ messages in thread
From: Matthew Brost @ 2026-08-06 4:04 UTC (permalink / raw)
To: Baul Lee
Cc: christian.koenig, ray.huang, maarten.lankhorst, mripard,
tzimmermann, airlied, simona, matthew.auld, dri-devel,
linux-kernel, federico.kirschbaum, stable
On Thu, Aug 06, 2026 at 12:43:56PM +0900, Baul Lee wrote:
> ttm_bo_vm_fault_reserved() derives two page indices from the caller's
> mmap(2) arguments and bounds only one of them:
>
> page_offset = ((address - vma->vm_start) >> PAGE_SHIFT) +
> vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
> page_last = vma_pages(vma) + vma->vm_pgoff -
> drm_vma_node_start(&bo->base.vma_node);
>
> if (unlikely(page_offset >= PFN_UP(bo->base.size)))
> return VM_FAULT_SIGBUS;
>
> bo->base.size appears once in the function, bounding page_offset on
> entry. page_last comes straight from vma_pages(vma) and is the loop
> terminator:
>
> if (unlikely(++page_offset >= page_last))
> break;
>
> so the object size never bounds it. For an object of N pages, a fault
> on the last in-object page passes the entry test with page_offset
> N - 1, and the prefault loop then walks N..N+14, reading
> ttm->pages[page_offset] or
> ttm_bo_io_mem_pfn(bo, page_offset) and installing each frame with
> vmf_insert_pfn_prot().
>
> page_last exceeds the object whenever the VMA is longer than it.
> drm_gem_mmap_obj() rejects that on the DRM node, but the fbdev path
> reaches the object function through drm_gem_prime_mmap(), which does
> not. It is also exceeded by a mapping no longer than the object taken
> at a nonzero file offset, so the handler needs its own bound.
>
> With a 128-page object mapped 192 pages long, one read fault at index
> N - 1 leaves the fifteen frames after the object readable through the
> mapping; on a fresh mapping, reading index N without first faulting
> N - 1 is SIGBUS. For a system-memory placement the page array is
> over-read as well:
>
> BUG: KASAN: slab-out-of-bounds in ttm_bo_vm_fault_reserved+0x248/0x57c
> Read of size 8 at addr ffff0000078aac00 by task e1/219
> __asan_load8+0x84/0xb0
> ttm_bo_vm_fault_reserved+0x248/0x57c
> ttm_bo_vm_fault+0xe4/0x140
> __do_fault+0x6c/0x2f0
>
> Clamp page_last to the object.
>
> Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>
You are going to want an Assisted-by tag here as XBOW is an AI tool?
>
> Fixes: ba4e7d973dd0 ("drm: Add the TTM GPU memory manager subsystem.")
This won't apply to ba4e7d973dd0. More below.
> Cc: stable@vger.kernel.org
> Signed-off-by: Baul Lee <baul.lee@xbow.com>
> ---
> drivers/gpu/drm/ttm/ttm_bo_vm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c45..14ebf6ee3c47 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -212,6 +212,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
> page_last = vma_pages(vma) + vma->vm_pgoff -
> drm_vma_node_start(&bo->base.vma_node);
> + page_last = min_t(unsigned long, page_last, PFN_UP(bo->base.size));
This looks correct but maybe to make backporting easier all the way to
ba4e7d973dd0, we do this instead...
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index a80510489c45..3529371a37d5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -274,7 +274,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
}
address += PAGE_SIZE;
- if (unlikely(++page_offset >= page_last))
+ if (unlikely(++page_offset >= page_last ||
+ page_offset >= PFN_UP(bo->base.size)))
break;
}
return ret;
The above code git blame show this line was last modified in
ba4e7d973dd0.
Then in non-fixes patch, do it like you have it here.
Matt
>
> if (unlikely(page_offset >= PFN_UP(bo->base.size)))
> return VM_FAULT_SIGBUS;
> --
> 2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/ttm: clamp the prefault window to the buffer object
2026-08-06 3:43 [PATCH] drm/ttm: clamp the prefault window to the buffer object Baul Lee
2026-08-06 3:54 ` sashiko-bot
2026-08-06 4:04 ` Matthew Brost
@ 2026-08-06 11:38 ` Christian König
2 siblings, 0 replies; 4+ messages in thread
From: Christian König @ 2026-08-06 11:38 UTC (permalink / raw)
To: Baul Lee, ray.huang, maarten.lankhorst, mripard, tzimmermann,
airlied, simona
Cc: matthew.auld, matthew.brost, dri-devel, linux-kernel,
federico.kirschbaum, stable
On 8/6/26 05:43, Baul Lee wrote:
> ttm_bo_vm_fault_reserved() derives two page indices from the caller's
> mmap(2) arguments and bounds only one of them:
>
> page_offset = ((address - vma->vm_start) >> PAGE_SHIFT) +
> vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
> page_last = vma_pages(vma) + vma->vm_pgoff -
> drm_vma_node_start(&bo->base.vma_node);
>
> if (unlikely(page_offset >= PFN_UP(bo->base.size)))
> return VM_FAULT_SIGBUS;
>
> bo->base.size appears once in the function, bounding page_offset on
> entry. page_last comes straight from vma_pages(vma) and is the loop
> terminator:
>
> if (unlikely(++page_offset >= page_last))
> break;
>
> so the object size never bounds it. For an object of N pages, a fault
> on the last in-object page passes the entry test with page_offset
> N - 1, and the prefault loop then walks N..N+14, reading
> ttm->pages[page_offset] or
> ttm_bo_io_mem_pfn(bo, page_offset) and installing each frame with
> vmf_insert_pfn_prot().
>
> page_last exceeds the object whenever the VMA is longer than it.
> drm_gem_mmap_obj() rejects that on the DRM node, but the fbdev path
> reaches the object function through drm_gem_prime_mmap(), which does
> not. It is also exceeded by a mapping no longer than the object taken
> at a nonzero file offset, so the handler needs its own bound.
>
> With a 128-page object mapped 192 pages long,
And exactly that sentence explains why this whole patch is superfluous.
A 128 page object absolutely *can't* be mapped into 192 pages VMA!
If we would allow that the driver side could crash extremely badly long before we even get here.
Do you have any reproducer which exercises this?
Regards,
Christian.
> one read fault at index
> N - 1 leaves the fifteen frames after the object readable through the
> mapping; on a fresh mapping, reading index N without first faulting
> N - 1 is SIGBUS. For a system-memory placement the page array is
> over-read as well:
>
> BUG: KASAN: slab-out-of-bounds in ttm_bo_vm_fault_reserved+0x248/0x57c
> Read of size 8 at addr ffff0000078aac00 by task e1/219
> __asan_load8+0x84/0xb0
> ttm_bo_vm_fault_reserved+0x248/0x57c
> ttm_bo_vm_fault+0xe4/0x140
> __do_fault+0x6c/0x2f0
>
> Clamp page_last to the object.
>
> Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>
>
> Fixes: ba4e7d973dd0 ("drm: Add the TTM GPU memory manager subsystem.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Baul Lee <baul.lee@xbow.com>
> ---
> drivers/gpu/drm/ttm/ttm_bo_vm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c45..14ebf6ee3c47 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -212,6 +212,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
> page_last = vma_pages(vma) + vma->vm_pgoff -
> drm_vma_node_start(&bo->base.vma_node);
> + page_last = min_t(unsigned long, page_last, PFN_UP(bo->base.size));
>
> if (unlikely(page_offset >= PFN_UP(bo->base.size)))
> return VM_FAULT_SIGBUS;
> --
> 2.50.1 (Apple Git-155)
^ permalink raw reply [flat|nested] 4+ messages in thread