[PATCH 0/2] mm: Fixes on pte markers

[PATCH 0/2] mm: Fixes on pte markers

[Peter Xu]

Patch 1 resolves the syzkiller report from Pengfei.

Patch 2 further harden pte markers when used with the recent swapin error
markers.  The major case is we should persist a swapin error marker after
fork(), so child shouldn't read a corrupted page.

No report so far with patch 2, but it can be somehow tested with things
like:
https://github.com/xzpeter/clibs/blob/master/misc/pageout.c
Plus some hacks.

Please have a look, thanks.

Peter Xu (2):
  mm/uffd: Fix pte marker when fork() without fork event
  mm: Fix a few rare cases of using swapin error pte marker

 mm/hugetlb.c  |  3 +++
 mm/memory.c   | 14 +++++++-------
 mm/mprotect.c |  8 +++++++-
 3 files changed, 17 insertions(+), 8 deletions(-)

-- 
2.37.3

[PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[Peter Xu]

When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may
have it and has pte marker installed.  The warning is improper along with
the comment.  The right thing is to inherit the pte marker when needed, or
keep the dst pte empty.

A vague guess is this happened by an accident when there's the prior patch
to introduce src/dst vma into this helper during the uffd-wp feature got
developed and I probably messed up in the rebase, since if we replace
dst_vma with src_vma the warning & comment it all makes sense too.

Hugetlb did exactly the right here (copy_hugetlb_page_range()).  Fix the
general path.

Reproducer:

https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c

Cc: <stable@vger.kernel.org> # 5.19+
Fixes: c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
Reported-by: Pengfei Xu <pengfei.xu@intel.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/memory.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index aad226daf41b..032ef700c3e8 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 			return -EBUSY;
 		return -ENOENT;
 	} else if (is_pte_marker_entry(entry)) {
-		/*
-		 * We're copying the pgtable should only because dst_vma has
-		 * uffd-wp enabled, do sanity check.
-		 */
-		WARN_ON_ONCE(!userfaultfd_wp(dst_vma));
-		set_pte_at(dst_mm, addr, dst_pte, pte);
+		if (userfaultfd_wp(dst_vma))
+			set_pte_at(dst_mm, addr, dst_pte, pte);
 		return 0;
 	}
 	if (!userfaultfd_wp(dst_vma))
-- 
2.37.3

[PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker

[Peter Xu]

This patch should harden commit 15520a3f0469 ("mm: use pte markers for swap
errors") on using pte markers for swapin errors on a few corner cases.

1. Propagate swapin errors across fork()s: if there're swapin errors in
   the parent mm, after fork()s the child should sigbus too when an error
   page is accessed.

2. Fix a rare condition race in pte_marker_clear() where a uffd-wp pte
   marker can be quickly switched to a swapin error.

3. Explicitly ignore swapin error pte markers in change_protection().

I mostly don't worry on (2) or (3) at all, but we should still have them.
Case (1) is special because it can potentially cause silent data corrupt on
child when parent has swapin error triggered with swapoff, but since swapin
error is rare itself already it's probably not easy to trigger either.

Currently there is a priority difference between the uffd-wp bit and the
swapin error entry, in which the swapin error always has higher
priority (e.g. we don't need to wr-protect a swapin error pte marker).

If there will be a 3rd bit introduced, we'll probably need to consider a
more involved approach so we may need to start operate on the bits.  Let's
leave that for later.

This patch is tested with case (1) explicitly where we'll get corrupted
data before in the child if there's existing swapin error pte markers, and
after patch applied the child can be rightfully killed.

We don't need to copy stable for this one since 15520a3f0469 just landed as
part of v6.2-rc1, only "Fixes" applied.

Fixes: 15520a3f0469 ("mm: use pte markers for swap errors")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/hugetlb.c  | 3 +++
 mm/memory.c   | 8 ++++++--
 mm/mprotect.c | 8 +++++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f5f445c39dbc..1e8e4eb10328 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 				entry = huge_pte_clear_uffd_wp(entry);
 			set_huge_pte_at(dst, addr, dst_pte, entry);
 		} else if (unlikely(is_pte_marker(entry))) {
+			/* No swap on hugetlb */
+			WARN_ON_ONCE(
+			    is_swapin_error_entry(pte_to_swp_entry(entry)));
 			/*
 			 * We copy the pte marker only if the dst vma has
 			 * uffd-wp enabled.
diff --git a/mm/memory.c b/mm/memory.c
index 032ef700c3e8..3e836fecd035 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 			return -EBUSY;
 		return -ENOENT;
 	} else if (is_pte_marker_entry(entry)) {
-		if (userfaultfd_wp(dst_vma))
+		if (is_swapin_error_entry(entry) || userfaultfd_wp(dst_vma))
 			set_pte_at(dst_mm, addr, dst_pte, pte);
 		return 0;
 	}
@@ -3625,8 +3625,12 @@ static vm_fault_t pte_marker_clear(struct vm_fault *vmf)
 	/*
 	 * Be careful so that we will only recover a special uffd-wp pte into a
 	 * none pte.  Otherwise it means the pte could have changed, so retry.
+	 *
+	 * This should also cover the case where e.g. the pte changed
+	 * quickly from a PTE_MARKER_UFFD_WP into PTE_MARKER_SWAPIN_ERROR.
+	 * So is_pte_marker() check is not enough to safely drop the pte.
 	 */
-	if (is_pte_marker(*vmf->pte))
+	if (pte_same(vmf->orig_pte, *vmf->pte))
 		pte_clear(vmf->vma->vm_mm, vmf->address, vmf->pte);
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
 	return 0;
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 093cb50f2fc4..a6f905211327 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -245,7 +245,13 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,
 					newpte = pte_swp_mksoft_dirty(newpte);
 				if (pte_swp_uffd_wp(oldpte))
 					newpte = pte_swp_mkuffd_wp(newpte);
-			} else if (pte_marker_entry_uffd_wp(entry)) {
+			} else if (is_pte_marker_entry(entry)) {
+				/*
+				 * Ignore swapin errors unconditionally,
+				 * because any access should sigbus anyway.
+				 */
+				if (is_swapin_error_entry(entry))
+					continue;
 				/*
 				 * If this is uffd-wp pte marker and we'd like
 				 * to unprotect it, drop it; the next page
-- 
2.37.3

Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker

[Huang, Ying]

Peter Xu <peterx@redhat.com> writes:

> This patch should harden commit 15520a3f0469 ("mm: use pte markers for swap
> errors") on using pte markers for swapin errors on a few corner cases.
>
> 1. Propagate swapin errors across fork()s: if there're swapin errors in
>    the parent mm, after fork()s the child should sigbus too when an error
>    page is accessed.
>
> 2. Fix a rare condition race in pte_marker_clear() where a uffd-wp pte
>    marker can be quickly switched to a swapin error.
>
> 3. Explicitly ignore swapin error pte markers in change_protection().
>
> I mostly don't worry on (2) or (3) at all, but we should still have them.
> Case (1) is special because it can potentially cause silent data corrupt on
> child when parent has swapin error triggered with swapoff, but since swapin
> error is rare itself already it's probably not easy to trigger either.
>
> Currently there is a priority difference between the uffd-wp bit and the
> swapin error entry, in which the swapin error always has higher
> priority (e.g. we don't need to wr-protect a swapin error pte marker).
>
> If there will be a 3rd bit introduced, we'll probably need to consider a
> more involved approach so we may need to start operate on the bits.  Let's
> leave that for later.
>
> This patch is tested with case (1) explicitly where we'll get corrupted
> data before in the child if there's existing swapin error pte markers, and
> after patch applied the child can be rightfully killed.
>
> We don't need to copy stable for this one since 15520a3f0469 just landed as
> part of v6.2-rc1, only "Fixes" applied.
>
> Fixes: 15520a3f0469 ("mm: use pte markers for swap errors")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  mm/hugetlb.c  | 3 +++
>  mm/memory.c   | 8 ++++++--
>  mm/mprotect.c | 8 +++++++-
>  3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f5f445c39dbc..1e8e4eb10328 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  				entry = huge_pte_clear_uffd_wp(entry);
>  			set_huge_pte_at(dst, addr, dst_pte, entry);
>  		} else if (unlikely(is_pte_marker(entry))) {
> +			/* No swap on hugetlb */
> +			WARN_ON_ONCE(
> +			    is_swapin_error_entry(pte_to_swp_entry(entry)));
>  			/*
>  			 * We copy the pte marker only if the dst vma has
>  			 * uffd-wp enabled.
> diff --git a/mm/memory.c b/mm/memory.c
> index 032ef700c3e8..3e836fecd035 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  			return -EBUSY;
>  		return -ENOENT;
>  	} else if (is_pte_marker_entry(entry)) {
> -		if (userfaultfd_wp(dst_vma))
> +		if (is_swapin_error_entry(entry) || userfaultfd_wp(dst_vma))

Should we do this in [1/2]?  It appears that we introduce an issue in
[1/2] and fix it in [2/2]?

Best Regards,
Huang, Ying

>  			set_pte_at(dst_mm, addr, dst_pte, pte);
>  		return 0;
>  	}
> @@ -3625,8 +3625,12 @@ static vm_fault_t pte_marker_clear(struct vm_fault *vmf)
>  	/*
>  	 * Be careful so that we will only recover a special uffd-wp pte into a
>  	 * none pte.  Otherwise it means the pte could have changed, so retry.
> +	 *
> +	 * This should also cover the case where e.g. the pte changed
> +	 * quickly from a PTE_MARKER_UFFD_WP into PTE_MARKER_SWAPIN_ERROR.
> +	 * So is_pte_marker() check is not enough to safely drop the pte.
>  	 */
> -	if (is_pte_marker(*vmf->pte))
> +	if (pte_same(vmf->orig_pte, *vmf->pte))
>  		pte_clear(vmf->vma->vm_mm, vmf->address, vmf->pte);
>  	pte_unmap_unlock(vmf->pte, vmf->ptl);
>  	return 0;
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 093cb50f2fc4..a6f905211327 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -245,7 +245,13 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,
>  					newpte = pte_swp_mksoft_dirty(newpte);
>  				if (pte_swp_uffd_wp(oldpte))
>  					newpte = pte_swp_mkuffd_wp(newpte);
> -			} else if (pte_marker_entry_uffd_wp(entry)) {
> +			} else if (is_pte_marker_entry(entry)) {
> +				/*
> +				 * Ignore swapin errors unconditionally,
> +				 * because any access should sigbus anyway.
> +				 */
> +				if (is_swapin_error_entry(entry))
> +					continue;
>  				/*
>  				 * If this is uffd-wp pte marker and we'd like
>  				 * to unprotect it, drop it; the next page

Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker

[Peter Xu]

On Thu, Dec 15, 2022 at 03:12:13PM +0800, Huang, Ying wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > This patch should harden commit 15520a3f0469 ("mm: use pte markers for swap
> > errors") on using pte markers for swapin errors on a few corner cases.
> >
> > 1. Propagate swapin errors across fork()s: if there're swapin errors in
> >    the parent mm, after fork()s the child should sigbus too when an error
> >    page is accessed.
> >
> > 2. Fix a rare condition race in pte_marker_clear() where a uffd-wp pte
> >    marker can be quickly switched to a swapin error.
> >
> > 3. Explicitly ignore swapin error pte markers in change_protection().
> >
> > I mostly don't worry on (2) or (3) at all, but we should still have them.
> > Case (1) is special because it can potentially cause silent data corrupt on
> > child when parent has swapin error triggered with swapoff, but since swapin
> > error is rare itself already it's probably not easy to trigger either.
> >
> > Currently there is a priority difference between the uffd-wp bit and the
> > swapin error entry, in which the swapin error always has higher
> > priority (e.g. we don't need to wr-protect a swapin error pte marker).
> >
> > If there will be a 3rd bit introduced, we'll probably need to consider a
> > more involved approach so we may need to start operate on the bits.  Let's
> > leave that for later.
> >
> > This patch is tested with case (1) explicitly where we'll get corrupted
> > data before in the child if there's existing swapin error pte markers, and
> > after patch applied the child can be rightfully killed.
> >
> > We don't need to copy stable for this one since 15520a3f0469 just landed as
> > part of v6.2-rc1, only "Fixes" applied.
> >
> > Fixes: 15520a3f0469 ("mm: use pte markers for swap errors")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  mm/hugetlb.c  | 3 +++
> >  mm/memory.c   | 8 ++++++--
> >  mm/mprotect.c | 8 +++++++-
> >  3 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index f5f445c39dbc..1e8e4eb10328 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
> >  				entry = huge_pte_clear_uffd_wp(entry);
> >  			set_huge_pte_at(dst, addr, dst_pte, entry);
> >  		} else if (unlikely(is_pte_marker(entry))) {
> > +			/* No swap on hugetlb */
> > +			WARN_ON_ONCE(
> > +			    is_swapin_error_entry(pte_to_swp_entry(entry)));
> >  			/*
> >  			 * We copy the pte marker only if the dst vma has
> >  			 * uffd-wp enabled.
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 032ef700c3e8..3e836fecd035 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> >  			return -EBUSY;
> >  		return -ENOENT;
> >  	} else if (is_pte_marker_entry(entry)) {
> > -		if (userfaultfd_wp(dst_vma))
> > +		if (is_swapin_error_entry(entry) || userfaultfd_wp(dst_vma))
> 
> Should we do this in [1/2]?  It appears that we introduce an issue in
> [1/2] and fix it in [2/2]?

Patch 1 copied stable with 5.19+, this one is not.

So if we want to squash, we may want to squash both patches into one, then
we'll need an explicit follow up on stable branch with something like patch
1.  The current way works easier for stable, but I can also do the other.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker

[Huang, Ying]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Dec 15, 2022 at 03:12:13PM +0800, Huang, Ying wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > This patch should harden commit 15520a3f0469 ("mm: use pte markers for swap
>> > errors") on using pte markers for swapin errors on a few corner cases.
>> >
>> > 1. Propagate swapin errors across fork()s: if there're swapin errors in
>> >    the parent mm, after fork()s the child should sigbus too when an error
>> >    page is accessed.
>> >
>> > 2. Fix a rare condition race in pte_marker_clear() where a uffd-wp pte
>> >    marker can be quickly switched to a swapin error.
>> >
>> > 3. Explicitly ignore swapin error pte markers in change_protection().
>> >
>> > I mostly don't worry on (2) or (3) at all, but we should still have them.
>> > Case (1) is special because it can potentially cause silent data corrupt on
>> > child when parent has swapin error triggered with swapoff, but since swapin
>> > error is rare itself already it's probably not easy to trigger either.
>> >
>> > Currently there is a priority difference between the uffd-wp bit and the
>> > swapin error entry, in which the swapin error always has higher
>> > priority (e.g. we don't need to wr-protect a swapin error pte marker).
>> >
>> > If there will be a 3rd bit introduced, we'll probably need to consider a
>> > more involved approach so we may need to start operate on the bits.  Let's
>> > leave that for later.
>> >
>> > This patch is tested with case (1) explicitly where we'll get corrupted
>> > data before in the child if there's existing swapin error pte markers, and
>> > after patch applied the child can be rightfully killed.
>> >
>> > We don't need to copy stable for this one since 15520a3f0469 just landed as
>> > part of v6.2-rc1, only "Fixes" applied.
>> >
>> > Fixes: 15520a3f0469 ("mm: use pte markers for swap errors")
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  mm/hugetlb.c  | 3 +++
>> >  mm/memory.c   | 8 ++++++--
>> >  mm/mprotect.c | 8 +++++++-
>> >  3 files changed, 16 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> > index f5f445c39dbc..1e8e4eb10328 100644
>> > --- a/mm/hugetlb.c
>> > +++ b/mm/hugetlb.c
>> > @@ -4884,6 +4884,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>> >  				entry = huge_pte_clear_uffd_wp(entry);
>> >  			set_huge_pte_at(dst, addr, dst_pte, entry);
>> >  		} else if (unlikely(is_pte_marker(entry))) {
>> > +			/* No swap on hugetlb */
>> > +			WARN_ON_ONCE(
>> > +			    is_swapin_error_entry(pte_to_swp_entry(entry)));
>> >  			/*
>> >  			 * We copy the pte marker only if the dst vma has
>> >  			 * uffd-wp enabled.
>> > diff --git a/mm/memory.c b/mm/memory.c
>> > index 032ef700c3e8..3e836fecd035 100644
>> > --- a/mm/memory.c
>> > +++ b/mm/memory.c
>> > @@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>> >  			return -EBUSY;
>> >  		return -ENOENT;
>> >  	} else if (is_pte_marker_entry(entry)) {
>> > -		if (userfaultfd_wp(dst_vma))
>> > +		if (is_swapin_error_entry(entry) || userfaultfd_wp(dst_vma))
>> 
>> Should we do this in [1/2]?  It appears that we introduce an issue in
>> [1/2] and fix it in [2/2]?
>
> Patch 1 copied stable with 5.19+, this one is not.
>
> So if we want to squash, we may want to squash both patches into one, then
> we'll need an explicit follow up on stable branch with something like patch
> 1.  The current way works easier for stable, but I can also do the other.

Got it.  Thanks for explanation.  It's OK for me to keep them in current
way.

Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[David Hildenbrand]

On 14.12.22 21:04, Peter Xu wrote:
> When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may
> have it and has pte marker installed.  The warning is improper along with
> the comment.  The right thing is to inherit the pte marker when needed, or
> keep the dst pte empty.
> 
> A vague guess is this happened by an accident when there's the prior patch
> to introduce src/dst vma into this helper during the uffd-wp feature got
> developed and I probably messed up in the rebase, since if we replace
> dst_vma with src_vma the warning & comment it all makes sense too.
> 
> Hugetlb did exactly the right here (copy_hugetlb_page_range()).  Fix the
> general path.
> 
> Reproducer:
> 
> https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
> 
> Cc: <stable@vger.kernel.org> # 5.19+
> Fixes: c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   mm/memory.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index aad226daf41b..032ef700c3e8 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>   			return -EBUSY;
>   		return -ENOENT;
>   	} else if (is_pte_marker_entry(entry)) {
> -		/*
> -		 * We're copying the pgtable should only because dst_vma has
> -		 * uffd-wp enabled, do sanity check.
> -		 */
> -		WARN_ON_ONCE(!userfaultfd_wp(dst_vma));
> -		set_pte_at(dst_mm, addr, dst_pte, pte);
> +		if (userfaultfd_wp(dst_vma))
> +			set_pte_at(dst_mm, addr, dst_pte, pte);
>   		return 0;
>   	}
>   	if (!userfaultfd_wp(dst_vma))

Staring at the code first made me go "what about other PTE markers". I 
then looked into the discussion in patch #2. The fix as is is 
suboptimal, because it

1) Removes the warning which is good, but
2) Silently drops swapin errors now

