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

[Peter Xu]

On Fri, Mar 24, 2023 at 09:51:27AM +0100, David Hildenbrand wrote:
> On 23.03.23 23:11, Peter Xu wrote:
> > 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.
> 
> Hmmm, I think you must only do that for !unshare (FAULT_FLAG_WRITE).
> Otherwise you'll never be able to resolve an unsharing request on a r/o
> mapped hugetlb page that has the uffd-wp set?
> 
> Or am I missing something?

No, I think you're right. I'll fix that up when posting a v3. Thanks,

-- 
Peter Xu