Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230405155120.3608140-1-peterx@redhat.com> <CAHbLzkqKE-TE9Od1E=PQDGuhoR+r-TOz4LP8WQgucm_6ZVYTRA@mail.gmail.com>
 <ZC25d12d0sc1L7tS@x1n>
In-Reply-To: <ZC25d12d0sc1L7tS@x1n>
From:   Yang Shi <shy828301@gmail.com>
Date:   Wed, 5 Apr 2023 11:25:24 -0700
Message-ID: <CAHbLzkqzYxCrJdH8f7OY5x9-mngK+xKJUZ+HCB9V-O+yQqKE4w@mail.gmail.com>
Subject: Re: [PATCH] mm/khugepaged: Check again on anon uffd-wp during isolation
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Axel Rasmussen <axelrasmussen@google.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        David Hildenbrand <david@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        linux-stable <stable@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Apr 5, 2023 at 11:10=E2=80=AFAM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Apr 05, 2023 at 09:59:15AM -0700, Yang Shi wrote:
> > On Wed, Apr 5, 2023 at 8:51=E2=80=AFAM Peter Xu <peterx@redhat.com> wro=
te:
> > >
> > > 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 on=
ly did
> > > that for the 1st phase, not the 2nd phase.
> > >
> > > Since __collapse_huge_page_isolate() happens after a round of small p=
age
> > > 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 p=
tes
> > > 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 se=
ems to
> > > still be a real bug even with a very small race window, still worth t=
o 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 bu=
t
> 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@gmail.com>

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