On Tue, Jul 12, 2022 at 09:34:50AM -0500, Eric W. Biederman wrote:
> Tycho Andersen <tycho@tycho.pizza> writes:
>
> > On Mon, Jul 11, 2022 at 06:06:21PM -0500, Eric W. Biederman wrote:
> >> Tycho Andersen <tycho@tycho.pizza> writes:
> >> It is not different enough to change the semantics. What I am aiming
> >> for is having a dedicated flag indicating a task will exit, that
> >> fatal_signal_pending can check. And I intend to make that flag one way
> >> so that once it is set it will never be cleared.
> >
> > Ok - how far out is that? I'd like to try to convince Miklos to land
> > the fuse part of this fix now, but without the "look at shared signals
> > too" patch, that fix is useless. I'm not married to my patch, but I
> > would like to get this fixed somehow soon.
>
> My point is that we need to figure out why you need the look at shared
> signals.
At least in the case where the task was already exiting, it's because
complete_signal() never wakes them up.
> If I can get everything reviewed my changes will be in the next merge
> window (it unfortunately always takes longer to get the code reviewed
> than I would like).
>
> However when my changes land does not matter. What you are trying to
> solve is orthogonal of my on-going work.
>
> The problem is that looking at shared signals is fundamentally broken.
> A case in point is that kernel threads can have a pending SIGKILL that
> is not a fatal signal. As kernel threads are allowed to ignore or even
> handle SIGKILL.
>
> If you want to change fatal_signal_pending to include PF_EXITING I would
> need to double check the implications but I think that would work, and
> would not have the problems including the shared pending state of
> SIGKILL.
I think that would work. I'll test it out, thanks.
> >> The other thing I have played with that might be relevant was removing
> >> the explicit wait in zap_pid_ns_processes and simply not allowing wait
> >> to reap the pid namespace init until all it's children had been reaped.
> >> Essentially how we deal with the thread group leader for ordinary
> >> processes. Does that sound like it might help in the fuse case?
> >
> > No, the problem is that the wait code doesn't know to look in the
> > right place, so waiting later still won't help.
>
> I was suggesting to modify the kernel so that zap_pid_ns_processes would
> not wait for the zapped processes. Instead I was proposing that
> delay_group_leader called from wait_consider_task would simply refuse to
> allow the init process of a pid namespace to be reaped until every other
> process of that pid namespace had exited.
>
> You can prototype how that would affect the deadlock by simply removing
> the waiting from zap_pid_ns_processes.
>
> I suggest that simply because that has the potential to remove some of
> the strange pid namespace cases.
>
> I don't understand the problematic interaction between pid namespace
> shutdown and the fuse daemon, so I am merely suggesting a possibility
> that I know can simplify pid namespace shutdown.
>
> Something like:
>
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index f4f8cb0435b4..d22a30b0b0cf 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -207,47 +207,6 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> read_unlock(&tasklist_lock);
> rcu_read_unlock();
>
> - /*
> - * Reap the EXIT_ZOMBIE children we had before we ignored SIGCHLD.
> - * kernel_wait4() will also block until our children traced from the
> - * parent namespace are detached and become EXIT_DEAD.
> - */
> - do {
> - clear_thread_flag(TIF_SIGPENDING);
> - rc = kernel_wait4(-1, NULL, __WALL, NULL);
> - } while (rc != -ECHILD);
> -
> - /*
> - * kernel_wait4() misses EXIT_DEAD children, and EXIT_ZOMBIE
> - * process whose parents processes are outside of the pid
> - * namespace. Such processes are created with setns()+fork().
> - *
> - * If those EXIT_ZOMBIE processes are not reaped by their
> - * parents before their parents exit, they will be reparented
> - * to pid_ns->child_reaper. Thus pidns->child_reaper needs to
> - * stay valid until they all go away.
> - *
> - * The code relies on the pid_ns->child_reaper ignoring
> - * SIGCHILD to cause those EXIT_ZOMBIE processes to be
> - * autoreaped if reparented.
> - *
> - * Semantically it is also desirable to wait for EXIT_ZOMBIE
> - * processes before allowing the child_reaper to be reaped, as
> - * that gives the invariant that when the init process of a
> - * pid namespace is reaped all of the processes in the pid
> - * namespace are gone.
> - *
> - * Once all of the other tasks are gone from the pid_namespace
> - * free_pid() will awaken this task.
> - */
> - for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
> - if (pid_ns->pid_allocated == init_pids)
> - break;
> - schedule();
> - }
> - __set_current_state(TASK_RUNNING);
> -
> if (pid_ns->reboot)
> current->signal->group_exit_code = pid_ns->reboot;
Yes, but we need to add the wait to delay_group_leader(), and if the
tasks are stuck indefinitely looking at the wrong condition, I don't
see how moving it will help resolve things.
Thanks,
Tycho
