processes: add zombie references, each of them prevents reap

[kib]

Summary

Add the p_zombieref bitmask into struct proc, which enumerates
all legitimate waiters on the process exit status. Among them are
parent for PZOMBIEREF_PARENT, real parent for PZOMBIEREF_ORPHAN if
different from parent, and the holder of the process descriptor for
PZOMBIEREF_PROCDESC, if the process was created by pdfork().

Require all zombie refs to be cleared to reap zombie. This prevents
stealing the exit status from the parent by pdwait()ing on a procdesc
obtained by pdopenpid(), or by waitpid() by debugger from the real
parent.

Additionally, fix reporting of orphan exits,

Due to the way the exit from the loop checking for orphans was
organized, the status of an eligible orphan was dropped to the floor.
exit1() did not woke up the real parent, only the debugger and procdesc
waiter.  Look up the real parent and wake it up on the orphaned child
exit, also setting the PZOMBIEREF_ORPHAN if real parent is different
from the parent.

[kib]

Real latest diff.

[kib]

Note that the patch breaks existing tests, but IMO the current behavior is not logical. For instance, when I truss -f /bin/sh using process descriptors, shell is missing the zombie exit status. In other words, pdopenpid() breaks correct code.

I will update the tests and the man page if we agree on this.

[kib]

sys/kern/kern_exit.c

1271

In particular, this should make the pdfork-ed child invisible to waitpid().

[markj]

In D58264#1336261, @kib wrote:

Note that the patch breaks existing tests, but IMO the current behavior is not logical. For instance, when I truss -f /bin/sh using process descriptors, shell is missing the zombie exit status. In other words, pdopenpid() breaks correct code.

Maybe pdwait() shouldn't let you reap a zombie that isn't your child?

I will update the tests and the man page if we agree on this.

sys/kern/sys_procdesc.c

312

The proc lock is already held here.

[kib]

In D58264#1337050, @markj wrote:

In D58264#1336261, @kib wrote:

Note that the patch breaks existing tests, but IMO the current behavior is not logical. For instance, when I truss -f /bin/sh using process descriptors, shell is missing the zombie exit status. In other words, pdopenpid() breaks correct code.

Maybe pdwait() shouldn't let you reap a zombie that isn't your child?

This is what the patch does, in fact. We still need to return the data from pdwait(2) if called, so we need to keep the process structure around until both parent and procdesc called their wait(). The patch postpones reap_proc().

Or do you mean that if the process is the direct parent of the forked (not pdfork-ed) child and did pdopenpid()/pdwait(), then it should not need waitpid()? IMO it would add unneeded complication for the userspace API.

sys/kern/sys_procdesc.c

312

Yes, I fixed it some time ago but did not re-upload the patch.

[kib]

The current commit from the topic branch.

[kib]

Upload the right patch

[kib]

Fix inverted assertion.

[markj]

In D58264#1337064, @kib wrote:

In D58264#1337050, @markj wrote:

In D58264#1336261, @kib wrote:

Note that the patch breaks existing tests, but IMO the current behavior is not logical. For instance, when I truss -f /bin/sh using process descriptors, shell is missing the zombie exit status. In other words, pdopenpid() breaks correct code.

Maybe pdwait() shouldn't let you reap a zombie that isn't your child?

This is what the patch does, in fact. We still need to return the data from pdwait(2) if called, so we need to keep the process structure around until both parent and procdesc called their wait(). The patch postpones reap_proc().

Ok, I think this is not clear from the commit log message.

Or do you mean that if the process is the direct parent of the forked (not pdfork-ed) child and did pdopenpid()/pdwait(), then it should not need waitpid()? IMO it would add unneeded complication for the userspace API.

No, I think it's fine for pdopenpid() to have consistent semantics regardless of whether the caller is the parent of the target.

sys/kern/kern_fork.c

551

Suppose I pdfork() and then close the descriptor. How can I (or the reaper) reap the child? P2_ZOMBIEREF_PARENT is clear.

sys/kern/sys_procdesc.c

323–325

The read isn't unlocked, though.

664

What should we do with a procdesc received via an SCM_RIGHTS message? Shouldn't it behave the same as one opened by pdopenpid()?

[kib]

sys/kern/kern_fork.c

551

I think that the parent should not be able to reap in this situation.

On the parent exit, the child is reparented to a reaper, and there I believe we should re-enable P2_ZOMBIEREF_PARENT.

sys/kern/sys_procdesc.c

664

I do not quite understand the question.

How does the procdesc that was passed with SCM_RIGHTS, was obtained? It must have been the result of either pdfork() or pdopenpid(). This is recorded in P2_ZOMBIEREF flags. Then, passing the descriptor would just increment the refcount on the file (not even on the struct procdesc). I do not see why it needs any additional actions.

