Page MenuHomeFreeBSD

Implement INHERIT_ZERO for minherit(2).
ClosedPublic

Authored by delphij on Jul 17 2014, 2:32 AM.
Tags
None
Referenced Files
F81903231: D427.diff
Mon, Apr 22, 10:16 PM
Unknown Object (File)
Wed, Apr 17, 4:33 PM
Unknown Object (File)
Wed, Apr 17, 4:33 PM
Unknown Object (File)
Wed, Apr 17, 4:33 PM
Unknown Object (File)
Wed, Apr 17, 4:33 PM
Unknown Object (File)
Wed, Apr 17, 4:33 PM
Unknown Object (File)
Wed, Apr 17, 4:16 PM
Unknown Object (File)
Wed, Apr 17, 3:23 PM

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 ↗(On Diff #740)

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 ↗(On Diff #740)

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.

delphij edited edge metadata.

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 inline comments.
sys/sys/mman.h
46 ↗(On Diff #744)

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?

sys/vm/vm_map.c
3364 ↗(On Diff #744)

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 ?

Copy avail_ssize, adj_free and max_free.

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

This revision was automatically updated to reflect the committed changes.
wblock added inline comments.
head/lib/libc/sys/minherit.2
95

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

This "The" is not necessary.