Re: [PATCH] mm/khugepaged: Check again on anon uffd-wp during isolation

[Yang Shi]

On Wed, Apr 5, 2023 at 11:10 AM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Wed, Apr 05, 2023 at 09:59:15AM -0700, Yang Shi wrote:
> > On Wed, Apr 5, 2023 at 8:51 AM Peter Xu <peterx@xxxxxxxxxx> wrote:
> > >
> > > Khugepaged collapse an anonymous thp in two rounds of scans.  The 2nd round
> > > done in __collapse_huge_page_isolate() after hpage_collapse_scan_pmd(),
> > > during which all the locks will be released temporarily. It means the
> > > pgtable can change during this phase before 2nd round starts.
> > >
> > > It's logically possible some ptes got wr-protected during this phase, and
> > > we can errornously collapse a thp without noticing some ptes are
> > > wr-protected by userfault.  e1e267c7928f wanted to avoid it but it only did
> > > that for the 1st phase, not the 2nd phase.
> > >
> > > Since __collapse_huge_page_isolate() happens after a round of small page
> > > swapins, we don't need to worry on any !present ptes - if it existed
> > > khugepaged will already bail out.  So we only need to check present ptes
> > > with uffd-wp bit set there.
> > >
> > > This is something I found only but never had a reproducer, I thought it was
> > > one caused a bug in Muhammad's recent pagemap new ioctl work, but it turns
> > > out it's not the cause of that but an userspace bug.  However this seems to
> > > still be a real bug even with a very small race window, still worth to have
> > > it fixed and copy stable.
> >
> > Yeah, I agree. But I got confused by userfaultfd_wp(vma) and
> > pte_uffd_wp(pte). If a vma is armed with uffd wp, shall we skip the
> > whole vma? If so, whether it is better to just check vma? We do
> > revalidate vma once reacquiring mmap_lock, so we should be able to
> > bail out earlier.
>
> Checking against VMA is safe too, the difference is current code still
> allows thp to be collapsed as long as none of the page is explicitly
> protected over the thp range, even if the range is registered with
> userfault-wp.  That's also what e1e267c7928f does.
>
> Here we have slightly different handling between anon / file thps (file
> thps checks against the vma flags), IMHO mostly because we don't scan
> pgtables when making decisions to collapse a shmem thp, so we made it
> simple by checking against vma flags.  We can make it the same as anon but
> it might be an overkill just to scan the entries for uffd-wp purpose.
>
> For anon we always scans the pgtable anyway so it's easier to make a more
> accurate decision.

Aha, I see. It does resolve my confusion. Thanks for elaborating.

The patch looks good to me. Reviewed-by: Yang Shi <shy828301@xxxxxxxxx>

>
> Thanks,
>
> --
> Peter Xu
>