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

["Huang, Ying" <ying.huang@intel.com>]

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