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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHbLzkqzYxCrJdH8f7OY5x9-mngK+xKJUZ+HCB9V-O+yQqKE4w@mail.gmail.com>
Date:   Wed, 5 Apr 2023 11:25:24 -0700
From:   Yang Shi <shy828301@...il.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Nadav Amit <nadav.amit@...il.com>,
        David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        linux-stable <stable@...r.kernel.org>
Subject: Re: [PATCH] mm/khugepaged: Check again on anon uffd-wp during isolation

On Wed, Apr 5, 2023 at 11:10 AM Peter Xu <peterx@...hat.com> 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@...hat.com> 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@...il.com>

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