So it silently breaks something else temporarily ...


I remember, that theoretically we could have multiple markers stored in 
a single PTE marker.

Wouldn't it be cleaner to be able to "clean" specific markers from a PTE 
marker without having to special case on each and everyone? I mean, only 
uffd-wp is really special such that it might disappear for the target.

Something like (pseudocode):

if (!userfaultfd_wp(dst_vma))
	pte_marker_clear_uff_wp(entry);
if (!pte_marker_empty(entry)) {
	pte = make_pte_marker(pte_marker_get(entry));
	set_pte_at(dst_mm, addr, dst_pte, pte);
}

Then this fix would be correct and backport-able even without #2. And it
would work for new types of markers :)


I'd prefer a fix that doesn't break something else temporarily, even if 
the stable backport might require 5 additional minutes to do. So 
squashing #2 into #1 would also work.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[Peter Xu]

On Fri, Dec 16, 2022 at 10:04:27AM +0100, David Hildenbrand wrote:
> On 14.12.22 21:04, Peter Xu wrote:
> > When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may
> > have it and has pte marker installed.  The warning is improper along with
> > the comment.  The right thing is to inherit the pte marker when needed, or
> > keep the dst pte empty.
> > 
> > A vague guess is this happened by an accident when there's the prior patch
> > to introduce src/dst vma into this helper during the uffd-wp feature got
> > developed and I probably messed up in the rebase, since if we replace
> > dst_vma with src_vma the warning & comment it all makes sense too.
> > 
> > Hugetlb did exactly the right here (copy_hugetlb_page_range()).  Fix the
> > general path.
> > 
> > Reproducer:
> > 
> > https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
> > 
> > Cc: <stable@vger.kernel.org> # 5.19+
> > Fixes: c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
> > Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   mm/memory.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index aad226daf41b..032ef700c3e8 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -828,12 +828,8 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> >   			return -EBUSY;
> >   		return -ENOENT;
> >   	} else if (is_pte_marker_entry(entry)) {
> > -		/*
> > -		 * We're copying the pgtable should only because dst_vma has
> > -		 * uffd-wp enabled, do sanity check.
> > -		 */
> > -		WARN_ON_ONCE(!userfaultfd_wp(dst_vma));
> > -		set_pte_at(dst_mm, addr, dst_pte, pte);
> > +		if (userfaultfd_wp(dst_vma))
> > +			set_pte_at(dst_mm, addr, dst_pte, pte);
> >   		return 0;
> >   	}
> >   	if (!userfaultfd_wp(dst_vma))
> 
> Staring at the code first made me go "what about other PTE markers". I then
> looked into the discussion in patch #2. The fix as is is suboptimal, because
> it
> 
> 1) Removes the warning which is good, but
> 2) Silently drops swapin errors now
> 
> So it silently breaks something else temporarily ...
> 
> 
> I remember, that theoretically we could have multiple markers stored in a
> single PTE marker.
> 
> Wouldn't it be cleaner to be able to "clean" specific markers from a PTE
> marker without having to special case on each and everyone? I mean, only
> uffd-wp is really special such that it might disappear for the target.

