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

[Mike Kravetz]

On 03/21/23 15:18, Peter Xu wrote:

Thanks Peter!

> 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
               ^^^^
Nit, but is the word "then" intended to be there?  Almost makes it sound as
if wr-protected was a result of the previous 3 conditions being met.

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

I was going to suggest (1) as that would be the simplest.  But, since (3)
makes hugetlb_wp() safe for future callers, that is actually preferred.

> 
> 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);
> -- 
> 2.39.1
> 

Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>

-- 
Mike Kravetz