Re: [PATCH] mm/hugetlb: Fix uffd wr-protection for CoW optimization path

[David Hildenbrand]

On 21.03.23 20:18, Peter Xu wrote:
> This patch fixes an issue that a hugetlb uffd-wr-protected mapping can be
> writable even with uffd-wp bit set.  It only happens with all these
> conditions met: (1) hugetlb memory (2) private mapping (3) original mapping
> was missing, then (4) being wr-protected (IOW, pte marker installed).  Then
> write to the page to trigger.
>
> Userfaultfd-wp trap for hugetlb was implemented in hugetlb_fault() before
> even reaching hugetlb_wp() to avoid taking more locks that userfault won't
> need.  However there's one CoW optimization path for missing hugetlb page
> that can trigger hugetlb_wp() inside hugetlb_no_page(), that can bypass the
> userfaultfd-wp traps.
>
> A few ways to resolve this:
>
>    (1) Skip the CoW optimization for hugetlb private mapping, considering
>    that private mappings for hugetlb should be very rare, so it may not
>    really be helpful to major workloads.  The worst case is we only skip the
>    optimization if userfaultfd_wp(vma)==true, because uffd-wp needs another
>    fault anyway.
>
>    (2) Move the userfaultfd-wp handling for hugetlb from hugetlb_fault()
>    into hugetlb_wp().  The major cons is there're a bunch of locks taken
>    when calling hugetlb_wp(), and that will make the changeset unnecessarily
>    complicated due to the lock operations.
>
>    (3) Carry over uffd-wp bit in hugetlb_wp(), so it'll need to fault again
>    for uffd-wp privately mapped pages.
>
> This patch chose option (3) which contains the minimum changeset (simplest
> for backport) and also make sure hugetlb_wp() itself will start to be
> always safe with uffd-wp ptes even if called elsewhere in the future.
>
> This patch will be needed for v5.19+ hence copy stable.
>
> Reported-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>
> Cc: linux-stable <stable@xxxxxxxxxxxxxxx>
> Fixes: 166f3ecc0daf ("mm/hugetlb: hook page faults for uffd write protection")
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
>   mm/hugetlb.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 8bfd07f4c143..22337b191eae 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5478,7 +5478,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
>   		       struct folio *pagecache_folio, spinlock_t *ptl)
>   {
>   	const bool unshare = flags & FAULT_FLAG_UNSHARE;
> -	pte_t pte;
> +	pte_t pte, newpte;
>   	struct hstate *h = hstate_vma(vma);
>   	struct page *old_page;
>   	struct folio *new_folio;
> @@ -5622,8 +5622,10 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
>   		mmu_notifier_invalidate_range(mm, range.start, range.end);
>   		page_remove_rmap(old_page, vma, true);
>   		hugepage_add_new_anon_rmap(new_folio, vma, haddr);
> -		set_huge_pte_at(mm, haddr, ptep,
> -				make_huge_pte(vma, &new_folio->page, !unshare));
> +		newpte = make_huge_pte(vma, &new_folio->page, !unshare);
> +		if (huge_pte_uffd_wp(pte))
> +			newpte = huge_pte_mkuffd_wp(newpte);
> +		set_huge_pte_at(mm, haddr, ptep, newpte);
>   		folio_set_hugetlb_migratable(new_folio);
>   		/* Make the old page be freed below */
>   		new_folio = page_folio(old_page);

Looks correct to me. Do we have a reproducer?

Acked-by: David Hildenbrand <david@xxxxxxxxxx>

--
Thanks,

David / dhildenb