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

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.

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.

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(-)

diff --git a/mm/memory.c b/mm/memory.c
index 86a973119bd4..13b70861c8a3 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1981,7 +1981,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 
 	if (can_reclaim_pt) {
 		if (direct_reclaim || zap_pte_table_if_empty(mm, pmd, start, &pmdval)) {
-			pte_free_tlb(tlb, pmd_pgtable(pmdval), addr);
+			pte_free_tlb(tlb, pmd_pgtable(pmdval), start);
 			mm_dec_nr_ptes(mm);
 		}
 	}
--