Page MenuHomeFreeBSD

mips _pmap_unwire_ptp races MipsDoTLBMiss
ClosedPublic

Authored by freebsdphab-AX9_cmx.ietfng.org on Apr 18 2020, 11:11 PM.
Tags
Referenced Files
Unknown Object (File)
Fri, Nov 15, 7:58 AM
Unknown Object (File)
Thu, Nov 14, 8:54 PM
Unknown Object (File)
Thu, Nov 14, 3:58 PM
Unknown Object (File)
Thu, Nov 14, 3:57 PM
Unknown Object (File)
Wed, Nov 13, 10:36 AM
Unknown Object (File)
Wed, Nov 13, 10:16 AM
Unknown Object (File)
Wed, Nov 13, 3:10 AM
Unknown Object (File)
Wed, Nov 13, 2:38 AM
Subscribers

Details

Summary

MipsDoTLBMiss will load a segmap entry or pde, check that it isn't zero,
and then chase that pointer to a physical page. If that page has been freed
in the interim, it will read garbage and go on to populate the TLB with it.

This can happen because pmap_unwire_ptp zeros out the pde and
vm_page_free_zero()s the ptp (or, recursively, zeros out the segmap
entry and vm_page_free_zero()s the pdp) without interlocking against
MipsDoTLBMiss. The pmap is locked, and pvh_global_lock may or may not
be held, but this is not enough. Solve this issue by inserting TLB
shootdowns within _pmap_unwire_ptp; as MipsDoTLBMiss runs with IRQs
deferred, the IPIs involved in TLB shootdown are sufficient to ensure
that MipsDoTLBMiss sees either a zero segmap entry / pde or a non-zero
entry and the pointed-to page still not freed.

Thanks to Konstantin Belousov and Mark Johnston for confirming the bug.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30598
Build 28338: arc lint + arc unit

Event Timeline

I believe it is better to split the review into two. The pmap_emulate_modified() looks good to me, and I do not see why the other part should block this one.

freebsdphab-AX9_cmx.ietfng.org retitled this revision from Fix two races in the MIPS pmap to mips _pmap_unwire_ptp races MipsDoTLBMiss.Apr 21 2020, 6:40 PM
freebsdphab-AX9_cmx.ietfng.org edited the summary of this revision. (Show Details)
sys/mips/mips/pmap.c
1751

Should it be "segmap entry, PD, and PT"?

1858

Couldn't this block of changes be made much smaller by simply assigning va = va_next before breaking from the loop here?

1861

style(9) encourages having all local variable declarations at the beginning of the function.

1990

I would suggest going through all of the pmap_unuse_pt() calls in this file and adding an explicit (void) cast to calls where we don't care about the return value.

freebsdphab-AX9_cmx.ietfng.org added inline comments.
sys/mips/mips/pmap.c
1858

I won't claim that my changes are the most elegant, so there's almost surely a better way of achieving what I wanted, but...

The proposed change attempts to defer TLB shootdowns until it is certain that it must issue one, whereas the existing code may fire off several for narrow slices of the PT being traversed (repeated triggering of line 1850) and then may discover that it did one for the whole PT as part of pte_remove_page (well, pmap_unwire_pt) anyway.

That said, I seem to have considered the end of the page being a series of !PTE_V entries but neglected the possibility that the page begins with !PTE_V entries. I've amended the patch.

This looks reasonable to me aside from the style nits.

sys/mips/mips/pmap.c
1513

There should be no space between the cast and function name.

1858

I see, thanks.

1908

I'd find the code slightly clearer if PAGE_SIZE were added here instead of at the pmap_invalidate_range() call.

This revision is now accepted and ready to land.Apr 23 2020, 2:20 PM

Note that for instance x86 has similar issue, because hardware is free to speculatively walk page tables and load any valid PTE or intermediate paging structure. There all page table entries clearing functions take the free list pointer, where the freed page table is put. Only after the caller did IPI to invalidate TLB for cleared PTE, the pages from the list can be freed.

freebsdphab-AX9_cmx.ietfng.org marked 2 inline comments as done.

More respose to markj feedback

This revision now requires review to proceed.Apr 24 2020, 2:40 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 24 2020, 9:21 PM
This revision was automatically updated to reflect the committed changes.