Page MenuHomeFreeBSD

riscv: Fix another race in pmap_pinit()
ClosedPublic

Authored by markj on Feb 14 2022, 3:06 PM.

Details

Summary

Commit c862d5f2a789 ("riscv: Fix a race in pmap_pinit()") did not really
fix the race. Alan writes,

Suppose that N entries in the L1 tables are in use, and we are in the
middle of the memcpy().  Specifically, we have read the zero-filled
(N+1)st entry from the kernel L1 table.  Then, we are preempted.  Now,
another core/thread does pmap_growkernel(), which fills the (N+1)st
entry.  Finally, we return to the original core/thread, and overwrite
the valid entry with the zero that we earlier read.

Try to fix the race properly, by copying kernel L1 entries while holding
the allpmaps lock. To avoid doing unnecessary work while holding this
global lock, copy only the entries that we expect to be valid. We could
go further: since kernel map L1 entries are never invalidated since DMAP
L1 entries are currently never demoted, we could just count the number
of valid entries with the lock held and copy them after dropping the
lock. That seems a bit fragile to me, though: if the second assumption
stops being true, then we'll have a subtle bug and will have to go back
to copying under the lock. Also note that in (soon to be added) SV48
mode, there is no need for the allpmaps list at all.

Reported by: alc, jrtc27

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Feb 14 2022, 3:06 PM
jrtc27 added inline comments.
sys/riscv/riscv/pmap.c
1239

The switch from memcpy seems like a separate change?..

sys/riscv/riscv/pmap.c
1239

Yes, I'm effectively making two changes here: fixing the race, and trying to reduce the amount of work done under the allpmaps lock. I can commit the changes separately; for review purposes it seemed reasonable to squash the two.

arichardson added inline comments.
sys/riscv/riscv/pmap.c
1239

Should be allowed now and avoids an extra line at the start?

markj marked an inline comment as done.

Shrink i's scope.

This revision is now accepted and ready to land.Feb 15 2022, 8:07 AM
This revision was automatically updated to reflect the committed changes.