Message-ID: <Ys2PwTS0qFmGNFqy@netflix>
Date:   Tue, 12 Jul 2022 09:14:09 -0600
From:   Tycho Andersen <tycho@...ho.pizza>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Miklos Szeredi <miklos@...redi.hu>,
        Christian Brauner <brauner@...nel.org>,
        fuse-devel <fuse-devel@...ts.sourceforge.net>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: strange interaction between fuse + pidns

On Tue, Jul 12, 2022 at 09:34:50AM -0500, Eric W. Biederman wrote:
> Tycho Andersen <tycho@...ho.pizza> writes:
> 
> > On Mon, Jul 11, 2022 at 06:06:21PM -0500, Eric W. Biederman wrote:
> >> Tycho Andersen <tycho@...ho.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

Powered by blists - more mailing lists