Page MenuHomeFreeBSD
Differential D17323

Fix two issues with mprotect(PROT_WRITE) and wiring.
ClosedPublic
Actions

Authored by kib on Sep 25 2018, 8:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 1, 1:53 AM
Unknown Object (File)
Tue, Apr 16, 6:35 PM
Unknown Object (File)
Tue, Apr 16, 6:35 PM
Unknown Object (File)
Sun, Apr 14, 4:17 AM
Unknown Object (File)
Apr 2 2024, 2:23 AM
Unknown Object (File)
Mar 31 2024, 1:36 PM
Unknown Object (File)
Mar 19 2024, 5:13 PM
Unknown Object (File)
Mar 9 2024, 11:44 AM
View All 43 Files
Subscribers
imp

Details

Reviewers
andrew
alc
markj
Commits
rS338999: Correct vm_fault_copy_entry() handling of backing file truncation
rS338998: In vm_fault_copy_entry(), we should not assert that entry is charged
rS338997: In vm_fault_copy_entry(), collect the code to initialize a newly
Summary

There are two fixes for vm_fault_copy_entry():

  • We should not assert that entry is charged if the dst_object is not of swap type. It can only happen when entry does not require copy, otherwise vm_map_protect() already adds the charge. So the assert was right for the case where swap object was allocated in the vm_fault_copy_entry(), but not when it was just copied from src_entry and its type is not swap.
  • if a wired map entry is backed by vnode and the file is truncated, corresponding pages are invalidated. vm_fault_copy_entry() should be aware of it and allow for invalid pages past end of file. Also, such pages should be not mapped into userspace.

Found by: andrew using syzkaller

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 19851

Event Timeline

kib created this revision.Sep 25 2018, 8:16 PM
Herald added a subscriber: imp. · View Herald TranscriptSep 25 2018, 8:16 PM
Harbormaster completed remote builds in B19814: Diff 48461.Sep 25 2018, 8:16 PM
kib marked an inline comment as done.Sep 25 2018, 8:17 PM
kib added inline comments.
sys/vm/vm_fault.c
1753

As I noted elsewere, this assert might be reasonable dropped at all due to msync(MS_INVALIDATE).

alc added inline comments.Sep 26 2018, 3:28 PM
sys/vm/vm_fault.c
1653–1656

I think it clearer to include the new conditions here, and not in the KASSERT:

} else if ((dst_object->type == OBJT_DEFAULT ||
    dst_object->type == OBJT_SWAP)  &&
    dst_object->cred == NULL)

This will add to the run-time cost of handling DEFAULT and SWAP objects, but eliminate the pointless assignments for other types of objects.

1753

Thinking ...

alc added inline comments.Sep 26 2018, 3:56 PM
sys/vm/vm_fault.c
1643–1644

On an unrelated note, couldn't the assignments here to the object move up to the previous "else"? They don't need to be here any more than the assignments to pg_color or flags. (The object is effectively private for a short time after creation.)

kib updated this revision to Diff 48487.Sep 26 2018, 8:20 PM
kib marked an inline comment as done.

Move all dst_object initialization lines into creation block.
Rearrange conditional in if vs KASSERT.

Harbormaster completed remote builds in B19827: Diff 48487.Sep 26 2018, 8:20 PM
alc added inline comments.Sep 27 2018, 5:20 PM
sys/vm/vm_fault.c
1738

Couldn't we simply break out of the enclosing "for" loop here if src_m is after the end of the object? Then, neither the KASSERT nor pmap_enter() call would have to change. Moreover, further iterations of the loop are pointless.

kib updated this revision to Diff 48530.Sep 27 2018, 6:47 PM
kib marked an inline comment as done.

End the loop if out of bounds.

Harbormaster completed remote builds in B19847: Diff 48530.Sep 27 2018, 6:47 PM
alc added inline comments.Sep 27 2018, 10:29 PM
sys/vm/vm_fault.c
1749–1750

Can't you reorder things so that the xunbusy() isn't needed? In other words, move the xbusy() below the "if" statement. Otherwise, this looks good to me.

kib updated this revision to Diff 48535.Sep 27 2018, 10:45 PM

Remove xunbusy(). Reduce diff.

Harbormaster completed remote builds in B19851: Diff 48535.Sep 27 2018, 10:45 PM
alc accepted this revision.Sep 28 2018, 4:13 AM
This revision is now accepted and ready to land.Sep 28 2018, 4:13 AM
Closed by commit rS338997: In vm_fault_copy_entry(), collect the code to initialize a newly (authored by kib). · Explain WhySep 28 2018, 2:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
vm/
15 lines

Diff 48535

View Options

sys/vm/vm_fault.c

Loading...