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

On Mon 03-08-26 17:37:08, [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.
> 
> 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]>

Very well spotted. It was really easy to miss that change during the
rework.

Acked-by: Michal Hocko <[email protected]>

Thanks!

> ---
>  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);
>  		}
>  	}
> -- 

-- 
Michal Hocko
SUSE Labs