Re: [PATCH 2/2] mm/khugepaged: check for empty PTE tables before retracting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm/khugepaged: race between retract_page_tables() and
  copy_page_range()
- [Medium] mm/khugepaged: retract_page_tables() breaks uffd RWP on file
  and shmem VMAs
--

commit 15ea4d4b6e2586260561871f9acd4c3bff3278ec
Author: David Hildenbrand (Arm) <david@kernel.org>

mm/khugepaged: check for empty PTE tables before retracting

This commit replaces the VMA flag checks in retract_page_tables() with a
scan under the PTE lock to verify that the PTE table is empty before
retracting it.

> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 2a5ee8fc42335..f39ea3d076554 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
[ ... ]
> @@ -2081,48 +2082,6 @@ void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
>  	try_collapse_pte_mapped_thp(mm, addr, install_pmd);
>  }
>  
> -/* Can we retract page tables for this file-backed VMA? */
> -static bool file_backed_vma_is_retractable(struct vm_area_struct *vma)
> -{
> -	/*
> -	 * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
> -	 * got written to. These VMAs are likely not worth removing
> -	 * page tables from, as PMD-mapping is likely to be split later.
> -	 *
> -	 * Can't use vma_has_anon_rmap() here as the VMA may be stabilised
> -	 * by the file rmap lock.
> -	 */
> -	if (READ_ONCE(vma->anon_vma))
> -		return false;
> -
> -	/*
> -	 * When a vma is registered with uffd-wp or RWP, we cannot recycle
> -	 * the page table because there may be pte markers installed.
> -	 * VM_UFFD_RWP ranges similarly rely on per-PTE uffd state
> -	 * and cannot be recycled to a shared PMD. Other vmas can still
> -	 * have the same file mapped hugely, but skip this one: it will
> -	 * always be mapped in small page size for these registrations.
> -	 */
> -	if (userfaultfd_protected(vma))
> -		return false;
> -
> -	/*
> -	 * Preserve guard and poison markers when retracting page tables.
> -	 *
> -	 * These flags are set atomically on marker installation under mmap/VMA
> -	 * read lock, and here we may not hold any VMA or mmap lock at all.
> -	 *
> -	 * This is therefore serialised on the PTE page table lock, which is
> -	 * obtained on marker installation after the flag is set, so this
> -	 * check being performed under this lock excludes races.
> -	 */
> -	if (vma_test_atomic_flag(vma, VMA_MAYBE_GUARD_BIT) ||
> -	    vma_test_atomic_flag(vma, VMA_MAYBE_UFFD_POISON_BIT))
> -		return false;
> -
> -	return true;
> -}
> -
>  static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  {
>  	struct vm_area_struct *vma;
> @@ -2133,10 +2092,18 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  		struct mm_struct *mm;
>  		unsigned long addr;
>  		pmd_t *pmd, pgt_pmd;
> -		spinlock_t *pml;
> -		spinlock_t *ptl;
> +		pte_t *pte;
> +		spinlock_t *pml = NULL, *ptl;
>  		bool success = false;
>  
> +		/*
> +		 * Skip private mappings that have been written to: we'll likely
> +		 * split them as soon as we CoW later. Use READ_ONCE() because
> +		 * we are neither holding the VMA lock nor the mmap_lock.
> +		 */
> +		if (READ_ONCE(vma->anon_vma))
> +			continue;

[Severity: Medium]
Can removing the VM_COPY_ON_FORK flag checks here cause retract_page_tables()
to race with copy_page_range() during fork()?

In dup_mmap(), the child VMA is linked into mapping->i_mmap and
i_mmap_unlock_write(mapping) is dropped before copy_page_range() runs.
vma_needs_copy() copies page tables not only when vma_has_anon_rmap() is set,
but also when VM_COPY_ON_FORK flags (VM_MAYBE_GUARD, VM_MAYBE_UFFD_POISON,
VM_UFFD_WP, or VM_UFFD_RWP) are set on a file or shmem VMA whose anon_vma is
still NULL.

mm/memory.c:copy_pte_range() {
	/*
	 * copy_pmd_range()'s prior pmd_none_or_clear_bad(src_pmd), and the
	 * error handling here, assume that exclusive mmap_lock on dst and src
	 * protects anon from unexpected THP transitions; with shmem and file
	 * protected by mmap_lock-less collapse skipping areas with an anon rmap
	 * (whereas vma_needs_copy() skips areas without one).  A rework
	 * can remove such assumptions later, but this is good enough for now.
	 */
	dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
	if (!dst_pte) {
		ret = -ENOMEM;
		goto out;
	}
}

Because retract_page_tables() no longer skips VMAs with VM_COPY_ON_FORK flags
set, it can retract the newly allocated empty dst_pmd in the child between
__pte_alloc() and pte_offset_map_lock() (or across cond_resched() before a
non-empty PTE is copied):

CPU0                                      CPU1
dup_mmap()
  mapping_rmap_tree_insert_after(dst_vma, ...)
  i_mmap_unlock_write(mapping)
  copy_page_range(dst_vma, src_vma)
    ...
    copy_pte_range()
      pte_alloc_map_lock(dst_mm, dst_pmd, ...)
        __pte_alloc(dst_mm, dst_pmd)
                                          collapse_file()
                                            retract_page_tables()
                                              pte_table_empty(pte) == true
                                              pmdp_collapse_flush(..., dst_pmd)
        pte_offset_map_lock(dst_mm, dst_pmd, ...)
          --> returns NULL
      ret = -ENOMEM;  <-- fork() fails spuriously

On the parent side, retracting an empty src_pmd concurrently with
copy_pmd_range() can also clear *src_pmd between pmd_none(*src_pmd) and
pmd_bad(*src_pmd) inside pmd_none_or_clear_bad(src_pmd), triggering a false
pmd_clear_bad() report.

> +
>  		addr = vma->vm_start +
>  			((pgoff - vma_start_pgoff(vma)) << PAGE_SHIFT);
>  		if (addr & ~HPAGE_PMD_MASK ||
> @@ -2150,52 +2117,48 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  		if (collapse_test_exit(mm))
>  			continue;
>  
> -		if (!file_backed_vma_is_retractable(vma))
> -			continue;
> -
>  		/* PTEs were notified when unmapped; but now for the PMD? */
>  		mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
>  					addr, addr + HPAGE_PMD_SIZE);
>  		mmu_notifier_invalidate_range_start(&range);
>  
> -		pml = pmd_lock(mm, pmd);
[ ... ]
> +		pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
> +		if (!pte)
> +			goto unlock;
>  
>  		/*
> -		 * Huge page lock is still held, so normally the page table must
> -		 * remain empty; and we have already skipped vmas with an anon
> -		 * rmap and userfaultfd_wp() vmas.  But since the mmap_lock is not
> -		 * held, it is still possible for a racing userfaultfd_ioctl() or
> -		 * madvise() to have inserted ptes or markers.  Now that we hold
> -		 * ptlock, repeating the retractable checks protects us from
> -		 * races against the prior checks.
> +		 * The folio lock prevents file faults from modifying the
> +		 * page table, but other things (userfaultfd, CoW, guard
> +		 * markers) could have modified the page table until we lock it
> +		 * and check.
>  		 */
> -		if (likely(file_backed_vma_is_retractable(vma))) {
> -			pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
> -			pmdp_get_lockless_sync();
> -			success = true;
> -		}
> +		if (!pte_table_empty(pte))
> +			goto unlock;

[Severity: Medium]
Does checking pte_table_empty() instead of userfaultfd_protected(vma) break
userfaultfd RWP (and WP) on file and shmem VMAs?

Unlike VM_UFFD_WP on write-protected PTEs, try_to_unmap_one() does not
install PTE markers for VM_UFFD_RWP (or for VM_UFFD_WP PTEs that are not
currently write-protected) when collapse_file() unmaps the small folios.
pte_table_empty(pte) is then true and retract_page_tables() clears the PMD to
pmd_none, even though try_collapse_pte_mapped_thp() still refuses
userfaultfd_protected(vma).

Once the PMD is pmd_none, the next fault in finish_fault() installs a huge
PMD before checking userfaultfd_armed(vma):

mm/memory.c:finish_fault() {
	if (pmd_none(*vmf->pmd)) {
		if (!needs_fallback && folio_test_pmd_mappable(folio)) {
			ret = do_set_pmd(vmf, folio, page);
			if (ret != VM_FAULT_FALLBACK)
				return ret;
		}
    ...
	/* Using per-page fault to maintain the uffd semantics */
	if (unlikely(userfaultfd_armed(vma)) || unlikely(needs_fallback)) {
		nr_pages = 1;
}

When userspace later issues UFFDIO_RWPROTECT on that file or shmem VMA,
change_pmd_range() sees pgtable_split_needed() is true and calls
__split_huge_pmd(), which zaps the file huge PMD back to pmd_none without
populating a PTE table (since pgtable_populate_needed() is false for
MM_CP_UFFD_RWP), so RWP protection is lost and subsequent accesses fault the
unprotected huge PMD back in without delivering RWP faults.  Installing a
huge PMD also causes PAGEMAP_SCAN to report all 512 pages as accessed or
written after a single 4KB page fault.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-b4-uffd_marker_loss-v1-0-82926b22704c@kernel.org?part=2