Re: [PATCH v5 16/36] mm/khugepaged: skip private node folios when trying to collapse.

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The replacement of `is_zone_device_page()` with `page_is_private_managed()` inside a `WARN_ON_ONCE` in `try_collapse_pte_mapped_thp()` triggers spurious kernel warnings and aborts PMD restoration for valid memory on private NUMA nodes.

Pre-existing issues:
- [Critical] Functions structurally modifying page tables under `mmap_write_lock()` fail to call `vma_start_write()`, leading to critical Use-After-Free race conditions with concurrent per-VMA lock readers (like MADV_DONTNEED).
--

--- Patch [16]: [PATCH v5 16/36] mm/khugepaged: skip private node folios when trying to collapse. ---
commit c62053ea9e5edc37182a3d1e8367dfb0ed6fbf08
Author: Gregory Price <[email protected]>

mm/khugepaged: skip private node folios when trying to collapse.

A collapse operation causes new THP allocation to occur, and may
migrate memory from one node to another. Handle this the same as
zone_device for now (disallow collapse).

> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 89ce6bcbc376b..fb4378cc17b10 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c

[ ... ]

> @@ -1944,7 +1944,7 @@ static enum scan_result try_collapse_pte_mapped_thp(struct mm_struct *mm, unsign
>  		}
>  
>  		page = vm_normal_page(vma, addr, ptent);
> -		if (WARN_ON_ONCE(page && is_zone_device_page(page)))
> +		if (WARN_ON_ONCE(page && page_is_private_managed(page)))

[Severity: High]
Does replacing is_zone_device_page() with page_is_private_managed() inside
this WARN_ON_ONCE trigger spurious warnings and abort PMD restoration?

When a user maps a file-backed THP on a private NUMA node, temporarily
splits the PMD into PTEs, and then triggers a PMD collapse, we reach
try_collapse_pte_mapped_thp(). Because page_is_private_managed() can be
true for valid memory on private nodes, won't this trigger the warning
and unintentionally set page to NULL?

If panic_on_warn is enabled, this could be used by an unprivileged user
to reliably crash the system.

[Severity: Critical]
This is a pre-existing issue, but does try_collapse_pte_mapped_thp() need
to ensure vma_start_write() is called to prevent a use-after-free with
concurrent per-VMA lock readers?

When a thread registers a uprobe via register_for_each_vma(), it acquires
the mmap_write_lock() and eventually calls into this function:

kernel/events/uprobes.c:register_for_each_vma() {
    ...
    mmap_write_lock(mm);
    if (check_stable_address_space(mm))
        goto unlock;
    vma = find_vma(mm, info->vaddr);
    ...
}

try_collapse_pte_mapped_thp() then clears PMDs and frees PTE pages under
the assumption of exclusive access:

mm/khugepaged.c:try_collapse_pte_mapped_thp() {
    ...
    result = find_pmd_or_thp_or_none(mm, haddr, &pmd);
    ...
    start_pte = pte_offset_map_lock(mm, pmd, haddr, &ptl);
    ...
}

However, if another thread concurrently executes a VMA-locked operation
like MADV_DONTNEED, it only holds the VMA read lock. Because
vma_start_write() was never called to drain these concurrent readers, the
two threads can race to free or access the same PTE page.

>  			page = NULL;
>  		/*
>  		 * Note that uprobe, debugger, or MAP_PRIVATE may change the

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16