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

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

[Peter Xu]

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

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

[Peter Xu]

Mike,

For some reason I forgot to copy you.. sorry.  Here's the link:

https://lore.kernel.org/linux-mm/20230321191840.1897940-1-peterx@redhat.com

On Tue, Mar 21, 2023 at 03:18:40PM -0400, 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);
> -- 
> 2.39.1
> 

-- 
Peter Xu

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@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?

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

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

[Peter Xu]

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.

> Acked-by: David Hildenbrand <david@redhat.com>

Thanks,

-- 
Peter Xu

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

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

-- 
Mike Kravetz

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

[Peter Xu]

On Tue, Mar 21, 2023 at 01:57:19PM -0700, Mike Kravetz 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
>                ^^^^
> 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.

Not intended..  Since this is not fixup-able, I'll reword here when I'll
need to repost.

[...]

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

Thanks!

-- 
Peter Xu

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

[Muhammad Usama Anjum]

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.

> 
>> Acked-by: David Hildenbrand <david@redhat.com>
> 
> Thanks,
> 

-- 
BR,
Muhammad Usama Anjum

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

[Peter Xu]

[-- Attachment #1: Type: text/plain, Size: 5588 bytes --]

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.

Thanks,

-- 
Peter Xu

[-- Attachment #2: 0001-mm-hugetlb-Fix-uffd-wr-protection-for-CoW-optimizati.patch --]
[-- Type: text/plain, Size: 3584 bytes --]

From 4a294f9ec5d2ba94a6a7ecf03bd096ea35902f2f Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Tue, 21 Mar 2023 14:58:42 -0400
Subject: [PATCH v2] mm/hugetlb: Fix uffd wr-protection for CoW optimization path

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 hugetlb private
mappings, when someone firstly wr-protects a missing pte (which will
install a pte marker), followed by a write to the page.  That will trigger
a missing fault and an optimized CoW in the same fault stack.

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) Skip hugetlb_wp() when uffd-wp bit is still set.  It means it
  requires another hugetlb_fault() to resolve the uffd-wp bit first.

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 8bfd07f4c143..b60959f2a3f0 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 = huge_ptep_get(ptep);
 	struct hstate *h = hstate_vma(vma);
 	struct page *old_page;
 	struct folio *new_folio;
@@ -5487,6 +5487,17 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 	unsigned long haddr = address & huge_page_mask(h);
 	struct mmu_notifier_range range;
 
+	/*
+	 * Never handle CoW for uffd-wp protected pages.  It should be only
+	 * handled when the uffd-wp protection is removed.
+	 *
+	 * Note that only the CoW optimization path can trigger this and
+	 * got skipped, because hugetlb_fault() will always resolve uffd-wp
+	 * bit first.
+	 */
+	if (huge_pte_uffd_wp(pte))
+		return 0;
+
 	/*
 	 * hugetlb does not support FOLL_FORCE-style write faults that keep the
 	 * PTE mapped R/O such as maybe_mkwrite() would do.
@@ -5500,7 +5511,6 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 		return 0;
 	}
 
-	pte = huge_ptep_get(ptep);
 	old_page = pte_page(pte);
 
 	delayacct_wpcopy_start();
-- 
2.39.1

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

[Muhammad Usama Anjum]

On 3/24/23 3:11 AM, 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.
This attached v2 works. Please add:

Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> 
> Thanks,
> 

-- 
BR,
Muhammad Usama Anjum

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

[David Hildenbrand]

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

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@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?

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

-- 
Peter Xu