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
Unknown Object (File)
Tue, Mar 3, 3:57 PM
Unknown Object (File)
Mon, Mar 2, 9:43 PM
Unknown Object (File)
Mon, Mar 2, 7:45 AM
Unknown Object (File)
Mon, Mar 2, 7:35 AM
Unknown Object (File)
Mon, Mar 2, 12:36 AM
Unknown Object (File)
Sun, Mar 1, 8:34 AM
Unknown Object (File)
Sun, Mar 1, 6:24 AM
Unknown Object (File)
Sun, Mar 1, 3:28 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 71033
Build 67916: 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
664–665

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

markj added inline comments.
sys/vm/vm_fault.c
664–665

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
664–665

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
668

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
668

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

In D55536#1271983, @alc wrote:

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

No. The latest version adds some prints. @ashafer I wonder if you can test the latest version and confirm which of the added messages you see on the console?

I only see the "upgrading to write fault" for many processes. Tested with both xfce and plasma. Seems like all the apps that initiate graphics get reported by the message (xfwm4, firefox, plasmashell, Xorg, etc)

I only see the "upgrading to write fault" for many processes. Tested with both xfce and plasma. Seems like all the apps that initiate graphics get reported by the message (xfwm4, firefox, plasmashell, Xorg, etc)

Thanks!

In D55536#1271983, @alc wrote:

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

Per @ashafer's results, it seems the constituent pages are already dirty. That isn't surprising for this particular usage.