[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0e029877-4ca9-40f0-933f-6d0779c95d72@redhat.com>
Date: Fri, 2 May 2025 09:37:44 +0200
From: David Hildenbrand <david@...hat.com>
To: Petr Vaněk <arkamar@...as.cz>
Cc: linux-kernel@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
Ryan Roberts <ryan.roberts@....com>, linux-mm@...ck.org,
stable@...r.kernel.org
Subject: Re: [PATCH 1/1] mm: Fix folio_pte_batch() overcount with zero PTEs
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@...r.kernel.org
>>>>>>>>>>>> Signed-off-by: Petr Vaněk <arkamar@...as.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.
>>
>> 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@...as.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@...as.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)
>>> {
>>
>> 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.
>
> I am still considering if it would make sense to send both patches, I am
> not sure if reseting _PAGE_PRESENT flag is enough, because of swapping
> or other areas which I am not aware of.
The problem I'm having with pte_mfn_to_pfn() updates is that I don't
understand what the expected output is even supposed to be. You should
probably ask XEN folks (I realized that they are not CCed).
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists