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

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cem requested review of this revision.Jul 16 2020, 5:02 PM
sys/amd64/amd64/pmap.c
8283 ↗(On Diff #74530)

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

sys/mips/mips/pmap.c
3267 ↗(On Diff #74530)

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 ↗(On Diff #74530)

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 ↗(On Diff #74530)

will fix

sys/mips/mips/pmap.c
3267 ↗(On Diff #74530)

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.