Re: [PATCH] drm/ttm: clamp the prefault window to the buffer object

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 <[email protected]>

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: [email protected]
Signed-off-by: Baul Lee <[email protected]>

> 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/[email protected]?part=1