Re: [PATCH v4 11/14] userfaultfd: add UFFD_FEATURE_RWP_ASYNC for async fault resolution

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Asynchronous UFFD RWP resolution creates invalid clean+writable page table entries, violating the software invariant that writable PTEs must be dirty.
--

commit 1c4c9040fe9c09a64714fa2d71bf254acc3ebf2f
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

userfaultfd: add UFFD_FEATURE_RWP_ASYNC for async fault resolution

This commit introduces asynchronous resolution for read-write protection
(RWP) faults in userfaultfd. It allows the kernel to auto-resolve faults
on protected pages by restoring PTE permissions in-place without blocking
the faulting thread.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index cd32bd51e3118..803fbc41e5014 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c

[ ... ]

> @@ -2291,7 +2291,30 @@ static inline bool can_change_pmd_writable(struct vm_area_struct *vma,
>  
>  vm_fault_t do_huge_pmd_uffd_rwp(struct vm_fault *vmf)
>  {

[ ... ]

> +	pmd = pmd_modify(vmf->orig_pmd, vma->vm_page_prot);
> +	/* pmd_modify() preserves _PAGE_UFFD; drop it on resolution */
> +	pmd = pmd_clear_uffd(pmd);
> +	pmd = pmd_mkyoung(pmd);
> +	if (!pmd_write(pmd) &&
> +	    vma_wants_manual_pte_write_upgrade(vma) &&
> +	    can_change_pmd_writable(vma, vmf->address, pmd))
> +		pmd = pmd_mkwrite(pmd, vma);

[Severity: Critical]
Does this create an invalid clean and writable PMD entry?

When upgrading the PMD to writable using pmd_mkwrite(), it looks like
pmd_mkdirty() also needs to be called. Writable page table entries must be
dirty.

If a clean anonymous page undergoes an asynchronous RWP resolution, it could
be upgraded to a clean and writable PMD. This allows modifications to the
page without the kernel's knowledge, which could cause the reclaim mechanism
to discard the modified page instead of writing it to swap.

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f63718296cc20..a5ff9018af060 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c

[ ... ]

> @@ -6070,7 +6070,37 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,

[ ... ]

> +		ptl = huge_pte_lock(h, mm, vmf.pte);
> +		pte = huge_ptep_get(mm, vmf.address, vmf.pte);
> +		if (pte_protnone(pte) && huge_pte_uffd(pte)) {
> +			unsigned int shift = huge_page_shift(h);
> +
> +			pte = huge_pte_modify(pte, vma->vm_page_prot);

[Severity: Critical]
Can this result in a clean and writable PTE for shared mappings?

The huge_pte_modify() function applies vma->vm_page_prot, which might be
writable for shared mappings. If the original PTE was clean, applying the
writable protection could create a clean and writable entry without
explicitly ensuring the dirty bit is set.

> diff --git a/mm/memory.c b/mm/memory.c
> index 3e393881031d2..89c9a44d07ced 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c

[ ... ]

> @@ -6137,8 +6137,31 @@ static void numa_rebuild_large_mapping(struct vm_fault *vmf, struct vm_area_stru
>  
>  static vm_fault_t do_uffd_rwp(struct vm_fault *vmf)
>  {

[ ... ]

> +	pte = pte_modify(vmf->orig_pte, vmf->vma->vm_page_prot);
> +	/* pte_modify() preserves _PAGE_UFFD; drop it on resolution */
> +	pte = pte_clear_uffd(pte);
> +	pte = pte_mkyoung(pte);
> +	if (!pte_write(pte) &&
> +	    vma_wants_manual_pte_write_upgrade(vmf->vma) &&
> +	    can_change_pte_writable(vmf->vma, vmf->address, pte))
> +		pte = pte_mkwrite(pte, vmf->vma);

[Severity: Critical]
Does this write upgrade need to set the dirty bit?

Similar to the PMD case above, upgrading to a writable PTE via
pte_mkwrite() without also calling pte_mkdirty() creates an invalid clean
and writable page table entry.

For comparison, the NUMA fault handling in do_numa_page() correctly sets
both when performing a write upgrade.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525113737.1942478-1-kas@kernel.org?part=11