Details

Reviewers

asomers
brooks
emaste
markj

Group Reviewers

capsicum

Commits

rG2d555ec85a71: lib/libsys, lib/libc: export pdwait
rGf7b56887cc07: Document pdrfork(2) and pdwait(2)
rG4d707825bf62: Add pdwait(2)
rG09984871d8ca: procdesc: postpone freeing the zombie' pid until procdesc is freed
rGaa72df78d799: sys: Add cap_pdwait_rights
rGa560abedfb4f: audit: handle AUE_PDWAIT
rG7fe33d58a826: kern/kern_exit.c: extract wait6_check_alive() helper
rG2b67cfa39d83: kern/kern_exit.c: extract some helpers from proc_to_reap()
rG109b9f48ec4e: kern/kern_exit.c: define the exterror category for exit/wait
rGf10b4b6131d4: lib/libsys, lib/libc: export pdrfork(2)
rG5c2ee618d5ec: sys: add pdrfork(2)
rGd0d4b9b9df2a: sys: add AUE_PDRFORK
rG7211cd2cce74: kern/kern_fork.c: define the exterror category for fork
rG472c32a83b27: kern/kern_fork/exit.c: organize includes
rGf5acbacb28f9: kern/kern_exit.c: some style
rG6af3cf27ed00: freebsd32_rusage_out(): bzero the compat32 structure

Summary

kern/kern_exit.c: some style


kern/kern_fork/exit.c: organize includes