I..e., I think you mean suppose we pdfork-ed, then passed the descriptor. P2_ZOMBIEREF_PROCDESC was already set. In this situation, pdopenpid() does nothing as well.

Note that if we have more than one file referencing the struct procdesc, still only one pdwait() gets the exit status. I do not see why allowing pdwait() on each pd individually would be useful or even correct.

[kib]

Restore P2_ZOMBIEREF_PARENT when reparenting to reaper, to allow reaper do it deeds.

[kib]

sys/kern/kern_exit.c

1500–1501

Could somebody explain the meaning of this check and the action? It came from d0675399d09f02d34, where wait4() was enabled for cap mode, and this block added.

It behaves very strange. If pdforked child exited, and we entered the loop at the right time, then proc_to_reap() above handles the process. Otherwise the continue statement below skips incrementing nfound, and if no other processes found by proc_to_reap(), we return ECHLD (avoiding the sleep).

I tend to think that the block should be just removed.

[kib]

Remove the weird block.
Several minor fixes accumulated from the testing.

[markj]

sys/kern/kern_exit.c

737

The locking protocol requires p's proc lock to be held here, but it is not.

[kib]

Change p_zombieref lock annotation to be just proctree_lock.

[markj]

sys/kern/kern_exit.c

728

It is not orphaned, the flag name is wrong. The process is being debugged.

The procctl man page uses "orphan" to mean something totally different:

PROC_REAP_ACQUIRE
        Enable orphaned process reaping for future children of the current
        process.

        If a parent process exits before one or more of its children
        processes, the remaining children processes are orphaned.

and this usage makes sense to me.

The use of "orphan" to mean "temporarily reparented to a debugger" is very confusing IMO. It should be called "fostering" or something like that. At least, PZOMBIEREF_ORPHAN should be called PZOMBIEREF_TRACER or something like that.

735

Presumably proc_realparent() should assert this, in the (p->p_treeflag & P_TREE_ORPHANED) != 0 case.

1500–1501

The comment seems to explain the motivation. I wonder if another motivation was a future CHERI-like compartmentalization of several modules (e.g., shared libraries) within the same process. A given module might want to be able to fork a child process for some reason, while keeping it invisible from other components. In that case, if a different component calls wait(2), it must not see the "private" child process.

If pdforked child exited, and we entered the loop at the right time, then proc_to_reap() above handles the process.

Yes, I agree.

The wait man page seems to document this behaviour:

The wait() family of functions will only return a child process created
with pdfork(2) if the calling process is not in capsicum(4) capability
mode, and wait has been explicitly given the child's process ID.

sys/kern/sys_procdesc.c

437

Now, if finstall() failed, we may reparent the process anyway?

[kib]

Fix one more locking comment.

[kib]

sys/kern/kern_exit.c

728

It is not _TRACER definitely, the PZOMBIEREF_PARENT is what could be called the _TRACER.
I renamed _ORPHAN to _REALPARENT, it is consistent with the proc_realparent() naming.

1500–1501

This is very strange behavior, even if implemented properly.

I think that the removal is right, since there is relatively complete procdesc API. The semantic breakup is worth it, but I will also think about fixing the fallback if any is reported.

sys/kern/sys_procdesc.c

437

