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

[David Hildenbrand <david@redhat.com>]

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@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.
>>>
>>> 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@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)
>>>>     {
>>>
>>> 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@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>

and stay first author.

Thanks!

-- 
Cheers,

David / dhildenb