Page MenuHomeFreeBSD

Fix some issues with pmap_protect().
ClosedPublic

Authored by markj on Jan 2 2019, 6:17 PM.

Details

Summary

Note that pmap_protect() is only ever used to remove permissions from
existing mappings.

  • When clearing PTE_W, use an atomic update to the PTE; otherwise we may clobber updates done by the hardware.
  • Handle VM_PROT_EXECUTE.
  • Clear PTE_D and mark the page dirty when removing write permission from a mapping.

Diff Detail

Repository
rS 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 created this revision.Jan 2 2019, 6:17 PM
kib added inline comments.Jan 2 2019, 9:52 PM
sys/riscv/riscv/pmap.c
1918 ↗(On Diff #52487)

I see that you changed this to atomic clear, so my comment from other review is not relevant.

But what if PTE_D bit set after the pmap_load() but before atomic_clear() ? This seems to be legit and would cause missing dirty set and consequent corruption of the user data. Shouldn't you use atomic_cmpxchg() there and retry on failure ?

markj updated this revision to Diff 52498.Jan 2 2019, 11:19 PM
markj marked an inline comment as done.
  • Fix the bug that kib pointed out.
markj added inline comments.Jan 2 2019, 11:21 PM
sys/riscv/riscv/pmap.c
1918 ↗(On Diff #52487)

Indeed, thanks. :(

BTW, I used _long instead of _64 here because I believe we'll eventually want to support 32-bit kernels. I'll update the macros in D18717 in a separate commit.

kib accepted this revision.Jan 2 2019, 11:37 PM
This revision is now accepted and ready to land.Jan 2 2019, 11:37 PM
jhb accepted this revision.Jan 2 2019, 11:55 PM
jhb added inline comments.
sys/riscv/riscv/pmap.c
1874 ↗(On Diff #52498)

Oops.

This revision was automatically updated to reflect the committed changes.