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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/8/26 00:51, sashiko-bot@kernel.org wrote:
> 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.

Ah, try_collapse_pte_mapped_thp() runs withe the mmap lock, whereby
retract_page_tables() only holds the rmap lock.



> 
> 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.

Yeah, that would likely need a different handling.

> 
>> +
>>  		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.

"Since pgtable_populate_needed() is false for MM_CP_UFFD_RWP" I think RWP also
needs proper uffd marker support, independent of the change here.
-- 
Cheers,

David