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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| sys/kern/kern_exit.c | ||
|---|---|---|
| 1486–1487 | 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. | |
| sys/kern/kern_exit.c | ||
|---|---|---|
| 734 | The locking protocol requires p's proc lock to be held here, but it is not. | |
| sys/kern/kern_exit.c | ||
|---|---|---|
| 725 | 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. | |
| 732 | Presumably proc_realparent() should assert this, in the (p->p_treeflag & P_TREE_ORPHANED) != 0 case. | |
| 1486–1487 | 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.
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 | ||
| 442 | Now, if finstall() failed, we may reparent the process anyway? | |
| sys/kern/kern_exit.c | ||
|---|---|---|
| 725 | It is not _TRACER definitely, the PZOMBIEREF_PARENT is what could be called the _TRACER. | |
| 1486–1487 | 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 | ||
| 442 | I think that the right condition is if ((fp->f_pdflags & F_PD_NOFINSTALL) == 0 &&
(p->p_zombieref & (PZOMBIEREF_PARENT |
PZOMBIEREF_REALPARENT)) == 0) { | |
Comment Actions
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.
Comment Actions
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 ---
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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?
Comment Actions
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.
Comment Actions
Again, I sense I am misunderstanding something basic, but: only let the (real) parent reap. Whether it used fork() or pdfork() does not matter, only the parent can decide whether to reap. Any procdesc holder can use pdwait() to collect the instantaneous status, or to block until the parent reaps. Once the parent reaps, procdesc_reap() turns all procdescs into tombstones. (The PID can only be reused once all procdescs are closed, though, but maybe this is not desirable.)
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.
Right, but, this just means that after child exit, the debugger must call waitpid() to release the child back to the parent. proc_reap() handles this. Isn't that expected?
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.
Comment Actions
This makes pdwait() unusable, for several reasons. First, it is not guaranteed that the parent would *wait() on the process at all. It is legitimate to pass the fd, and make the recipient to pdwait() on it to reap. Second, other pdwait-ers result is not guaranteed, they could could never know if they get the status or not.
I thought that I can make something workable from your proposal, by combining with the approach in D58407, but pdopenpid/pdwait stay unusable.
(The PID can only be reused once all procdescs are closed, though, but maybe this is not desirable.)
This was explicitly requested by asomers@. It is reasonable.
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.
Right, but, this just means that after child exit, the debugger must call waitpid() to release the child back to the parent. proc_reap() handles this. Isn't that expected?
I will consider it after we decide about zombierefs.
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.
Comment Actions
The first reason is a problem only because of backwards compatibility, right? pdwait() is in 15.1 (I had forgotten about this), we cannot change its behaviour now.
The second reason doesn't quite make sense to me. Once the target process, its status is stored in p_xexit and could be saved in the procdesc if needed. There, it could be read multiple times.
I thought that I can make something workable from your proposal, by combining with the approach in D58407, but pdopenpid/pdwait stay unusable.
(The PID can only be reused once all procdescs are closed, though, but maybe this is not desirable.)
This was explicitly requested by asomers@. It is reasonable.
Ok, that seems fine.
Comment Actions
Well, I believe that pdwait() would be otherwise useless, regardless of the stability guarantees. The whole point of passing the procdesc it to allow the recipient to act as if it is the parent.
The second reason doesn't quite make sense to me. Once the target process, its status is stored in p_xexit and could be saved in the procdesc if needed. There, it could be read multiple times.
It is more involved, and this is what I did in D58407.
I thought that I can make something workable from your proposal, by combining with the approach in D58407, but pdopenpid/pdwait stay unusable.
(The PID can only be reused once all procdescs are closed, though, but maybe this is not desirable.)
This was explicitly requested by asomers@. It is reasonable.
Ok, that seems fine.
Comment Actions
Drop the orphaned handling change for now.
Document most rational approach for mixing waitpid() and procdescs I can end up with, so far. I believe it is compatible with the current implementation.
Add PD_NOWAITPID flag for pdfork(), to get the better behavior.
Hopefully it is cleaner now.
Comment Actions
I am fine with that revision in principle, but maybe @asomers will have some opinion there? I guess that this patch depends on that one?
So to summarize, if the process was created with pdfork(PD_NOWAITPID), then the process is reaped when the last referencing procdesc is closed. Otherwise, it is only reaped when the parent or reaper calls waitpid() on it (but not pdwait()). Is that right?
Do we really need PD_NOWAITPID? How does it interact with the pdrfork() RFNOWAIT flag?
| lib/libsys/pdfork.2 | ||
|---|---|---|
| 208 ↗ | (On Diff #182816) | This is true even before the process exited. Maybe, "The .Fn pdwait function may be called on a process descriptor an unlimited number of times. In particular, it does not reap the target process, even if it has exited." |
| 216 ↗ | (On Diff #182816) | |
| 217 ↗ | (On Diff #182816) | |
| 221 ↗ | (On Diff #182816) | |
| 230 ↗ | (On Diff #182816) | I find the following phrasing a bit clearer: If the process was created by .Fn pdfork , and the .Dv PD_NOWAITPID flag was not specified, then after exiting, the process will not be reaped until the parent or reaper has called .Xr waitpid 2 and all process descriptors referencing the process are closed. |
| 236 ↗ | (On Diff #182816) | |
| 245 ↗ | (On Diff #182816) | How does all of this interact with PS_NOCLDWAIT? |
| sys/kern/kern_exit.c | ||
| 683 | I think this was rebased incorrectly. | |
Comment Actions
I would be happy to drop the flag, but I do not see how to provide both old behavior, and the reasonable (new) behavior without it.
How does it interact with the pdrfork() RFNOWAIT flag?
Right, I changed fork to only set PZOMBIEREF_PARENT when RFNOWAIT is not set.
| lib/libsys/pdfork.2 | ||
|---|---|---|
| 245 ↗ | (On Diff #182816) | PS_NOCLDWAIT is only handled somehow if PZOMBIEREF_PARENT is set, i.e. waitpid(2) was allowed during fork. Then the behavior is same. Otherwise, there is no parent involvement. |
Comment Actions
Handle RFNOWAIT.
Add chunk that stops reaping in the kern_pdwait() loop.
Edit man page.
| lib/libsys/pdfork.2 | ||
|---|---|---|
| 207 ↗ | (On Diff #182997) | |
| 241 ↗ | (On Diff #182997) | |
| 243 ↗ | (On Diff #182997) | |
| sys/kern/kern_exit.c | ||
| 1015 | Nitpicking, but I'd expect it is better to release proctree_lock first, as it's more likely to be contended. | |
| 1513 | This will always return 0, no? The (p->p_zombieref & zombieref) == 0 check in proc_to_reap() will be trivially true, so if p is an orphan (i.e., it has been re-parented to a debugger), and the real parent wait()s on it, we will not increment nfound. | |
| sys/kern/kern_fork.c | ||
| 556 | Don't we need to handle RFNOWAIT in this branch too? It can be passed to plain rfork(). Or does the reaper need PZOMBIEREF_PARENT anyway? | |
Comment Actions
Restore the 'check_only' argument to proc_to_reap(). Directly use PZOMBIEREF_PARENT in proc_to_reap(), it cannot use any other zombie reference.
| sys/kern/kern_fork.c | ||
|---|---|---|
| 556 | Reaper needs PZOMBIEREF_PARENT. The flag is required for the waitpid() to get the status and to reap the process. Put it differently, for waitpid() to behave as if there is no procdescs, the PZOMBIEREF_PARENT must be set. | |
Comment Actions
When reparenting back to the real parent, restore PZOMBIEREF_PARENT, but only if the real parent wants to call waitpid() on the child.
Comment Actions
I ran the full regression test suite with the three patches applied, and got an OOM somewhere in the network tests. I can see a zombie leak somewhere, running the test again I see:
6441 threads: 19 running, 834 sleeping, 4960 zombie, 628 waiting
and the zombie count is growing slowly.
Comment Actions
Could you please print out p_zombierefs for several of such zombies?
BTW one of the changes that I upload shortly might fix it, but I am not sure.
I will ask pho to help with the reproduction.
Comment Actions
Clear PZOMBIEREF_PROCDESC when the last procdesc reference is gone.
Man page editing.
Comment Actions
I think this is ok. The result is not quite the same as what I proposed originally: I believe, with your patches, that if a process creates a child with pdfork(), then it cannot use pdwait() to reap the child, it still must use waitpid(). It would be nicer to make pdwait() reap the child in this case, IMO.
Comment Actions
If the process is created with pdfork(PD_NOWAITPID), then waitpid() is not needed for reap. We can add proc_reap(PZOMBIEREF_PROCDESC) into pdwait() PDF_EXITED block to reap on the first pdwait(). I do not have strong opinion on this, but it is somewhat illogical IMO: the zombie is gone, but other procdescs would still return exit status. Right now zombie is reaped on the last procdesc close.