I think that the right condition is

			if ((fp->f_pdflags & F_PD_NOFINSTALL) == 0 &&
			    (p->p_zombieref & (PZOMBIEREF_PARENT |
			    PZOMBIEREF_REALPARENT)) == 0) {

[kib]

Rename PZOMBIEREF_ORPHAN to PZOMBIEREF_REALPARENT.
Move an assert into proc_realparent().
Correct the condition to reparent on the last procdesc close.
Start modifying the man pages.

[markj]

Did you run the ptrace tests? Several fail, and then I see a panic

panic: reaped an orphan (pid 0)
cpuid = 13
time = 1784674501
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe00f1a525b0
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe00f1a52710
vpanic() at vpanic+0x214/frame 0xfffffe00f1a528b0
panic() at panic+0xb5/frame 0xfffffe00f1a52980
kern_wait6() at kern_wait6+0x57c/frame 0xfffffe00f1a52a10
sys_wait4() at sys_wait4+0x18e/frame 0xfffffe00f1a52d10
amd64_syscall() at amd64_syscall+0x3d8/frame 0xfffffe00f1a52f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe00f1a52f30
--- syscall (7, FreeBSD ELF64, wait4), rip = 0x3ce8fd7e46ea, rsp = 0x3ce8f93b2d78, rbp = 0x3ce8f93b2db0 ---

[kib]

In D58264#1339293, @markj wrote:

Did you run the ptrace tests? Several fail, and then I see a panic

panic: reaped an orphan (pid 0)
cpuid = 13
time = 1784674501
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe00f1a525b0
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe00f1a52710
vpanic() at vpanic+0x214/frame 0xfffffe00f1a528b0
panic() at panic+0xb5/frame 0xfffffe00f1a52980
kern_wait6() at kern_wait6+0x57c/frame 0xfffffe00f1a52a10
sys_wait4() at sys_wait4+0x18e/frame 0xfffffe00f1a52d10
amd64_syscall() at amd64_syscall+0x3d8/frame 0xfffffe00f1a52f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe00f1a52f30
--- syscall (7, FreeBSD ELF64, wait4), rip = 0x3ce8fd7e46ea, rsp = 0x3ce8f93b2d78, rbp = 0x3ce8f93b2db0 ---

I run my capsicumized truss to check the branch sanity.

For this case, I believe the assert is simply wrong: it is expected that proc_to_reap() reaps the zombie.

[kib]

proc_to_reap(orphan) can reap.

I promised to look at the tests if there is an agreement on the idea of this change. I know that there is some breakage.

[markj]

In D58264#1339297, @kib wrote:

proc_to_reap(orphan) can reap.

I promised to look at the tests if there is an agreement on the idea of this change. I know that there is some breakage.

Some summary of 1) current semantics 2) proposed semantics would be useful. The review description is not very clear.

As I understand it, 2) is:

• only the parent can reap the child, by calling wait() or pdwait(),
• any procdescs which refer to the child will prevent the child PID from being recycled,
• anybody may use pdwait() to fetch the status of the child, but only the parent can consume the status.

[kib]

In D58264#1339299, @markj wrote:

In D58264#1339297, @kib wrote:

proc_to_reap(orphan) can reap.

I promised to look at the tests if there is an agreement on the idea of this change. I know that there is some breakage.

Some summary of 1) current semantics 2) proposed semantics would be useful. The review description is not very clear.

As I understand it, 2) is:

• only the parent can reap the child, by calling wait() or pdwait(),
• any procdescs which refer to the child will prevent the child PID from being recycled,
• anybody may use pdwait() to fetch the status of the child, but only the parent can consume the status.

Lets ignore the reaping, it is not too relevant for the semantic. The patch is about exit status.

Right now, after the process exited, only one call to *wait*() (i.e. either pdwait() or waitpid()) returns the status.
This was more or less fine until pdopenpid(): if the process was created with fork(), there cannot be a procdesc referencing it. So only waitpid() could get the exit status.
If the process was created with pdfork(), then (AFAIU the model proposed by rwatson) you need to register knote(EVFILTER_PROCDESC, NOTE_EXIT) and get the exit status from the fired event, the zombie was cleared by closing the procdesc.
There is also that strange racy allowance for waitpid() over the forked child.

I changed two things, namely, added pdwait() and pdopenpid(). pdwait() is fine, pdopenpid() is also fine by itself. But combined, they cause trouble: parent forked (not pdforked) the child, and expects that waitpid() must return the exit status. Unrelated code can pdopenpid() the child, and pdwait() on the procdesc, stealing the exit status. The waitpid() in the parent has nothing to return, and paradoxically for the parent, there is no child. I discovered this by running the modified truss.

The proposal is:

• if child was forked, the parent is guaranteed that waitpid() returns the exit status after the child exit.
• If child was pdforked, waitpid does not see the child.
• if there are procdescs on the child (does not matter, was the child forked or pdforked), we guarantee that exactly one pdwait() on some of the procdescs returns the exit status.

Additionally, if the child was temporarily reparented to debugger, then sometimes real parent was missing the exit status. Although not directly related to the proposal above, the fix naturally fits into the changeset.

Is it clearer now?

[markj]

In D58264#1339300, @kib wrote:

In D58264#1339299, @markj wrote:

In D58264#1339297, @kib wrote:

proc_to_reap(orphan) can reap.

I promised to look at the tests if there is an agreement on the idea of this change. I know that there is some breakage.

Some summary of 1) current semantics 2) proposed semantics would be useful. The review description is not very clear.

As I understand it, 2) is:

• only the parent can reap the child, by calling wait() or pdwait(),
• any procdescs which refer to the child will prevent the child PID from being recycled,
• anybody may use pdwait() to fetch the status of the child, but only the parent can consume the status.

Lets ignore the reaping, it is not too relevant for the semantic. The patch is about exit status.

