Re: [PATCH 1/1] mm: Fix folio_pte_batch() overcount with zero PTEs

Petr Vaněk
2025-04-30T16:00:59Z

On Wed, Apr 30, 2025 at 04:37:21PM +0200, David Hildenbrand wrote:
> On 30.04.25 13:52, Petr Vaněk wrote:
> > On Tue, Apr 29, 2025 at 08:56:03PM +0200, David Hildenbrand wrote:
> >> On 29.04.25 20:33, Petr Vaněk wrote:
> >>> On Tue, Apr 29, 2025 at 05:45:53PM +0200, David Hildenbrand wrote:
> >>>> On 29.04.25 16:52, David Hildenbrand wrote:
> >>>>> On 29.04.25 16:45, Petr Vaněk wrote:
> >>>>>> On Tue, Apr 29, 2025 at 04:29:30PM +0200, David Hildenbrand wrote:
> >>>>>>> On 29.04.25 16:22, Petr Vaněk wrote:
> >>>>>>>> folio_pte_batch() could overcount the number of contiguous PTEs when
> >>>>>>>> pte_advance_pfn() returns a zero-valued PTE and the following PTE in
> >>>>>>>> memory also happens to be zero. The loop doesn't break in such a case
> >>>>>>>> because pte_same() returns true, and the batch size is advanced by one
> >>>>>>>> more than it should be.
> >>>>>>>>
> >>>>>>>> To fix this, bail out early if a non-present PTE is encountered,
> >>>>>>>> preventing the invalid comparison.
> >>>>>>>>
> >>>>>>>> This issue started to appear after commit 10ebac4f95e7 ("mm/memory:
> >>>>>>>> optimize unmap/zap with PTE-mapped THP") and was discovered via git
> >>>>>>>> bisect.
> >>>>>>>>
> >>>>>>>> Fixes: 10ebac4f95e7 ("mm/memory: optimize unmap/zap with PTE-mapped THP")
> >>>>>>>> Cc: stable@vger.kernel.org
> >>>>>>>> Signed-off-by: Petr Vaněk <arkamar@atlas.cz>
> >>>>>>>> ---
> >>>>>>>> mm/internal.h | 2 ++
> >>>>>>>> 1 file changed, 2 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/mm/internal.h b/mm/internal.h
> >>>>>>>> index e9695baa5922..c181fe2bac9d 100644
> >>>>>>>> --- a/mm/internal.h
> >>>>>>>> +++ b/mm/internal.h
> >>>>>>>> @@ -279,6 +279,8 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> >>>>>>>> dirty = !!pte_dirty(pte);
> >>>>>>>> pte = __pte_batch_clear_ignored(pte, flags);
> >>>>>>>>
> >>>>>>>> + if (!pte_present(pte))
> >>>>>>>> + break;
> >>>>>>>> if (!pte_same(pte, expected_pte))
> >>>>>>>> break;
> >>>>>>>
> >>>>>>> How could pte_same() suddenly match on a present and non-present PTE.
> >>>>>>
> >>>>>> In the problematic case pte.pte == 0 and expected_pte.pte == 0 as well.
> >>>>>> pte_same() returns a.pte == b.pte -> 0 == 0. Both are non-present PTEs.
> >>>>>
> >>>>> Observe that folio_pte_batch() was called *with a present pte*.
> >>>>>
> >>>>> do_zap_pte_range()
> >>>>> if (pte_present(ptent))
> >>>>> zap_present_ptes()
> >>>>> folio_pte_batch()
> >>>>>
> >>>>> How can we end up with an expected_pte that is !present, if it is based
> >>>>> on the provided pte that *is present* and we only used pte_advance_pfn()
> >>>>> to advance the pfn?
> >>>>
> >>>> I've been staring at the code for too long and don't see the issue.
> >>>>
> >>>> We even have
> >>>>
> >>>> VM_WARN_ON_FOLIO(!pte_present(pte), folio);
> >>>>
> >>>> So the initial pteval we got is present.
> >>>>
> >>>> I don't see how
> >>>>
> >>>> nr = pte_batch_hint(start_ptep, pte);
> >>>> expected_pte = __pte_batch_clear_ignored(pte_advance_pfn(pte, nr), flags);
> >>>>
> >>>> would suddenly result in !pte_present(expected_pte).
> >>>
> >>> The issue is not happening in __pte_batch_clear_ignored but later in
> >>> following line:
> >>>
> >>> expected_pte = pte_advance_pfn(expected_pte, nr);
> >>>
> >>> The issue seems to be in __pte function which converts PTE value to
> >>> pte_t in pte_advance_pfn, because warnings disappears when I change the
> >>> line to
> >>>
> >>> expected_pte = (pte_t){ .pte = pte_val(expected_pte) + (nr << PFN_PTE_SHIFT) };
> >>>
> >>> The kernel probably uses __pte function from
> >>> arch/x86/include/asm/paravirt.h because it is configured with
> >>> CONFIG_PARAVIRT=y:
> >>>
> >>> static inline pte_t __pte(pteval_t val)
> >>> {
> >>> return (pte_t) { PVOP_ALT_CALLEE1(pteval_t, mmu.make_pte, val,
> >>> "mov %%rdi, %%rax", ALT_NOT_XEN) };
> >>> }
> >>>
> >>> I guess it might cause this weird magic, but I need more time to
> >>> understand what it does :)
> >
> > I understand it slightly more. __pte() uses xen_make_pte(), which calls
> > pte_pfn_to_mfn(), however, mfn for this pfn contains INVALID_P2M_ENTRY
> > value, therefore the pte_pfn_to_mfn() returns 0, see [1].
> >
> > I guess that the mfn was invalidated by xen-balloon driver?
> >
> > [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/xen/mmu_pv.c?h=v6.15-rc4#n408
> >
> >> What XEN does with basic primitives that convert between pteval and
> >> pte_t is beyond horrible.
> >>
> >> How come set_ptes() that uses pte_next_pfn()->pte_advance_pfn() does not
> >> run into this?
> >
> > I don't know, but I guess it is somehow related to pfn->mfn translation.
> >
> >> Is it only a problem if we exceed a certain pfn?
> >
> > No, it is a problem if the corresponding mft to given pfn is invalid.
> >
> > I am not sure if my original patch is a good fix.
>
> No :)
>
> Maybe it would be
> > better to have some sort of native_pte_advance_pfn() which will use
> > native_make_pte() rather than __pte(). Or do you think the issue is in
> > Xen part?
>
> I think what's happening is that -- under XEN only -- we might get garbage when
> calling pte_advance_pfn() and the next PFN would no longer fall into the folio. And
> the current code cannot deal with that XEN garbage.
>
> But still not 100% sure.
>
> The following is completely untested, could you give that a try?

