mips _pmap_unwire_ptp races MipsDoTLBMiss

[freebsdphab-AX9_cmx.ietfng.org]

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.

[kib]

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]

Now without D24523 included

[markj]

sys/mips/mips/pmap.c

1751 ↗

(On Diff #70851)

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

1858 ↗

(On Diff #70851)

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

1861 ↗

(On Diff #70851)

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

1990 ↗

(On Diff #70851)

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]

sys/mips/mips/pmap.c

1858 ↗

(On Diff #70851)

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.

[freebsdphab-AX9_cmx.ietfng.org]

Revised version as per markj's feedback

[markj]

This looks reasonable to me aside from the style nits.

sys/mips/mips/pmap.c

1513 ↗

(On Diff #70902)

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

1922 ↗

(On Diff #70902)

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

1858 ↗

(On Diff #70851)

I see, thanks.

[kib]

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]

More respose to markj feedback