Re: [PATCH] Fix incorrect flush address in direct page table reclaim

On 8/4/26 5:05 PM, David Hildenbrand (Arm) wrote:
> On 8/4/26 02:37, [email protected] wrote:
>> From: Andy Lutomirski <[email protected]>
>>
>> When zap_pte_range reclaims a page table, it does:
>>
>>      pte_free_tlb(tlb, pmd_pgtable(pmdval), addr);
>>
>> and this is unconditionally wrong: if this code executes, addr *always*
>> points one past the end of the range covered by the table.  The addr
>> parameter is used to flush the TLB (really the paging-structure-cache)
>> to drop references to the to-be-freed table, and any architecture that
>> cares about the parameter will flush the wrong address.  (But they'll
>> still free the correct page).
>>
>> I think it's worth contemplating why the kernel works at all.
> 
> Note that this only triggers when someone does e.g., a MADV_DONTNEED over
> a large enough range (covering at least a full PTE table).
> 
> So this isn't the ordinary munmap()/exit() page table reclaim code.
> 
> I'm still surprised that it took so long to show up; likely we need more
> targeted tests for PT_RECLAIM that

We backported the PT_RECLAIM to our internal tree a while ago (excluding
the rework patch being fixed here), and it has been running stably ever
since.

> 
>>
>> If we hit the offending line of code, we will first clear the PMD entry
>> (line 1954, zap_empty_pte_table), then we will issue pending flushes if
>> force_flush is set (tlb_flush_mmu_tlbonly(tlb)), then we will skip the
>> retry on line 1979 (phew!), and then we will do the offending
>> pte_free_tlb call.  *Or* we will clear the PMD entry immediately before
>> pte_free_tlb (line 1983, zap_pte_table_if_empty).
>>
>> If we have any pending flushes (i.e. we actually zapped any last-level
>> entries) at the time we clear the PMD entry, then the flush really ought
>> to flush all references to the table (Linus certainly seems to think it
>> will on all architectures [0]).
>>
>> The condition under which we have no accumulated flushes at the time of
>> the clear is very complex (the whole zap_pte_range function has absurdly
>> complex control flow).  If we do hit the bad case, then we will end up
>> clearing the PMD entry after the last time the range is flushed, and any
>> CPU is free to cache a reference to the (empty) page table.  If this
>> happens due to an ordinary read or write, it would segfault, so it would
>> be rare.  But the cache could be speculatively filled as well.  Then
>> we'll flush the wrong address and then free and possibly reuse the
>> table.
>>
>> On x86, even flushing the wrong address works on non-KPTI Intel systems
>> because INVLPG flushes *all* paging-structure-caches, not just the ones
>> for the target address.  But INVPCID does not, and flush_tlb_one_user
>> will use INVPCID if it's available.  And then we're toast.  AMD systems
>> are more susceptible: we set the EFER.TCE bit, which makes even INVLPG
>> only flush the target address.
>>
>> P.S. IMO zap_pte_range is a mess.  The control flow is excessively
>> complex.  The direct_reclaim variable itself has a confused meaning --
>> for the first part of the function it means, approximately, "we should
>> free the table if can_reclaim_pt".  But, later on, it means "we ALREADY
>> reclaimed the table".  And the goto retry on line 1979 is IMO just
>> asking for trouble if the condition ever changes such that it might
>> happen after clearing the PMD.
> 
> There is certainly room for improvement :)
> 
> This should, however, not be part of the patch description (best be had below the ---)
> 
>>
>> I think this might fix an issue in ripgrep reported here:
>> https://github.com/BurntSushi/ripgrep/issues/3494
>>
>> [0] https://lore.kernel.org/all/CA+55aFzBggoXtNXQeng5d_mRoDnaMBE5Y+URs+PHR67nUpMtaw@mail.gmail.com/T/#u
>>
>> Fixes: 4c640eb4181c ("mm: move pte table reclaim code to memory.c")
>> Cc: David Hildenbrand (Red Hat) <[email protected]>
>> Cc: Qi Zheng <[email protected]>
>> Cc: Liam Howlett <[email protected]>
>> Cc: "Liam R. Howlett" <[email protected]>
>> Cc: Lorenzo Stoakes <[email protected]>
>> Cc: Michal Hocko <[email protected]>
>> Cc: Mike Rapoport <[email protected]>
>> Cc: Suren Baghdasaryan <[email protected]>
>> Cc: Vlastimil Babka <[email protected]>
>> Cc: Andrew Morton <[email protected]>.org>
>> Cc: [email protected]
>> Signed-off-by: Andy Lutomirski <[email protected]>
>> ---
>>   mm/memory.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>

Acked-by: Qi Zheng <[email protected]>

Thanks,
Qi