Re: [PATCH 1/1] mm: Fix folio_pte_batch() overcount with zero PTEs
From: David Hildenbrand
Date: Fri May 02 2025 - 05:42:35 EST
On 02.05.25 09:37, David Hildenbrand wrote:
On 01.05.25 09:45, Petr Vaněk wrote:
On Wed, Apr 30, 2025 at 11:25:56PM +0200, David Hildenbrand wrote:
On 30.04.25 18:00, Petr Vaněk wrote:
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@xxxxxxxxxxxxxxx
Signed-off-by: Petr Vaněk <arkamar@xxxxxxxx>
---
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.
Cool!
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@xxxxxxxx>
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@xxxxxxxx>
---
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)
{
That might do as well.
I assume the following would also work? (removing the early ==1 check)
Yes, it also works in my case and the removal makes sense to me.
It has the general benefit of removing the pte_pfn() call from the
loop body, which is why I like that fix. (almost looks like a cleanup)
Indeed, it looks like a cleanup to me as well :)
Okay, let me polish the patch up and send it out later.
Actually, can you polish it up and send it out? I think we need a better
description on how exactly that problems happens, in particular
involving pte_none(). So stuff from the cover letter should probably be
living in here.
Feel free to add my
Co-developed-by: David Hildenbrand <david@xxxxxxxxxx>
Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
and stay first author.
Thanks!
--
Cheers,
David / dhildenb