Peter Xu <peterx@xxxxxxxxxx> writes:
> On Thu, Dec 15, 2022 at 03:12:13PM +0800, Huang, Ying wrote:
>> Peter Xu <peterx@xxxxxxxxxx> 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@xxxxxxxxxx>
>> > ---
>> > 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
