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