Right now, after the process exited, only one call to *wait*() (i.e. either pdwait() or waitpid()) returns the status.
This was more or less fine until pdopenpid(): if the process was created with fork(), there cannot be a procdesc referencing it. So only waitpid() could get the exit status.
If the process was created with pdfork(), then (AFAIU the model proposed by rwatson) you need to register knote(EVFILTER_PROCDESC, NOTE_EXIT) and get the exit status from the fired event, the zombie was cleared by closing the procdesc.
There is also that strange racy allowance for waitpid() over the forked child.

I changed two things, namely, added pdwait() and pdopenpid(). pdwait() is fine, pdopenpid() is also fine by itself. But combined, they cause trouble: parent forked (not pdforked) the child, and expects that waitpid() must return the exit status. Unrelated code can pdopenpid() the child, and pdwait() on the procdesc, stealing the exit status. The waitpid() in the parent has nothing to return, and paradoxically for the parent, there is no child. I discovered this by running the modified truss.

The proposal is:

• if child was forked, the parent is guaranteed that waitpid() returns the exit status after the child exit.

Makes sense.

• If child was pdforked, waitpid does not see the child.

This changes existing semantics, right? Why is that ok?

• if there are procdescs on the child (does not matter, was the child forked or pdforked), we guarantee that exactly one pdwait() on some of the procdescs returns the exit status.

I'm dumb: why can't they all return the exit status? p_xexit is not cleared after it is read.

Additionally, if the child was temporarily reparented to debugger, then sometimes real parent was missing the exit status. Although not directly related to the proposal above, the fix naturally fits into the changeset.

The "missing the exit status" means, the real parent was not being woken up, right? This is the change to exit1(). So, the real parent does not lose the exit status, it is just not waking up when it should, right?

But I don't quite understand that part: proc_reap() will wake up the real parent in the p->p_oppid != p->p_pptr->p_pid case. Why isn't that sufficient?

[kib]

In D58264#1339528, @markj wrote:

In D58264#1339300, @kib wrote:

The proposal is:

• if child was forked, the parent is guaranteed that waitpid() returns the exit status after the child exit.

Makes sense.

• If child was pdforked, waitpid does not see the child.

This changes existing semantics, right? Why is that ok?

IMO it is illogical. I think that waitpid() was enabled as a desperate measure due to lack of pdwait() and trouble implementing it.
But ok, I think I see how to still allow waitpid(). The proposed semantic is that waitpid() is allowed once, but is not required for reaping, and does not cause reaping. The child is reaped when the procdesc is pdwait-ed or closed still. This is done with 'weak' references.

• if there are procdescs on the child (does not matter, was the child forked or pdforked), we guarantee that exactly one pdwait() on some of the procdescs returns the exit status.

I'm dumb: why can't they all return the exit status? p_xexit is not cleared after it is read.

May be it is me who is dumb, but I do not see then when to reap? The problem is that it is impossible to distinguish between filedesc and file. For instance, I might set a flag on the file referencing procdesc that pdwait() was done on it. But this means that dup-ed file descriptor cannot pdwait().

This behavior would be very confusing, I do not know how to specify it without specifying the implementation.

Additionally, if the child was temporarily reparented to debugger, then sometimes real parent was missing the exit status. Although not directly related to the proposal above, the fix naturally fits into the changeset.

The "missing the exit status" means, the real parent was not being woken up, right? This is the change to exit1(). So, the real parent does not lose the exit status, it is just not waking up when it should, right?

But I don't quite understand that part: proc_reap() will wake up the real parent in the p->p_oppid != p->p_pptr->p_pid case. Why isn't that sufficient?

The existing code handling orphans in kern_wait6():

	if (nfound == 0) {
		LIST_FOREACH(p, &q->p_orphans, p_orphan) {
			ret = proc_to_reap(td, p, idtype, id, NULL, options,
			    NULL, NULL, true);
			if (ret != 0) {
				KASSERT(ret != -1, ("reaped an orphan (pid %d)",
				    (int)td->td_retval[0]));
				PROC_UNLOCK(p);
				nfound++;
				break;
			}
		}
	}

Suppose that proc_to_reap() returned non-zero (in fact it can only return 1). In this case the staus is not returned, we only incremented nfound, that does nothing except allows to sleep. So unless the debugger get rid of its (exited) tracee, the real parent does not see the exit status.

Practically if I only add the wakeup for the real parent, then it has no effect due to the above. Since the debugger needs to execute some syscalls to release the target, the wakeup done at the exit time goes nowhere, we just restart the sleep in kern_wait6().

Might be a wakeup on reparent back to real parent would be enough.

[kib]

Add weak zombie references that allow waitpid() but do not block reaping.

[kib]

Rebase on top of D58407

[kib]

Fix locking in procdecs_close() for zombies case.