Remove sys/cdefs.h.
Remove sys/param.h.
Order the sys/*.h includes alphabetically.


kern/kern_fork.c: define the exterror category for fork

Convert EINVALs in kern_fork.c into EXTERRORs.


sys: add AUE_PDRFORK


contrib/openbsm: add AUE_PDRFORK


sys: add pdrfork(2)


lib/libsys, lib/libc: export pdrfork(2)


kern/kern_exit.c: define the exterror category for exit/wait

Convert EINVALs in kern_exit.c into EXTERRORs.


kern/kern_exit.c: extract some helpers from proc_to_reap()


kern/kern_exit.c: extract wait6_check_alive() helper


Add pdwait(2)


lib/libsys, lib/libc: export pdwait


Regen for the fork and exit/wait exterror category addition


Regen syscall tables after pdfork(2) and pdwait(2) additions

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 69740
Build 66623: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Renumber AUE_PDRFORK.
Drop contrib/openbsm patch, similar to how other new AUE_ constants are not imported there.

Do not release procdesc->pd_pid if the descriptor was already detached from the struct proc. In this case, the pid is freed by reaping.

This looks great, @kib! If you commit it, I'll follow up with the test suite. A few questions:

Are you planning to write the man page yourself, or would you like help with that?
Even if you don't MFC the new syscalls, would it be possible to MFC the pid recycling change?

This revision is now accepted and ready to land.Tue, Jan 20, 1:12 AM

Add documentation.
Fix pdwait() prototype.

This revision now requires review to proceed.Tue, Jan 20, 3:41 AM

Herald added a subscriber: ziaee. · View Herald TranscriptTue, Jan 20, 3:41 AM

In D54592#1252227, @asomers wrote:

This looks great, @kib! If you commit it, I'll follow up with the test suite. A few questions:

Are you planning to write the man page yourself, or would you like help with that?

I prototyped the initial version of the docs. Feel free to suggest improvements.

Even if you don't MFC the new syscalls, would it be possible to MFC the pid recycling change?

I plan to MFC everything. It depends on how the live testing in HEAD goes, of course.

Add note about the pid reuse.

Thanks for writing the man page too. I've made a few comments, mostly to make the English more grammatical and idiomatic.

lib/libsys/pdfork.2
33 ↗	(On Diff #170090)	Don't forget to adjust .Dd too
104 ↗	(On Diff #170090)	More idiomatic English.
112–114 ↗	(On Diff #170090)	number agreement
117 ↗	(On Diff #170090)
118 ↗	(On Diff #170090)
134–135 ↗	(On Diff #170090)
138 ↗	(On Diff #170090)
177–178 ↗	(On Diff #170090)
233 ↗	(On Diff #170090)	Or perhaps sooner, if MFCd? I'll set a reminder for myself to update the man page before 15.1, if you've MFCd by then.

This revision now requires changes to proceed.Tue, Jan 20, 1:21 PM

ziaee added inline comments.Tue, Jan 20, 1:48 PM

lib/libsys/pdfork.2
80 ↗	(On Diff #170090)

emaste added inline comments.Tue, Jan 20, 1:48 PM

lib/libsys/pdfork.2
102 ↗	(On Diff #170090)	I think this sentence was OK before, but was a bit long/wordy, tried to tighten it up slightly here. @asomers does this change sound fine?
112–114 ↗	(On Diff #170090)	Mine would be "for a description of the possible rfflag flags."

asomers added inline comments.Tue, Jan 20, 2:03 PM

lib/libsys/pdfork.2
102 ↗	(On Diff #170090)	Sure, except you're missing a space in "betweenthe".

kib marked 13 inline comments as done.Tue, Jan 20, 2:33 PM

kib added inline comments.

lib/libsys/pdfork.2
233 ↗	(On Diff #170090)	Yes, it is only a safest estimation.

Grammar and wording of the man page update.

asomers requested changes to this revision.Tue, Jan 20, 2:48 PM

asomers added inline comments.

lib/libsys/pdfork.2
182–183 ↗	(On Diff #170123)	Missing macro dots

This revision now requires changes to proceed.Tue, Jan 20, 2:48 PM

Correct macro names.

asomers accepted this revision.Tue, Jan 20, 3:16 PM

This revision is now accepted and ready to land.Tue, Jan 20, 3:16 PM

crest_freebsd_rlwinm.de added a subscriber: crest_freebsd_rlwinm.de.Tue, Jan 20, 3:45 PM

markj added inline comments.Tue, Jan 20, 10:15 PM

sys/kern/kern_exit.c
1322	Why "%jx"? `options` is an int.
1452	Is it formally correct to dereference `p` here? AFAICS there is nothing preventing it from being recycled after the proc lock is dropped.
1559	Why do we check p_state == PRS_ZOMBIE twice? I do not see how it can change: if wait6_check_alive() returns false, the proc lock will not have been dropped.
1573	I think this path is missing fdrop()?

kib marked 4 inline comments as done.Wed, Jan 21, 12:31 AM

kib added inline comments.

sys/kern/kern_exit.c
1322	The format is utilized by usermode, which always casts the arguments to uintmax_t: we do not know the real types of the values passed by the kernel, we only know that they are integer. I noted it in the exterror(9) man page that the 'j' modifier must always be applied.
1452	Formally yes because struct proc are type-stable. Of course there is a possible race with the 'p' reuse, whatever unlikely it is. I changed this to use the local variable pid that already caches the pid, same as in line 1432.
1559	Yes, I think this if(){} block can be removed. This is a remnant of earlier, different, structure of the pdwait() loop.

Avoid racy access to the p_pid member of possibly reclaimed struct proc.
Other review' comments are handled.

This revision now requires review to proceed.Wed, Jan 21, 12:32 AM

markj accepted this revision.Wed, Jan 21, 1:54 PM

markj added inline comments.

lib/libsys/pdfork.2
115 ↗	(On Diff #170167)	Either the "the" should be removed, or you should write "The .Fn pdrfork system call".
136 ↗	(On Diff #170167)
sys/compat/freebsd32/freebsd32_misc.c
303	BTW, I suspect freebsd32_rusage_out() should do some zeroing. struct timeval32 will have a hole on non-amd64 platforms, so we are copying out uninitialized memory there.
sys/kern/kern_exit.c
1452	Right, I was referring to the race.
sys/kern/subr_capability.c
93 ↗	(On Diff #170167)	rights.4 should be updated to mention the new CAP_PDWAIT.
sys/kern/sys_procdesc.c
314	Do we need to wake up the kern_pdwait() loop here too? I guess not, since the pdwait() caller must be holding a reference on the procdesc, so PDF_CLOSED cannot be set?

This revision is now accepted and ready to land.Wed, Jan 21, 1:54 PM

kib marked 6 inline comments as done.Wed, Jan 21, 2:20 PM

kib added inline comments.

lib/libsys/pdfork.2
136 ↗	(On Diff #170167)	It is function as well, and might become a wrapper some day. Then we need to edit the man page, this is why I slightly prefer the function term. But ok.
sys/kern/sys_procdesc.c
314	If PDF_CLOSED set, then indeed no process might have the file installed into the descriptors table.

markj added inline comments.Wed, Jan 21, 2:25 PM

lib/libsys/pdfork.2
136 ↗	(On Diff #170167)	Ok, it is just a suggestion for consistency, e.g., above you wrote "The pdrfork() system call".
sys/kern/sys_procdesc.c
314	I would maybe add a KASSERT that PDF_CLOSED is not set in the kern_pdwait() loop, but it is just a suggestion.