Page MenuHomeFreeBSD

Revert r240317 to prevent leaking pmap entries
ClosedPublic

Authored by cem on Jul 16 2020, 5:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 12, 8:22 AM
Unknown Object (File)
Fri, Apr 12, 8:22 AM
Unknown Object (File)
Mar 1 2024, 1:21 PM
Unknown Object (File)
Dec 20 2023, 3:04 AM
Unknown Object (File)
Dec 12 2023, 8:00 PM
Unknown Object (File)
Oct 14 2023, 1:30 PM
Unknown Object (File)
Aug 24 2023, 11:49 PM
Unknown Object (File)
Aug 4 2023, 11:18 AM
Subscribers

Details

Summary

Subsequent to r240317, kmem_free() was replaced with kva_free() (r254025).
kva_free() releases the KVA allocation for the mapped region, but no longer
clears the pmap (pagetable) entries.

An affected pmap_unmapdev operation would leave the still-pmap'd VA space
free for allocation by other KVA consumers. However, this bug easily
avoided notice for ~7 years because most devices (1) never call
pmap_unmapdev and (2) on amd64, mostly fit within the DMAP and do not need
KVA allocations. Other affected arch are less popular: i386, MIPS, and
PowerPC. Arm64, arm32, and riscv are not affected.

Reported by: Don Morris <dgmorris AT earthlink.net>
Submitted by: Don Morris (amd64)
Discussed with: markj

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32369
Build 29850: arc lint + arc unit

Event Timeline

cem requested review of this revision.Jul 16 2020, 5:02 PM
sys/amd64/amd64/pmap.c
8283

size / PAGE_SIZE is more commonly spelled atop(size).

sys/mips/mips/pmap.c
3267

Why not use qremove? pmap_kremove_device() doesn't perform TLB invalidation, which I presume is why you're using qremove elsewhere. The subr_devmap.c implementation doesn't either, which seems like a bug.

sys/mips/mips/pmap.c
3267

I agree. I think that the only problem that this issue introduces is the lack of invalidation on removal, which means that next mapping (which we typically do without invalidating the allocated VA) has undefined consequences. It is almost fine to leave stale PTEs, except for speculations, but not fine to leave TLB unflushed.

cem planned changes to this revision.Jul 16 2020, 8:23 PM
cem added inline comments.
sys/amd64/amd64/pmap.c
8283

will fix

sys/mips/mips/pmap.c
3267

will do.

cem marked 2 inline comments as done.
  • pmap_qremove() over pmap_kremove_device() in mips
  • atop() for "/ PAGE_SIZE"
This revision is now accepted and ready to land.Jul 16 2020, 8:33 PM
This revision was automatically updated to reflect the committed changes.