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

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

[Andy Lutomirski]

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.

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

--Andy

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

[David Hildenbrand (Arm)]

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

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

[David Hildenbrand (Arm)]

On 8/3/26 10:31, David Hildenbrand (Arm) wrote:
> 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.

Stupid me, we have "start" right there that we can just use.

-- 
Cheers,

David

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

[David Hildenbrand (Arm)]

>> 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.

We should end up in tlb_flush_mmu_tlbonly(tlb) -> tlb_flush(tlb) ->
flush_tlb_mm_range() with the wrong range, so yes, please send a
fix asap, thanks!

-- 
Cheers,

David