Re: TLB free bug in 4c640eb4181c ("mm: move pte table reclaim code to memory.c")

On 8/1/26 16:00, Andy Lutomirski wrote:
> Hi all-
> 
> I saw a fun bug report in ripgrep and a studious but pretty bad
> AI-generated analysis, and I peeked at the actual code.  I'm rather
> suspicious of this:
> 
>     if (can_reclaim_pt) {
>         if (direct_reclaim || zap_pte_table_if_empty(mm, pmd, start, &pmdval)) {
>             pte_free_tlb(tlb, pmd_pgtable(pmdval), addr);  <-- what is
> addr here?
>             mm_dec_nr_ptes(mm);
>         }
>     }
> 
> It looks to me (and an LLM -- I can *never* remember what all the
> tlb_xyz functions do, so I asked an LLM for a summary), like addr is
> not guaranteed to point at the range being zapped, because the do loop
> above may increment it right past the end.

It will actually always point at the end, whereby the end is at
the start of the next page table :/

pte_table_reclaim_possible() makes sure that we reclaim only when covering a full
page table.

Subtracting "PMD_SIZE" from start would ... or just remembering the original start.

> 
> On x86, by my reading of the extremely vague text in the SDM volume 3,
> INVLPG doesn't care in the sense that (most of?) the code paths
> reaching this have already validated that there weren't any live PTEs
> in the page and INVLPG promises to flush higher level paging structure
> caches for *all* addresses.  But INVPCID makes no such promise that I
> can see.
> 
> So it kind of seems like this code might end up flushing the wrong
> paging structure caches, leaving all *actual mappings* in the TLB
> valid but also leaving a cached reference to the to-be-freed
> pagetable.  Which would be quite bad.

:/

> 
> Original report here:
> 
> https://github.com/BurntSushi/ripgrep/issues/3494
> 
> I haven't sent a patch because I *still* don't pretend to have
> followed what all the tlb_ functions promise to do, and I don't want
> to submit a subtle patch based just on an LLM telling me what *it*
> thinks those functions do.  I could trace though all this mess
> manually, but I bet that one of you actually remembers :)

The memory.c code is definitely broken. I guess the real question is,
what the effect of that is.

-- 
Cheers,

David