Page MenuHomeFreeBSD

Revert r240317 to prevent leaking pmap entries

Authored by cem on Jul 16 2020, 5:02 PM.



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>
Submitted by: Don Morris (amd64)
Discussed with: markj

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

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.

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

will fix

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.