Quotting the commit message in patch 2:

  Currently there is a priority difference between the uffd-wp bit and the
  swapin error entry, in which the swapin error always has higher priority
  (e.g. we don't need to wr-protect a swapin error pte marker).

  If there will be a 3rd bit introduced, we'll probably need to consider a
  more involved approach so we may need to start operate on the bits.
  Let's leave that for later.

I actually started the fix with something like that, but I noticed it's not
needed to add more code if there's no 3rd bit introduced so I dropped that.
I decided to go the simpler change approach and leave that for later.

> 
> Something like (pseudocode):
> 
> if (!userfaultfd_wp(dst_vma))
> 	pte_marker_clear_uff_wp(entry);
> if (!pte_marker_empty(entry)) {
> 	pte = make_pte_marker(pte_marker_get(entry));
> 	set_pte_at(dst_mm, addr, dst_pte, pte);
> }
> 
> Then this fix would be correct and backport-able even without #2. And it
> would work for new types of markers :)

When that comes, we may need one set_pte_marker_at() taking care of empty
pte markers, otherwise there can be a lot of such check.

> 
> 
> I'd prefer a fix that doesn't break something else temporarily, even if the
> stable backport might require 5 additional minutes to do. So squashing #2
> into #1 would also work.

The thing is whether do we care about someone: (1) explicitly checkout at
the commit of patch 1, then (2) runs the kernel, hit a swapnin error, (3)
fork(), and (4) access the swapin error page in the child.

