Implement INHERIT_ZERO for minherit(2).
ClosedPublic
Actions

Authored by delphij on Jul 17 2014, 2:32 AM.

Details

Reviewers

kib
jhb

Group Reviewers

manpages

Commits

rS319278: MFC r315272, r315370
rS319277: MFC r315272, r315370
rS315272: Implement INHERIT_ZERO for minherit(2).

Summary

INHERIT_ZERO is an OpenBSD feature. When a page is marked as such, it would
be zeroed upon fork(). This would be used in new arc4random(3) functions.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

delphij updated this revision to Diff 740.Jul 17 2014, 2:32 AM

delphij retitled this revision from to Implement INHERIT_ZERO for minherit(2)..

delphij updated this object.

delphij edited the test plan for this revision. (Show Details)

delphij added reviewers: kib, jhb.

The unobvious consequence of this patch is that not only VM_INHERIT_ZERO changes map entry in the child, but it also modifies behaviour of the fork() WRT the parent entry. For VM_INHERIT_COPY, both parent and child get private shadow objects inserted above the backing object for the entry. For VM_INHERIT_ZERO, child gets fresh empty object, while parent is kept without the shadow. In other words, VM_INHERIT_COPY made the parent also COW, while VM_INHERIT_ZERO implicitely shares.

I am not sure if this is important, but it is subtle.

sys/vm/vm_map.c
3355	This comment is definitely not true. I suppose that the code was copied from the VM_INHERIT_COPY case. It might be more reasonable to not do new_entry = old_entry assignment at all, since the only copied fields are start/end and (partially) flags.
3366	This is not the correct test. The entry charge may be already migrated to the backing object. You should use ENTRY_CHARGED() to see if the entry is accounted for swap use.

Looking at it some more, I think now that the test is not needed at all. New entry is the anonymous memory, virtually allocated at the fork time. The charge must be applied to the child' map entry unconditionally, it does not matter what kind of memory backed the same range in the parent process.

Attempt to fix most problems raised by kib@:

Fixed comments;
Limit new_entry copying by explicitly copying fields we need;
Unconditionally charge process because we are (actually) allocating new pages.

I don't see anything wrong, but I don't promise to understand all the details of setting up the new vm map entry correctly.

emaste added a subscriber: emaste.Oct 2 2014, 2:32 PM

emaste added inline comments.

sys/sys/mman.h
46	Can we make this MAP_INHERIT_ZERO (and provide MAP_ versions of the other ones, too)? OpenBSD and NetBSD use MAP_*, and there was discussion long ago about using the same #defines in FreeBSD (from before user-visible constants appeared in mman.h) It would be nice to avoid needing compatibility #defines or #ifdef FreeBSD tests in all of the consumers.

Anyway, it is relatively easy to implement syscall-less getpid(),
and simultaneously implement something which I called forkdepth(3).

So you'd use as the reseed necessary test getpid() != last_pid || forkdepth() != last_forkdepth?

kib added inline comments.Oct 2 2014, 3:01 PM

sys/vm/vm_map.c
3364	Hm, you need also avail_ssize, adh_free and max_free copied as well. Does it make sense to set stack to VM_INHERIT_ZERO ?

op added a subscriber: op.Feb 4 2015, 2:33 AM

delphij marked an inline comment as done.Mar 14 2017, 8:01 AM

Copy avail_ssize, adj_free and max_free.

(I don't think it makes sense to INHERIT_ZERO on stack though).

Herald added a reviewer: manpages. · View Herald TranscriptMar 14 2017, 8:03 AM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B8040: Diff 26243.Mar 14 2017, 8:46 AM

Closed by commit rS315272: Implement INHERIT_ZERO for minherit(2). (authored by delphij). · Explain WhyMar 14 2017, 5:10 PM

This revision was automatically updated to reflect the committed changes.

wblock added a subscriber: wblock.Apr 7 2017, 5:32 PM

wblock added inline comments.

head/lib/libc/sys/minherit.2
95 ↗	(On Diff #26260)	Change from passive to active and edit a bit: Cause the address space in question to be mapped as new anonymous pages which are initialized to all zero bytes in the child process.
139 ↗	(On Diff #26260)	This "The" is not necessary.