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

[David Hildenbrand <david@redhat.com>]

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@collabora.com>
>>>>> Cc: linux-stable <stable@vger.kernel.org>
>>>>> Fixes: 166f3ecc0daf ("mm/hugetlb: hook page faults for uffd write protection")
>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>>> ---
>>>>>    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?

-- 
Thanks,

David / dhildenb