Re: [PATCH v9 07/15] mm: preserve RWP marker across PTE rewrites

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Unconditional arming of the RWP marker on moved pages during UFFDIO_MOVE destroys resolved page states and corrupts non-present huge PMDs.
--

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

mm: preserve RWP marker across PTE rewrites

This commit wires the preservation of the uffd PTE bit and PAGE_NONE
marker through various paths that rewrite PTEs on VM_UFFD_RWP VMAs,
including swap, device-exclusive, migration, fork, and mprotect.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index aca2f10fc1f1b..7ee7651202e2a 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c

[ ... ]

> @@ -2846,6 +2867,13 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
>  		_dst_pmd = move_soft_dirty_pmd(src_pmdval);
>  		_dst_pmd = clear_uffd_wp_pmd(_dst_pmd);
>  	}
> +
> +	/* Re-arm RWP on the moved PMD if dst_vma is RWP-registered. */
> +	if (userfaultfd_rwp(dst_vma)) {
> +		_dst_pmd = pmd_modify(_dst_pmd, PAGE_NONE);
> +		_dst_pmd = pmd_mkuffd(_dst_pmd);
> +	}
> +
>  	set_pmd_at(mm, dst_addr, dst_pmd, _dst_pmd);
>  
>  	src_pgtable = pgtable_trans_huge_withdraw(mm, src_pmd);

[Severity: Critical]
Does this corrupt non-present PMDs like migration entries?

In move_pages_huge_pmd(), if the source is a non-present huge PMD
(such as a migration entry), the code takes the !src_folio path.
Applying pmd_modify() with PAGE_NONE to this entry will set the present
bit, because PAGE_NONE contains the present bit.

If a user thread accesses this corrupted migration entry, won't the page
fault handler treat it as a present PMD, call pmd_folio(), and dereference
the swap offset/type data as a physical PFN, causing a panic?

> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index ec1880d181711..4fca0409b7d7c 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1292,6 +1292,13 @@ static long move_present_ptes(struct mm_struct *mm,
>  		if (pte_dirty(orig_src_pte))
>  			orig_dst_pte = pte_mkdirty(orig_dst_pte);
>  		orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +
> +		/* Re-arm RWP on the moved PTE if dst_vma is RWP-registered. */
> +		if (userfaultfd_rwp(dst_vma)) {
> +			orig_dst_pte = pte_modify(orig_dst_pte, PAGE_NONE);
> +			orig_dst_pte = pte_mkuffd(orig_dst_pte);
> +		}
> +
>  		set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
>  
>  		src_addr += PAGE_SIZE;

[Severity: Critical]
Can this unconditional modification destroy the resolved state of the PTE?

In move_present_ptes(), the source PTE might have already been resolved
and have valid access state. By unconditionally applying PAGE_NONE to the
destination PTE regardless of the source PTE's state, won't this force a
spurious RWP fault on the next access?

> @@ -1373,6 +1380,9 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
>  	orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
>  	if (pgtable_supports_soft_dirty())
>  		orig_src_pte = pte_swp_mksoft_dirty(orig_src_pte);
> +	/* Re-arm RWP on the moved swap entry if dst_vma is RWP-registered. */
> +	if (userfaultfd_rwp(dst_vma))
> +		orig_src_pte = pte_swp_mkuffd(orig_src_pte);
>  	set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
>  	double_pt_unlock(dst_ptl, src_ptl);
>  
> @@ -1399,6 +1409,13 @@ static int move_zeropage_pte(struct mm_struct *mm,
>  
>  	zero_pte = pte_mkspecial(pfn_pte(zero_pfn(dst_addr),
>  					 dst_vma->vm_page_prot));
> +
> +	/* Re-arm RWP on the moved PTE if dst_vma is RWP-registered. */
> +	if (userfaultfd_rwp(dst_vma)) {
> +		zero_pte = pte_modify(zero_pte, PAGE_NONE);
> +		zero_pte = pte_mkuffd(zero_pte);
> +	}
> +
>  	ptep_clear_flush(src_vma, src_addr, src_pte);
>  	set_pte_at(mm, dst_addr, dst_pte, zero_pte);
>  	double_pt_unlock(dst_ptl, src_ptl);

[Severity: Critical]
Does unconditionally arming the RWP marker here in move_swap_pte() and 
move_zeropage_pte() cause similar issues with state loss or spurious faults 
when the pages are moved via UFFDIO_MOVE?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706084805.8400-1-kirill@shutemov.name?part=7