To me I don't care even starting from (1).. because it really shouldn't
happen at all in any serious environment.

The other reason is these are indeed two issues to solve.  Even if by
accident we kept the swapin error in old code we'll probably dump an
warning which is not wanted either.  It's not something someone will really
get benefit from..

So like many other places, I don't have a strong opinion, but personally I
prefer the current approach.

Andrew, let me know if you want me to repost with a squashed version.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[David Hildenbrand]

>>
>> Wouldn't it be cleaner to be able to "clean" specific markers from a PTE
>> marker without having to special case on each and everyone? I mean, only
>> uffd-wp is really special such that it might disappear for the target.
> 
> Quotting the commit message in patch 2:
> 
>    Currently there is a priority difference between the uffd-wp bit and the
>    swapin error entry, in which the swapin error always has higher priority
>    (e.g. we don't need to wr-protect a swapin error pte marker).
> 
>    If there will be a 3rd bit introduced, we'll probably need to consider a
>    more involved approach so we may need to start operate on the bits.
>    Let's leave that for later.
> 
> I actually started the fix with something like that, but I noticed it's not
> needed to add more code if there's no 3rd bit introduced so I dropped that.
> I decided to go the simpler change approach and leave that for later.

Okay, makes sense.

> 
>>
>> Something like (pseudocode):
>>
>> if (!userfaultfd_wp(dst_vma))
>> 	pte_marker_clear_uff_wp(entry);
>> if (!pte_marker_empty(entry)) {
>> 	pte = make_pte_marker(pte_marker_get(entry));
>> 	set_pte_at(dst_mm, addr, dst_pte, pte);
>> }
>>
>> Then this fix would be correct and backport-able even without #2. And it
>> would work for new types of markers :)
> 
> When that comes, we may need one set_pte_marker_at() taking care of empty
> pte markers, otherwise there can be a lot of such check.

