Problems handling teardown of pid namespaces
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 10.7k
- Forks
- 662
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 2
Description
In 4e10088344fba26eac97c6d76836d1baaba96cdd and followup work such as 5d7479dbea23d589532715ecce18ef85622ed6a1 @Keno got rid of "unstable exits" in favour of deterministic reaping of tasks when we know they're ready to be reaped. Unfortunately there are cases that didn't handle: when a process that is pid-1 in a PID namespace exits, rr can't reap it until rr has reaped all other tasks in that pid namespace. (Otherwise we deadlock, because the reaper task of a pid namespace (which is usually but not always tid 1 in that namespace) won't reach zombie state until all of the other tasks in that namespace have been reaped by rr.) The complexity of handling that keeps piling higher and higher :-(. Here's my attempt to chronicle this.
f12fe70d3d81033ebcd3812ccad7da44b8051f6b was my first step to handling that case. It just tries to detect in a straightforward way whether a task that has reached PTRACE_EVENT_EXIT would deadlock in the above manner, i.e. whether it looks like the reaper task for its pid namespace and there are unreaped tasks in that namespace. (See waiting_for_pid_namespace_tasks_to_exit().) That certainly helped but it turns out to not be enough.
Another bug related to reaping showed up and was fixed by 3875d7a530cbf7f21f3ac058318cc6f601048d35 (testcase in ff435c1f056f4fdc092699d2fce1858e255fd75c). What happens here is that we get a PTRACE_EVENT_EXIT for a thread-group leader T before receiving PTRACE_EVENT_EXIT for one of its child threads. In that case, rr was (correctly) declining to reap T, but RecordTask::did_reach_zombie would still delete our RecordTask object, so rr never reaps T. But then when trying to kill_all_tasks() at session end, rr would deadlock because one of the tasks to kill would enter zap_pid_ns_processes and wait to reap T, which it never can because rr is still tracing it. I fixed that by setting waiting_for_reap to true, not deleting the RecordTask when we decline to reap in did_reach_zombie, and letting Scheduler schedule and reap tasks in such a state. That fixed that bug, but to some extent reintroduced unstable exits :-(.
An ongoing source of problems is that a task can sometimes go straight through PTRACE_EVENT_EXIT without rr being aware of it, i.e. without rr seeing the PTRACE_EVENT_EXIT. I see this happening reliably in this situation: task A is pid-1 in a new PID namespace; it has a child process B which spawns thread C; thread C does exit_group (and actually does proceed through PTRACE_EVENT_EXIT and exits), which triggers a kill of task B; task B advances to PTRACE_EVENT_EXIT stop; simultaneously, task A exits and proceeds through its PTRACE_EVENT_EXIT to the kernel's zap_pid_ns_processes, sending a SIGKILL to B; that SIGKILL breaks B out of its PTRACE_EVENT_EXIT and B can become a zombie --- or reach its own zap_pid_ns_processes, if it's pid-1 in a nested pid-namespace --- without rr being aware that anything has happened to it. (We could infer from C's exit_group that B will die, but that smells of unstable-exit, and there may be cases where B dies in other ways.) BTW in practice we seem to need additional sibling threads of B/C to trigger the bug, I'm not sure why.
One bug related to this was when rr does a try_wait() on task B in that zap_pid_ns_processes() state. waitid(WSTOPPED | WNOHANG) would return ECHILD (B is not stopped), and waitid(WEXITED | WNOHANG) would return 0 with pid 0, since B has not exited. This triggered rr assertions. My fix in 7b822afc7cb061bad9043f6ad8229ec4de6fd39e; it's easy, just don't treat B as waitable in that case.
The bug I'm wrestling with now, and don't have a fix for, is this: task B is pid-1 in its own nested pid-namespace, and spawns a subprocess S into that namespace, and also has a sibling thread C. The above dance happens and B reaches zombie state without rr observing a status change --- it skips zap_pid_ns_processses because thread C is currently still alive to act as the pid-namespace reaper. Then C exits, rr gets its PTRACE_EVENT_EXIT, and waiting_for_pid_namespace_tasks_to_exit() returns false, because C's thread-group (according to rr) still has the live thread B to act as the reaper for the nested pid-namespace :-(. So, we go ahead and wait for C to exit, but it can't because S needs to be reaped by rr --- deadlock. (In particular, we hang in waitid(P_PID, tid, &info, WSTOPPED | WNOWAIT) called by Task::wait_exit; that's not expecting to see a stop, it's just waiting to return ECHILD when the task reaches zombie state.) I'm not sure how to fix this.
The bigger picture here is that trying to anticipate when we should reap each task is pretty difficult given the complexity the kernel's behavior, especially once this pid-namespace stuff is taken into account. And when we get this wrong, we have bugs that are very subtle, often intermittent due to variant scheduling, and very difficult to diagnose and write compact reliable testcases for. I haven't observed differences in behavior across kernel versions yet, but I wouldn't be surprised if there are some and I wouldn't feel justified complaining to kernel devs if there were.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with waiting_for_pid_namespace_tasks_to_exit(), RecordTask::did_reach_zombie, Scheduler, kill_all_tasks(), and Task::wait_exit(). Reproduce the nested PID-namespace scenario described, then trace how rr handles PTRACE_EVENT_EXIT and reaping when the namespace reaper changes. Done means the deadlock is resolved without reintroducing unstable exits.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, linux
- Domain
- devtools, operating-systems
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100