Date:   Fri, 22 Sep 2023 10:09:08 -0700
From:   Axel Rasmussen <axelrasmussen@...gle.com>
To:     Ryan Roberts <ryan.roberts@....com>
Cc:     Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
        Helge Deller <deller@....de>,
        Nicholas Piggin <npiggin@...il.com>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Gerald Schaefer <gerald.schaefer@...ux.ibm.com>,
        "David S. Miller" <davem@...emloft.net>,
        Arnd Bergmann <arnd@...db.de>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Muchun Song <muchun.song@...ux.dev>,
        SeongJae Park <sj@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Uladzislau Rezki <urezki@...il.com>,
        Christoph Hellwig <hch@...radead.org>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Peter Xu <peterx@...hat.com>,
        Qi Zheng <zhengqi.arch@...edance.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-parisc@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-riscv@...ts.infradead.org, linux-s390@...r.kernel.org,
        sparclinux@...r.kernel.org, linux-mm@...ck.org,
        stable@...r.kernel.org
Subject: Re: [PATCH v2 2/2] arm64: hugetlb: Fix set_huge_pte_at() to work with
 all swap entries

Looks correct to me - thanks for the fix!

Reviewed-by: Axel Rasmussen <axelrasmussen@...gle.com>

On Fri, Sep 22, 2023 at 9:41 AM Ryan Roberts <ryan.roberts@....com> wrote:
>
> On 22/09/2023 17:14, Will Deacon wrote:
> > On Fri, Sep 22, 2023 at 12:58:04PM +0100, Ryan Roberts wrote:
> >> When called with a swap entry that does not embed a PFN (e.g.
> >> PTE_MARKER_POISONED or PTE_MARKER_UFFD_WP), the previous implementation
> >> of set_huge_pte_at() would either cause a BUG() to fire (if
> >> CONFIG_DEBUG_VM is enabled) or cause a dereference of an invalid address
> >> and subsequent panic.
> >>
> >> arm64's huge pte implementation supports multiple huge page sizes, some
> >> of which are implemented in the page table with multiple contiguous
> >> entries. So set_huge_pte_at() needs to work out how big the logical pte
> >> is, so that it can also work out how many physical ptes (or pmds) need
> >> to be written. It previously did this by grabbing the folio out of the
> >> pte and querying its size.
> >>
> >> However, there are cases when the pte being set is actually a swap
> >> entry. But this also used to work fine, because for huge ptes, we only
> >> ever saw migration entries and hwpoison entries. And both of these types
> >> of swap entries have a PFN embedded, so the code would grab that and
> >> everything still worked out.
> >>
> >> But over time, more calls to set_huge_pte_at() have been added that set
> >> swap entry types that do not embed a PFN. And this causes the code to go
> >> bang. The triggering case is for the uffd poison test, commit
> >> 99aa77215ad0 ("selftests/mm: add uffd unit test for UFFDIO_POISON"),
> >> which causes a PTE_MARKER_POISONED swap entry to be set, coutesey of
> >> commit 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for
> >> hugetlbfs") - added in v6.5-rc7. Although review shows that there are
> >> other call sites that set PTE_MARKER_UFFD_WP (which also has no PFN),
> >> these don't trigger on arm64 because arm64 doesn't support UFFD WP.
> >>
> >> Arguably, the root cause is really due to commit 18f3962953e4 ("mm:
> >> hugetlb: kill set_huge_swap_pte_at()"), which aimed to simplify the
> >> interface to the core code by removing set_huge_swap_pte_at() (which
> >> took a page size parameter) and replacing it with calls to
> >> set_huge_pte_at() where the size was inferred from the folio, as
> >> descibed above. While that commit didn't break anything at the time, it
> >> did break the interface because it couldn't handle swap entries without
> >> PFNs. And since then new callers have come along which rely on this
> >> working. But given the brokeness is only observable after commit
> >> 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for hugetlbfs"),
> >> that one gets the Fixes tag.
> >>
> >> Now that we have modified the set_huge_pte_at() interface to pass the
> >> huge page size in the previous patch, we can trivially fix this issue.
> >>
> >> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
> >> Fixes: 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for hugetlbfs")
> >> Cc: <stable@...r.kernel.org> # 6.5+
> >> ---
> >>  arch/arm64/mm/hugetlbpage.c | 17 +++--------------
> >>  1 file changed, 3 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> >> index a7f8c8db3425..13fd592228b1 100644
> >> --- a/arch/arm64/mm/hugetlbpage.c
> >> +++ b/arch/arm64/mm/hugetlbpage.c
> >> @@ -241,13 +241,6 @@ static void clear_flush(struct mm_struct *mm,
> >>      flush_tlb_range(&vma, saddr, addr);
> >>  }
> >>
> >> -static inline struct folio *hugetlb_swap_entry_to_folio(swp_entry_t entry)
> >> -{
> >> -    VM_BUG_ON(!is_migration_entry(entry) && !is_hwpoison_entry(entry));
> >> -
> >> -    return page_folio(pfn_to_page(swp_offset_pfn(entry)));
> >> -}
> >> -
> >>  void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
> >>                          pte_t *ptep, pte_t pte, unsigned long sz)
> >>  {
> >> @@ -257,13 +250,10 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
> >>      unsigned long pfn, dpfn;
> >>      pgprot_t hugeprot;
> >>
> >> -    if (!pte_present(pte)) {
> >> -            struct folio *folio;
> >> -
> >> -            folio = hugetlb_swap_entry_to_folio(pte_to_swp_entry(pte));
> >> -            ncontig = num_contig_ptes(folio_size(folio), &pgsize);
> >> +    ncontig = num_contig_ptes(sz, &pgsize);
> >>
> >> -            for (i = 0; i < ncontig; i++, ptep++)
> >> +    if (!pte_present(pte)) {
> >> +            for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
> >>                      set_pte_at(mm, addr, ptep, pte);
> >
> > Our set_pte_at() doesn't use 'addr' for anything and the old code didn't
> > even bother to increment it here! I'm fine adding that, but it feels
> > unrelated to the issue which this patch is actually fixing.
>
> True. I agree its not strictly necessary and will presumably be optimized out.
> But I'm not sure that having knowledge that the implementation doesn't use it is
> a good reason not to call the interface correctly. I'll leave it as I've done it
> if that's ok.
>
> >
> > Either way:
> >
> > Acked-by: Will Deacon <will@...nel.org>
>
> Thanks!
>
> >
> > Will
>

Powered by blists - more mailing lists