Right. In the future it might be cleaner.

> 
>>
>>
>> I'd prefer a fix that doesn't break something else temporarily, even if the
>> stable backport might require 5 additional minutes to do. So squashing #2
>> into #1 would also work.
> 
> The thing is whether do we care about someone: (1) explicitly checkout at
> the commit of patch 1, then (2) runs the kernel, hit a swapnin error, (3)
> fork(), and (4) access the swapin error page in the child.

I'm more concerned about backports, when one backports #1 but not #2. In 
theory, patch #2 fixes patch #1, because that introduced IMHO a real 
regression -- a possible memory corruption when discarding a hwpoison 
marker. Warnings are not nice but at least indicate that something needs 
a second look.

> 
> To me I don't care even starting from (1).. because it really shouldn't
> happen at all in any serious environment.
> 
> The other reason is these are indeed two issues to solve.  Even if by
> accident we kept the swapin error in old code we'll probably dump an
> warning which is not wanted either.  It's not something someone will really
> get benefit from..
> 
> So like many other places, I don't have a strong opinion, but personally I
> prefer the current approach.


Me neither, two patches just felt more complicated than it should be.

Anyhow, the final code change LGTM.

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker

[David Hildenbrand]

> diff --git a/mm/memory.c b/mm/memory.c
> index 032ef700c3e8..3e836fecd035 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -828,7 +828,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>   			return -EBUSY;
>   		return -ENOENT;
>   	} else if (is_pte_marker_entry(entry)) {
> -		if (userfaultfd_wp(dst_vma))
> +		if (is_swapin_error_entry(entry) || userfaultfd_wp(dst_vma))
>   			set_pte_at(dst_mm, addr, dst_pte, pte);

As discussed, it would be great if we could in the future avoid such 
allow lists and instead have default allow and only handle problematic 
cases. Maybe we can do that in the future with selective clearing of 
markers.

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker

[David Hildenbrand]

On 14.12.22 21:04, Peter Xu wrote:
> This patch should harden commit 15520a3f0469 ("mm: use pte markers for swap
> errors") on using pte markers for swapin errors on a few corner cases.
> 
> 1. Propagate swapin errors across fork()s: if there're swapin errors in
>     the parent mm, after fork()s the child should sigbus too when an error
>     page is accessed.
> 
> 2. Fix a rare condition race in pte_marker_clear() where a uffd-wp pte
>     marker can be quickly switched to a swapin error.
> 
> 3. Explicitly ignore swapin error pte markers in change_protection().
> 
> I mostly don't worry on (2) or (3) at all, but we should still have them.
> Case (1) is special because it can potentially cause silent data corrupt on
> child when parent has swapin error triggered with swapoff, but since swapin
> error is rare itself already it's probably not easy to trigger either.
> 
> Currently there is a priority difference between the uffd-wp bit and the
> swapin error entry, in which the swapin error always has higher
> priority (e.g. we don't need to wr-protect a swapin error pte marker).
> 
> If there will be a 3rd bit introduced, we'll probably need to consider a
> more involved approach so we may need to start operate on the bits.  Let's
> leave that for later.
> 
> This patch is tested with case (1) explicitly where we'll get corrupted
> data before in the child if there's existing swapin error pte markers, and
> after patch applied the child can be rightfully killed.
> 
> We don't need to copy stable for this one since 15520a3f0469 just landed as
> part of v6.2-rc1, only "Fixes" applied.
> 
> Fixes: 15520a3f0469 ("mm: use pte markers for swap errors")
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[Peter Xu]

On Fri, Dec 16, 2022 at 04:57:33PM +0100, David Hildenbrand wrote:
> I'm more concerned about backports, when one backports #1 but not #2. In
> theory, patch #2 fixes patch #1, because that introduced IMHO a real
> regression -- a possible memory corruption when discarding a hwpoison
> marker. Warnings are not nice but at least indicate that something needs a
> second look.

Note that backporting patch 1 only is exactly what I wanted to do here - it
means his/her tree should not have the swapin error pte markers at all.

The swapin error pte marker change only existed for a few days in Linus's
tree, so no one should be backporting patch 2.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[David Hildenbrand]

On 16.12.22 17:24, Peter Xu wrote:
> On Fri, Dec 16, 2022 at 04:57:33PM +0100, David Hildenbrand wrote:
>> I'm more concerned about backports, when one backports #1 but not #2. In
>> theory, patch #2 fixes patch #1, because that introduced IMHO a real
>> regression -- a possible memory corruption when discarding a hwpoison
>> marker. Warnings are not nice but at least indicate that something needs a
>> second look.
> 
> Note that backporting patch 1 only is exactly what I wanted to do here - it
> means his/her tree should not have the swapin error pte markers at all.
> 
> The swapin error pte marker change only existed for a few days in Linus's
> tree, so no one should be backporting patch 2.

Right, and these patches are supposed to land in 6.2 as well. Makes 
sense to me then.

Especially, the other parts in patch #2 are worth being in a separate patch.

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event

[Miaohe Lin]

On 2022/12/15 4:04, Peter Xu wrote:
> When fork(), dst_vma is not guaranteed to have VM_UFFD_WP even if src may
> have it and has pte marker installed.  The warning is improper along with
> the comment.  The right thing is to inherit the pte marker when needed, or
> keep the dst pte empty.
> 
> A vague guess is this happened by an accident when there's the prior patch
> to introduce src/dst vma into this helper during the uffd-wp feature got
> developed and I probably messed up in the rebase, since if we replace
> dst_vma with src_vma the warning & comment it all makes sense too.
> 
> Hugetlb did exactly the right here (copy_hugetlb_page_range()).  Fix the
> general path.
> 
> Reproducer:
> 
> https://github.com/xupengfe/syzkaller_logs/blob/main/221208_115556_copy_page_range/repro.c
> 
> Cc: <stable@vger.kernel.org> # 5.19+
> Fixes: c56d1b62cce8 ("mm/shmem: handle uffd-wp during fork()")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216808
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Looks good to me. Thanks.
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Thanks,
Miaohe Lin

Re: [PATCH 2/2] mm: Fix a few rare cases of using swapin error pte marker

[Miaohe Lin]

On 2022/12/15 4:04, Peter Xu wrote:
> This patch should harden commit 15520a3f0469 ("mm: use pte markers for swap
> errors") on using pte markers for swapin errors on a few corner cases.
> 
> 1. Propagate swapin errors across fork()s: if there're swapin errors in
>    the parent mm, after fork()s the child should sigbus too when an error
>    page is accessed.
> 
> 2. Fix a rare condition race in pte_marker_clear() where a uffd-wp pte
>    marker can be quickly switched to a swapin error.
> 
> 3. Explicitly ignore swapin error pte markers in change_protection().
> 
> I mostly don't worry on (2) or (3) at all, but we should still have them.
> Case (1) is special because it can potentially cause silent data corrupt on
> child when parent has swapin error triggered with swapoff, but since swapin
> error is rare itself already it's probably not easy to trigger either.
> 
> Currently there is a priority difference between the uffd-wp bit and the
> swapin error entry, in which the swapin error always has higher
> priority (e.g. we don't need to wr-protect a swapin error pte marker).
> 
> If there will be a 3rd bit introduced, we'll probably need to consider a
> more involved approach so we may need to start operate on the bits.  Let's
> leave that for later.
> 
> This patch is tested with case (1) explicitly where we'll get corrupted
> data before in the child if there's existing swapin error pte markers, and
> after patch applied the child can be rightfully killed.
> 
> We don't need to copy stable for this one since 15520a3f0469 just landed as
> part of v6.2-rc1, only "Fixes" applied.
> 
> Fixes: 15520a3f0469 ("mm: use pte markers for swap errors")
> Signed-off-by: Peter Xu <peterx@redhat.com>

Looks good to me. Thanks.
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Thanks,
Miaohe Lin