Re: [PATCH 05/28] mm, swap: sanitize swap cache lookup convention

[Kairui Song <ryncsn@gmail.com>]

On Mon, May 19, 2025 at 12:38 PM Barry Song <21cnbao@gmail.com> wrote:
>
> > From: Kairui Song <kasong@tencent.com>
>
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index e5a0db7f3331..5b4f01aecf35 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> >                               goto retry;
> >                       }
> >               }
> > +             if (!folio_swap_contains(src_folio, entry)) {
> > +                     err = -EBUSY;
> > +                     goto out;
> > +             }
>
> It seems we don't need this. In move_swap_pte(), we have been checking pte pages
> are stable:
>
>         if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
>                                  dst_pmd, dst_pmdval)) {
>                 double_pt_unlock(dst_ptl, src_ptl);
>                 return -EAGAIN;
>         }

The tricky part is when swap_cache_get_folio returns the folio, both
folio and ptes are unlocked. So is it possible that someone else
swapped in the entries, then swapped them out again using the same
entries?

The folio will be different here but PTEs are still the same value to
they will pass the is_pte_pages_stable check, we previously saw
similar races with anon fault or shmem. I think more strict checking
won't hurt here.

>
> Also, -EBUSY is somehow incorrect error code.

Yes, thanks, I'll use EAGAIN here just like move_swap_pte.


>
> >               err = move_swap_pte(mm, dst_vma, dst_addr, src_addr, dst_pte, src_pte,
> >                               orig_dst_pte, orig_src_pte, dst_pmd, dst_pmdval,
> >                               dst_ptl, src_ptl, src_folio);
> >
>
> Thanks
> Barry