Page MenuHomeFreeBSD

vm_fault: Avoid creating clean, writeable superpage mappings
Needs ReviewPublic

Authored by markj on Thu, Feb 26, 6:38 PM.
Tags
None
Referenced Files
F146278197: D55536.diff
Sun, Mar 1, 8:34 AM
F146267648: D55536.id.diff
Sun, Mar 1, 6:24 AM
F146252320: D55536.id172781.diff
Sun, Mar 1, 3:28 AM
F146238979: D55536.id172897.diff
Sun, Mar 1, 1:02 AM
F146237920: D55536.id172897.diff
Sun, Mar 1, 12:47 AM
Unknown Object (File)
Fri, Feb 27, 10:11 AM
Unknown Object (File)
Fri, Feb 27, 4:28 AM
Unknown Object (File)
Fri, Feb 27, 1:06 AM
Subscribers

Details

Reviewers
alc
kib
Summary

The pmap layer requires writeable superpage mappings to be dirty.
Otherwise, during demotion, we may miss a hw update of the PDE which
sets the dirty bit.

When creating a managed superpage mapping without promotion, i.e., with
pmap_enter(psind == 1), we must therefore ensure that a writeable
mapping is created with the dirty bit pre-set. To that end,
vm_fault_soft_fast(), when handling a map entry with write permissions,
checks whether all constituent pages are dirty, and if so, converts the
fault to a write fault, so that pmap_enter() does the right thing. If
one or more pages is not dirty, we simply create a 4K mapping.

vm_fault_populate(), which may also create superpage mappings, did not
do this, and thus could create mappings which violate the invariant
described above. Modify it to instead proactively mark
constituent pages dirty if the mapping is writeable, and then
ensure that the mapping is dirty. Though, instead of dirtying the pages,
we could instead create a read-only mapping if some pages are not
already dirty.

Reported by: ashafer

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 71085
Build 67968: arc lint + arc unit

Event Timeline

markj requested review of this revision.Thu, Feb 26, 6:38 PM
sys/vm/vm_fault.c
606

Don' largepage code need the same treating?

Can confirm this fixes the reported panic, thanks! Tested xfce and KDE can survive repeated restarts. fwiw this looks and seems reasonable to me but you'll want someone who knows vm code properly to sign off too.

markj added inline comments.
sys/vm/vm_fault.c
606

I don't think so: largepage mappings are unmanaged (pmap_enter() asserts this) and writeable unmanaged mappings are automatically marked dirty, no matter the fault type (this is also handled in pmap_enter()).

kib added inline comments.
sys/vm/vm_fault.c
606

IMO setting the fault type to VM_PROT_WRITE always would be more safe (or portable to other arches, depending on view). But ok.

This revision is now accepted and ready to land.Fri, Feb 27, 6:04 PM
sys/vm/vm_fault.c
676–677

If you do this before the loop, won't vm_fault_dirty() dirty the page?

markj added inline comments.
sys/vm/vm_fault.c
676–677

Yes, I believe so.

I now think it's wrong to convert this fault into a write fault. Earlier, we passed the original fault type to vm_pager_populate(), and changing the type after that point seems fraught.

markj marked an inline comment as done.

Rather than preemptively dirtying pages, make the map read-only unless all pages are already dirty.

This revision now requires review to proceed.Fri, Feb 27, 9:46 PM
sys/vm/vm_fault.c
676–677

The new version still changes the fault type, but only if the pages are already dirty, in which case the pager should have observed a write fault from somewhere, even if it's a different mapping.

sys/vm/vm_fault.c
671

Isn't this reversed? If any of the pages is dirty, then the while mapping should set PG_M?

@markj Do we know if there is actually a mix of clean and dirty pages being mapped?

sys/vm/vm_fault.c
671

No, this version is creating a read-only superpage mapping if there exists at least one page that is not dirty.