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

[Peter Xu]

 On Thu, Mar 23, 2023 at 08:33:07PM +0500, Muhammad Usama Anjum wrote:
> Hi Peter,
> 
> Sorry for late reply.
> 
> On 3/22/23 12:50 AM, Peter Xu wrote:
> > On Tue, Mar 21, 2023 at 08:36:35PM +0100, David Hildenbrand wrote:
> >> 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?
> > 
> > I used a reproducer for the async mode I wrote (patch 2 attached, need to
> > change to VM_PRIVATE):
> > 
> > https://lore.kernel.org/all/ZBNr4nohj%2FTw4Zhw@x1n/
> > 
> > I don't think kernel kselftest can trigger it because we don't do strict
> > checks yet with uffd-wp bits.  I've already started looking into cleanup
> > the test cases and I do plan to add new tests to cover this.
> > 
> > Meanwhile, let's also wait for an ack from Muhammad.  Even though the async
> > mode is not part of the code base, it'll be a good test for verifying every
> > single uffd-wp bit being set or cleared as expected.
> I've tested by applying this patch. But the bug is still there. Just like
> Peter has mentioned, we are using our in progress patches related to
> pagemap_scan ioctl and userfaultd wp async patches to reproduce it.
> 
> To reproduce please build kernel and run pagemap_ioctl test in mm in
> hugetlb_mem_reproducer branch:
> https://gitlab.collabora.com/usama.anjum/linux-mainline/-/tree/hugetlb_mem_reproducer
> 
> In case you have any question on how to reproduce, please let me know. I'll
> try to provide a cleaner alternative.

Hmm, I think my current fix is incomplete if not wrong.  The root cause
should still be valid, however I overlooked another path:

	if (page_mapcount(old_page) == 1 && PageAnon(old_page)) {
		if (!PageAnonExclusive(old_page))
			page_move_anon_rmap(old_page, vma);
		if (likely(!unshare))
			set_huge_ptep_writable(vma, haddr, ptep);

		delayacct_wpcopy_end();
		return 0;
	}

We should bail out early in this path, and it'll be even easier we always
bail out hugetlb_wp() as long as uffd-wp is detected because userfault
should always be handled before any decision to CoW.

v2 attached.. Please give it another shot.

Thanks,

-- 
Peter Xu