Re: [PATCH v8 06/15] mm: add MM_CP_UFFD_RWP change_protection() flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unnecessary splitting of file-backed THPs when applying userfaultfd RWP.
- [High] UFFD RWP protection is silently bypassed on reclaimed file-backed pages because PTE markers are intentionally skipped.
- [Medium] Missing safety check and unimplemented logic for UFFD RWP in PUD-sized THPs.
--

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

mm: add MM_CP_UFFD_RWP change_protection() flag
    
Preparatory patch. Add the change_protection() primitive that
userfaultfd RWP will use.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 5b60862684aa6..b352b0dfe1ba2 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c

[Severity: Medium]
Could this change miss updating change_huge_pud() to reject
MM_CP_UFFD_RWP_ALL?

Currently, change_huge_pud() explicitly rejects MM_CP_UFFD_WP_ALL since
anonymous PUDs are not yet supported. By omitting a similar check for
MM_CP_UFFD_RWP_ALL, an anonymous PUD could be modified to PAGE_NONE without
the _PAGE_UFFD bit. 

Would this cause future faults to be misidentified as NUMA or mprotect faults
if anonymous PUDs are introduced?

[ ... ]

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 8340c8b228c67..7dcc94e7bfd60 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
[ ... ]
> @@ -325,6 +327,7 @@ static long change_pte_range(struct mmu_gather *tlb,
>  	long pages = 0;
>  	bool is_private_single_threaded;
>  	bool prot_numa = cp_flags & MM_CP_PROT_NUMA;
> +	bool uffd_rwp = cp_flags & MM_CP_UFFD_RWP;
>  	bool uffd_wp = cp_flags & MM_CP_UFFD_WP;
>  	int nr_ptes;

[Severity: High]
Does skipping PTE markers for uffd_rwp on pte_none entries allow silent
protection bypass under memory pressure?

When memory pressure reclaims a file-backed page protected with
UFFDIO_RWPROTECT, try_to_unmap_one() in mm/rmap.c clears the PTE to pte_none()
without preserving the UFFD state.

Since change_pte_range() explicitly skips installing markers when uffd_rwp is
set and oldpte is pte_none():

mm/mprotect.c:change_pte_range() {
        ...
        } else if (pte_none(oldpte)) {
                if (likely(!uffd_wp))
                        continue;
        ...
}

Would the page fault handler subsequently repopulate the PTE with normal
RW permissions from the page cache? This appears to lose the read-write
protection once a page is reclaimed.

[ ... ]

> @@ -428,7 +441,7 @@ pgtable_split_needed(struct vm_area_struct *vma, unsigned long cp_flags)
>  	 * (e.g. 2M shmem) because file thp is handled differently when
>  	 * split by erasing the pmd so far.
>  	 */
> -	return (cp_flags & MM_CP_UFFD_WP) && !vma_is_anonymous(vma);
> +	return (cp_flags & (MM_CP_UFFD_WP | MM_CP_UFFD_RWP)) && !vma_is_anonymous(vma);
>  }

[Severity: High]
Does this logic unnecessarily split file-backed THPs when applying
MM_CP_UFFD_RWP?

When UFFDIO_RWPROTECT is applied to a file-backed THP, change_pmd_range()
evaluates pgtable_split_needed(). Since this now returns true for
MM_CP_UFFD_RWP on non-anonymous VMAs, it forces __split_huge_pmd().

For file-backed THPs, splitting clears the PMD and creates unpopulated PTEs.
Because RWP intentionally skips installing markers for unpopulated PTEs, the
protection can be silently bypassed. Is it possible to avoid splitting here
since RWP does not require PTE markers?

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