Yes, it solves the issue for me.

However, maybe it would be better to solve it with the following patch.
The pte_pfn_to_mfn() actually returns the same value for non-present
PTEs. I suggest to return original PTE if the mfn is INVALID_P2M_ENTRY,
rather than empty non-present PTE, but the _PAGE_PRESENT bit will be set
to zero. Thus, we will not loose information about original pfn but it
will be clear that the page is not present.

From e84781f9ec4fb7275d5e7629cf7e222466caf759 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Van=C4=9Bk?= <arkamar@atlas.cz>
Date: Wed, 30 Apr 2025 17:08:41 +0200
Subject: [PATCH] x86/mm: Reset pte _PAGE_PRESENT bit for invalid mft
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Petr Vaněk <arkamar@atlas.cz>
---
arch/x86/xen/mmu_pv.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 38971c6dcd4b..92a6a9af0c65 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -392,28 +392,25 @@ static pteval_t pte_mfn_to_pfn(pteval_t val)
static pteval_t pte_pfn_to_mfn(pteval_t val)
{
if (val & _PAGE_PRESENT) {
unsigned long pfn = (val & PTE_PFN_MASK) >> PAGE_SHIFT;
pteval_t flags = val & PTE_FLAGS_MASK;
unsigned long mfn;

mfn = __pfn_to_mfn(pfn);

/*
- * If there's no mfn for the pfn, then just create an
- * empty non-present pte. Unfortunately this loses
- * information about the original pfn, so
- * pte_mfn_to_pfn is asymmetric.
+ * If there's no mfn for the pfn, then just reset present pte bit.
*/
if (unlikely(mfn == INVALID_P2M_ENTRY)) {
- mfn = 0;
- flags = 0;
+ mfn = pfn;
+ flags &= ~_PAGE_PRESENT;
} else
mfn &= ~(FOREIGN_FRAME_BIT | IDENTITY_FRAME_BIT);
val = ((pteval_t)mfn << PAGE_SHIFT) | flags;
}

return val;
}

__visible pteval_t xen_pte_val(pte_t pte)
{
--
2.48.1