[3/3] rcu-tasks: Fix synchronize_rcu_tasks() VS zap_pid_ns_processes()

Commit Message

Frederic Weisbecker Nov. 25, 2022, 1:55 p.m. UTC

RCU Tasks and PID-namespace unshare can interact in do_exit() in a
complicated circular dependency:

1) TASK A calls unshare(CLONE_NEWPID), this creates a new PID namespace
   that every subsequent child of TASK A will belong to. But TASK A
   doesn't itself belong to that new PID namespace.

2) TASK A forks() and creates TASK B. TASK A stays attached to its PID
   namespace (let's say PID_NS1) and TASK B is the first task belonging
   to the new PID namespace created by unshare()  (let's call it PID_NS2).

3) Since TASK B is the first task attached to PID_NS2, it becomes the
   PID_NS2 child reaper.

4) TASK A forks() again and creates TASK C which get attached to PID_NS2.
   Note how TASK C has TASK A as a parent (belonging to PID_NS1) but has
   TASK B (belonging to PID_NS2) as a pid_namespace child_reaper.

5) TASK B exits and since it is the child reaper for PID_NS2, it has to
   kill all other tasks attached to PID_NS2, and wait for all of them to
   die before getting reaped itself (zap_pid_ns_process()).

6) TASK A calls synchronize_rcu_tasks() which leads to
   synchronize_srcu(&tasks_rcu_exit_srcu).

7) TASK B is waiting for TASK C to get reaped. But TASK B is under a
   tasks_rcu_exit_srcu SRCU critical section (exit_notify() is between
   exit_tasks_rcu_start() and exit_tasks_rcu_finish()), blocking TASK A.

8) TASK C exits and since TASK A is its parent, it waits for it to reap
   TASK C, but it can't because TASK A waits for TASK B that waits for
   TASK C.

Pid_namespace semantics can hardly be changed at this point. But the
coverage of tasks_rcu_exit_srcu can be reduced instead.

The current task is assumed not to be concurrently reapable at this
stage of exit_notify() and therefore tasks_rcu_exit_srcu can be
temporarily relaxed without breaking its constraints, providing a way
out of the deadlock scenario.

Fixes: 3f95aa81d265 ("rcu: Make TASKS_RCU handle tasks that are almost done exiting")
Reported-by: Pengfei Xu <[e-mail]>
Suggested-by: Boqun Feng <[e-mail]>
Suggested-by: Neeraj Upadhyay <[e-mail]>
Suggested-by: Paul E. McKenney <[e-mail]>
Cc: Oleg Nesterov <[e-mail]>
Cc: Lai Jiangshan <[e-mail]>
Cc: Eric W . Biederman <[e-mail]>
Signed-off-by: Frederic Weisbecker <[e-mail]>
---
 include/linux/rcupdate.h |  2 ++
 kernel/pid_namespace.c   | 17 +++++++++++++++++
 kernel/rcu/tasks.h       | 14 ++++++++++++--
 3 files changed, 31 insertions(+), 2 deletions(-)

Comments

Eric W. Biederman Nov. 30, 2022, 6:37 p.m. UTC | #1

Frederic Weisbecker <[e-mail]> writes:

> RCU Tasks and PID-namespace unshare can interact in do_exit() in a
> complicated circular dependency:
>
> 1) TASK A calls unshare(CLONE_NEWPID), this creates a new PID namespace
>    that every subsequent child of TASK A will belong to. But TASK A
>    doesn't itself belong to that new PID namespace.
>
> 2) TASK A forks() and creates TASK B. TASK A stays attached to its PID
>    namespace (let's say PID_NS1) and TASK B is the first task belonging
>    to the new PID namespace created by unshare()  (let's call it PID_NS2).
>
> 3) Since TASK B is the first task attached to PID_NS2, it becomes the
>    PID_NS2 child reaper.
>
> 4) TASK A forks() again and creates TASK C which get attached to PID_NS2.
>    Note how TASK C has TASK A as a parent (belonging to PID_NS1) but has
>    TASK B (belonging to PID_NS2) as a pid_namespace child_reaper.
>
> 5) TASK B exits and since it is the child reaper for PID_NS2, it has to
>    kill all other tasks attached to PID_NS2, and wait for all of them to
>    die before getting reaped itself (zap_pid_ns_process()).
>
> 6) TASK A calls synchronize_rcu_tasks() which leads to
>    synchronize_srcu(&tasks_rcu_exit_srcu).
>
> 7) TASK B is waiting for TASK C to get reaped. But TASK B is under a
>    tasks_rcu_exit_srcu SRCU critical section (exit_notify() is between
>    exit_tasks_rcu_start() and exit_tasks_rcu_finish()), blocking TASK A.
>
> 8) TASK C exits and since TASK A is its parent, it waits for it to reap
>    TASK C, but it can't because TASK A waits for TASK B that waits for
>    TASK C.
>
> Pid_namespace semantics can hardly be changed at this point. But the
> coverage of tasks_rcu_exit_srcu can be reduced instead.
>
> The current task is assumed not to be concurrently reapable at this
> stage of exit_notify() and therefore tasks_rcu_exit_srcu can be
> temporarily relaxed without breaking its constraints, providing a way
> out of the deadlock scenario.
>
> Fixes: 3f95aa81d265 ("rcu: Make TASKS_RCU handle tasks that are almost done exiting")
> Reported-by: Pengfei Xu <[e-mail]>
> Suggested-by: Boqun Feng <[e-mail]>
> Suggested-by: Neeraj Upadhyay <[e-mail]>
> Suggested-by: Paul E. McKenney <[e-mail]>
> Cc: Oleg Nesterov <[e-mail]>
> Cc: Lai Jiangshan <[e-mail]>
> Cc: Eric W . Biederman <[e-mail]>
> Signed-off-by: Frederic Weisbecker <[e-mail]>
> ---
>  include/linux/rcupdate.h |  2 ++
>  kernel/pid_namespace.c   | 17 +++++++++++++++++
>  kernel/rcu/tasks.h       | 14 ++++++++++++--
>  3 files changed, 31 insertions(+), 2 deletions(-)

> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index f4f8cb0435b4..fc21c5d5fd5d 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -244,7 +244,24 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
>  		set_current_state(TASK_INTERRUPTIBLE);
>  		if (pid_ns->pid_allocated == init_pids)
>  			break;
> +		/*
> +		 * Release tasks_rcu_exit_srcu to avoid following deadlock:
> +		 *
> +		 * 1) TASK A unshare(CLONE_NEWPID)
> +		 * 2) TASK A fork() twice -> TASK B (child reaper for new ns)
> +		 *    and TASK C
> +		 * 3) TASK B exits, kills TASK C, waits for TASK A to reap it
> +		 * 4) TASK A calls synchronize_rcu_tasks()
> +		 *                   -> synchronize_srcu(tasks_rcu_exit_srcu)
> +		 * 5) *DEADLOCK*
> +		 *
> +		 * It is considered safe to release tasks_rcu_exit_srcu here
> +		 * because we assume the current task can not be concurrently
> +		 * reaped at this point.
> +		 */
> +		exit_tasks_rcu_stop();
>  		schedule();
> +		exit_tasks_rcu_start();
>  	}
>  	__set_current_state(TASK_RUNNING);

Two questions.

1) Is there any chance you need the exit_task_rcu_stop() and
   exit_tasks_rcu_start() around schedule in the part of this code that
   calls kernel_wait4.

2) I keep thinking zap_pid_ns_processes() should be changed so that
   after it sends SIGKILL to all of the relevant processes to not wait,
   and instead have wait_consider_task simply not allow the 
   init process of the pid namespace to be reaped.

   Am I right in thinking that such a change were to be made it would
   make remove the deadlock without having to have any special code?

   It is just tricky enough to do that I don't want to discourage your
   simpler change but this looks like a case that makes the pain of
   changing zap_pid_ns_processes worthwhile in the practice.

Eric

Paul E. McKenney Dec. 2, 2022, 7:51 p.m. UTC | #2

On Wed, Nov 30, 2022 at 12:37:15PM -0600, Eric W. Biederman wrote:
> Frederic Weisbecker <[e-mail]> writes:
> 
> > RCU Tasks and PID-namespace unshare can interact in do_exit() in a
> > complicated circular dependency:
> >
> > 1) TASK A calls unshare(CLONE_NEWPID), this creates a new PID namespace
> >    that every subsequent child of TASK A will belong to. But TASK A
> >    doesn't itself belong to that new PID namespace.
> >
> > 2) TASK A forks() and creates TASK B. TASK A stays attached to its PID
> >    namespace (let's say PID_NS1) and TASK B is the first task belonging
> >    to the new PID namespace created by unshare()  (let's call it PID_NS2).
> >
> > 3) Since TASK B is the first task attached to PID_NS2, it becomes the
> >    PID_NS2 child reaper.
> >
> > 4) TASK A forks() again and creates TASK C which get attached to PID_NS2.
> >    Note how TASK C has TASK A as a parent (belonging to PID_NS1) but has
> >    TASK B (belonging to PID_NS2) as a pid_namespace child_reaper.
> >
> > 5) TASK B exits and since it is the child reaper for PID_NS2, it has to
> >    kill all other tasks attached to PID_NS2, and wait for all of them to
> >    die before getting reaped itself (zap_pid_ns_process()).
> >
> > 6) TASK A calls synchronize_rcu_tasks() which leads to
> >    synchronize_srcu(&tasks_rcu_exit_srcu).
> >
> > 7) TASK B is waiting for TASK C to get reaped. But TASK B is under a
> >    tasks_rcu_exit_srcu SRCU critical section (exit_notify() is between
> >    exit_tasks_rcu_start() and exit_tasks_rcu_finish()), blocking TASK A.
> >
> > 8) TASK C exits and since TASK A is its parent, it waits for it to reap
> >    TASK C, but it can't because TASK A waits for TASK B that waits for
> >    TASK C.
> >
> > Pid_namespace semantics can hardly be changed at this point. But the
> > coverage of tasks_rcu_exit_srcu can be reduced instead.
> >
> > The current task is assumed not to be concurrently reapable at this
> > stage of exit_notify() and therefore tasks_rcu_exit_srcu can be
> > temporarily relaxed without breaking its constraints, providing a way
> > out of the deadlock scenario.
> >
> > Fixes: 3f95aa81d265 ("rcu: Make TASKS_RCU handle tasks that are almost done exiting")
> > Reported-by: Pengfei Xu <[e-mail]>
> > Suggested-by: Boqun Feng <[e-mail]>
> > Suggested-by: Neeraj Upadhyay <[e-mail]>
> > Suggested-by: Paul E. McKenney <[e-mail]>
> > Cc: Oleg Nesterov <[e-mail]>
> > Cc: Lai Jiangshan <[e-mail]>
> > Cc: Eric W . Biederman <[e-mail]>
> > Signed-off-by: Frederic Weisbecker <[e-mail]>
> > ---
> >  include/linux/rcupdate.h |  2 ++
> >  kernel/pid_namespace.c   | 17 +++++++++++++++++
> >  kernel/rcu/tasks.h       | 14 ++++++++++++--
> >  3 files changed, 31 insertions(+), 2 deletions(-)
> 
> > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> > index f4f8cb0435b4..fc21c5d5fd5d 100644
> > --- a/kernel/pid_namespace.c
> > +++ b/kernel/pid_namespace.c
> > @@ -244,7 +244,24 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> >  		set_current_state(TASK_INTERRUPTIBLE);
> >  		if (pid_ns->pid_allocated == init_pids)
> >  			break;
> > +		/*
> > +		 * Release tasks_rcu_exit_srcu to avoid following deadlock:
> > +		 *
> > +		 * 1) TASK A unshare(CLONE_NEWPID)
> > +		 * 2) TASK A fork() twice -> TASK B (child reaper for new ns)
> > +		 *    and TASK C
> > +		 * 3) TASK B exits, kills TASK C, waits for TASK A to reap it
> > +		 * 4) TASK A calls synchronize_rcu_tasks()
> > +		 *                   -> synchronize_srcu(tasks_rcu_exit_srcu)
> > +		 * 5) *DEADLOCK*
> > +		 *
> > +		 * It is considered safe to release tasks_rcu_exit_srcu here
> > +		 * because we assume the current task can not be concurrently
> > +		 * reaped at this point.
> > +		 */
> > +		exit_tasks_rcu_stop();
> >  		schedule();
> > +		exit_tasks_rcu_start();
> >  	}
> >  	__set_current_state(TASK_RUNNING);
> 
> Two questions.
> 
> 1) Is there any chance you need the exit_task_rcu_stop() and
>    exit_tasks_rcu_start() around schedule in the part of this code that
>    calls kernel_wait4.

Quite possibly, but I must defer to Frederic on this one.

> 2) I keep thinking zap_pid_ns_processes() should be changed so that
>    after it sends SIGKILL to all of the relevant processes to not wait,
>    and instead have wait_consider_task simply not allow the 
>    init process of the pid namespace to be reaped.
> 
>    Am I right in thinking that such a change were to be made it would
>    make remove the deadlock without having to have any special code?
> 
>    It is just tricky enough to do that I don't want to discourage your
>    simpler change but this looks like a case that makes the pain of
>    changing zap_pid_ns_processes worthwhile in the practice.

I would dearly love for there to be a fix that allowed the RCU-related
code to go back to what it was originally.  But there is apparently some
concern that users might be relying on the current sleep-while-exiting
semantics.  :-/

							Thanx, Paul

Frederic Weisbecker Dec. 2, 2022, 10:54 p.m. UTC | #3

On Wed, Nov 30, 2022 at 12:37:15PM -0600, Eric W. Biederman wrote:
> Frederic Weisbecker <[e-mail]> writes:
> Two questions.
> 
> 1) Is there any chance you need the exit_task_rcu_stop() and
>    exit_tasks_rcu_start() around schedule in the part of this code that
>    calls kernel_wait4.

Indeed it could be relaxed there too if necessary.

> 
> 2) I keep thinking zap_pid_ns_processes() should be changed so that
>    after it sends SIGKILL to all of the relevant processes to not wait,
>    and instead have wait_consider_task simply not allow the 
>    init process of the pid namespace to be reaped.
> 
>    Am I right in thinking that such a change were to be made it would
>    make remove the deadlock without having to have any special code?
> 
>    It is just tricky enough to do that I don't want to discourage your
>    simpler change but this looks like a case that makes the pain of
>    changing zap_pid_ns_processes worthwhile in the practice.

So you mean it still reaps those that were EXIT_ZOMBIE before ignoring
SIGCHLD (the kernel_wait4() call) but it doesn't sleep anymore on those
that autoreap (or get reaped by a parent outside that namespace) after
ignoring SIGCHLD? Namely it doesn't do the schedule() loop I'm working
around here and proceeds with exit_notify() and notifies its parent?

And then in this case the responsibility of sleeping, until the init_process
of the namespace is the last task in the namespace, goes to the parent while
waiting that init_process, right?

But what if the init_process of the given namespace autoreaps? Should it then
wait itself until the namespace is empty? And then aren't we back to the initial
issue?

Thanks.

Eric W. Biederman Dec. 2, 2022, 11:28 p.m. UTC | #4

Frederic Weisbecker <[e-mail]> writes:

> On Wed, Nov 30, 2022 at 12:37:15PM -0600, Eric W. Biederman wrote:
>> Frederic Weisbecker <[e-mail]> writes:
>> Two questions.
>> 
>> 1) Is there any chance you need the exit_task_rcu_stop() and
>>    exit_tasks_rcu_start() around schedule in the part of this code that
>>    calls kernel_wait4.
>
> Indeed it could be relaxed there too if necessary.

I was just wondering how you tell if it is necessary and if perhaps
the kernel_wait4 was overlooked.

>> 2) I keep thinking zap_pid_ns_processes() should be changed so that
>>    after it sends SIGKILL to all of the relevant processes to not wait,
>>    and instead have wait_consider_task simply not allow the 
>>    init process of the pid namespace to be reaped.
>> 
>>    Am I right in thinking that such a change were to be made it would
>>    make remove the deadlock without having to have any special code?
>> 
>>    It is just tricky enough to do that I don't want to discourage your
>>    simpler change but this looks like a case that makes the pain of
>>    changing zap_pid_ns_processes worthwhile in the practice.
>
> So you mean it still reaps those that were EXIT_ZOMBIE before ignoring
> SIGCHLD (the kernel_wait4() call) but it doesn't sleep anymore on those
> that autoreap (or get reaped by a parent outside that namespace) after
> ignoring SIGCHLD? Namely it doesn't do the schedule() loop I'm working
> around here and proceeds with exit_notify() and notifies its parent?

Yes.  A change to make it work like when the thread group leader exits
before the other threads.  So any actual sleeping happens in the reaper
of the init process when the reaper calls wait(2).

> And then in this case the responsibility of sleeping, until the init_process
> of the namespace is the last task in the namespace, goes to the parent while
> waiting that init_process, right?
>
> But what if the init_process of the given namespace autoreaps? Should it then
> wait itself until the namespace is empty? And then aren't we back to the initial
> issue?

The idea is that we only care when the userspace cares.  I don't think
there is any kernel code that fundamentally cares.  There might be a few
bits that accidentally care and those would need to be take care of when
making the proposed change.  The wait would only exist for userspace so
the same semantics are observed.

Eric

Frederic Weisbecker Dec. 4, 2022, 12:03 a.m. UTC | #5

On Fri, Dec 02, 2022 at 05:28:54PM -0600, Eric W. Biederman wrote:
> Frederic Weisbecker <[e-mail]> writes:
> 
> > On Wed, Nov 30, 2022 at 12:37:15PM -0600, Eric W. Biederman wrote:
> >> Frederic Weisbecker <[e-mail]> writes:
> >> Two questions.
> >> 
> >> 1) Is there any chance you need the exit_task_rcu_stop() and
> >>    exit_tasks_rcu_start() around schedule in the part of this code that
> >>    calls kernel_wait4.
> >
> > Indeed it could be relaxed there too if necessary.
> 
> I was just wondering how you tell if it is necessary and if perhaps
> the kernel_wait4 was overlooked.

As for the deadlock described in this patch, it involves waiting for
another task to reap yet another task. So it doesn't look necessary to
relax the lock when the current task does the reaping.


Now the following looks possible:

      TASK A                                    TASK B                       TASK C
      -----                                   -------                       ------
      mutex_lock(&lock1)
      synchronize_srcu(tasks_rcu_exit_srcu)
                                                                            mutex_lock(&lock1)
                                                                            //wait for TASK A to release &lock1
                                              exit_tasks_rcu_start();
                                              ...
                                              zap_pid_ns_processes()
                                                  kernel_wait4()
                                                      //wait for TASK C

> > So you mean it still reaps those that were EXIT_ZOMBIE before ignoring
> > SIGCHLD (the kernel_wait4() call) but it doesn't sleep anymore on those
> > that autoreap (or get reaped by a parent outside that namespace) after
> > ignoring SIGCHLD? Namely it doesn't do the schedule() loop I'm working
> > around here and proceeds with exit_notify() and notifies its parent?
> 
> Yes.  A change to make it work like when the thread group leader exits
> before the other threads.  So any actual sleeping happens in the reaper
> of the init process when the reaper calls wait(2).
> 
> > And then in this case the responsibility of sleeping, until the init_process
> > of the namespace is the last task in the namespace, goes to the parent while
> > waiting that init_process, right?
> >
> > But what if the init_process of the given namespace autoreaps? Should it then
> > wait itself until the namespace is empty? And then aren't we back to the initial
> > issue?
> 
> The idea is that we only care when the userspace cares.  I don't think
> there is any kernel code that fundamentally cares.  There might be a few
> bits that accidentally care and those would need to be take care of when
> making the proposed change.  The wait would only exist for userspace so
> the same semantics are observed.

I mean the issue is that if the pid_namespace reaper goes for autoreaping, then
it still has to go through that loop waiting for the remaining tasks of the
pid_namespace to be reaped. The loop will just happen later in exit_notify() but
eventually the issue remains: we'll have to relax tasks_rcu_exit_srcu
around that loop.

Thanks.

> 
> Eric

Oleg Nesterov Dec. 6, 2022, 4:49 p.m. UTC | #6

On 11/30, Eric W. Biederman wrote:
>
> 2) I keep thinking zap_pid_ns_processes() should be changed so that
>    after it sends SIGKILL to all of the relevant processes to not wait,

At least I think it should not wait for the tasks injected into this ns.

Because this looks like a kernel bug even if we forget about this deadlock.

Say we create a task P using clone(CLONE_NEWPID), then inject a task T into
P's pid-namespace via setns/fork. This make the process P "unkillable", it
will hang in zap_pid_ns_processes() "forever" until T->parent reaps a zombie
task T killed by P.

Oleg.

Paul E. McKenney Dec. 7, 2022, 2:34 p.m. UTC | #7

On Tue, Dec 06, 2022 at 05:49:28PM +0100, Oleg Nesterov wrote:
> On 11/30, Eric W. Biederman wrote:
> >
> > 2) I keep thinking zap_pid_ns_processes() should be changed so that
> >    after it sends SIGKILL to all of the relevant processes to not wait,
> 
> At least I think it should not wait for the tasks injected into this ns.
> 
> Because this looks like a kernel bug even if we forget about this deadlock.
> 
> Say we create a task P using clone(CLONE_NEWPID), then inject a task T into
> P's pid-namespace via setns/fork. This make the process P "unkillable", it
> will hang in zap_pid_ns_processes() "forever" until T->parent reaps a zombie
> task T killed by P.

Given that this is not the first time that Tasks Trace RCU has been
involved in a deadlock involving this exit path, I could certainly get
behind this approach.  ;-)

							Thanx, Paul

Frederic Weisbecker Dec. 7, 2022, 8:01 p.m. UTC | #8

On Tue, Dec 06, 2022 at 05:49:28PM +0100, Oleg Nesterov wrote:
> On 11/30, Eric W. Biederman wrote:
> >
> > 2) I keep thinking zap_pid_ns_processes() should be changed so that
> >    after it sends SIGKILL to all of the relevant processes to not wait,
> 
> At least I think it should not wait for the tasks injected into this ns.
> 
> Because this looks like a kernel bug even if we forget about this deadlock.
> 
> Say we create a task P using clone(CLONE_NEWPID), then inject a task T into
> P's pid-namespace via setns/fork. This make the process P "unkillable", it
> will hang in zap_pid_ns_processes() "forever" until T->parent reaps a zombie
> task T killed by P.

I think this was made that way on purpose, see the comment in
zap_pid_ns_processes():

	/*
	 * 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.

I can't say I like the fact that a parent not belonging to a new namespace
can create more than one child within that namespace but anyway this all look
like an ABI that can't be reverted now.

Thanks.

Oleg Nesterov Dec. 7, 2022, 8:39 p.m. UTC | #9

Frederic,

before anything else, your patches look good to me, it is not that
I tried to argue!

On 12/07, Frederic Weisbecker wrote:
>
> On Tue, Dec 06, 2022 at 05:49:28PM +0100, Oleg Nesterov wrote:
> >
> > At least I think it should not wait for the tasks injected into this ns.
> >
> > Because this looks like a kernel bug even if we forget about this deadlock.
> >
> I think this was made that way on purpose,

Well maybe. But to me we have this behaviour only because we (me at least)
do not know how to avoid the "hang" in this case.

> see the comment in zap_pid_ns_processes():

Heh ;) I wrote this comment in a53b83154914 ("exit: pidns: fix/update the
comments in zap_pid_ns_processes()") exactly because I didn't like this
behaviour, but I thought it must be documented.

> I can't say I like the fact that a parent not belonging to a new namespace
> can create more than one child within that namespace

not sure I understand but this looks fine and useful to me,

> but anyway this all look like an ABI that can't be reverted now.

perhaps... But you know, I wrote my previous email because 2 weeks ago
I had to investigate a bug report which blamed the kernel, while the
problem (unkillable process sleeping in zap_pid_ns_processes) was caused
by the dangling zombie injected into that process's namespace. And I am
still trying to convince the customer they need to fix userspace.

Thanks,

Oleg.

Frederic Weisbecker Dec. 9, 2022, 8:26 p.m. UTC | #10

On Wed, Dec 07, 2022 at 09:39:00PM +0100, Oleg Nesterov wrote:
> On 12/07, Frederic Weisbecker wrote:
> >
> > On Tue, Dec 06, 2022 at 05:49:28PM +0100, Oleg Nesterov wrote:
> > >
> > > At least I think it should not wait for the tasks injected into this ns.
> > >
> > > Because this looks like a kernel bug even if we forget about this deadlock.
> > >
> > I think this was made that way on purpose,
> 
> Well maybe. But to me we have this behaviour only because we (me at least)
> do not know how to avoid the "hang" in this case.
> 
> > see the comment in zap_pid_ns_processes():
> 
> Heh ;) I wrote this comment in a53b83154914 ("exit: pidns: fix/update the
> comments in zap_pid_ns_processes()") exactly because I didn't like this
> behaviour, but I thought it must be documented.

Bah! I should have guessed ;-)

> 
> > I can't say I like the fact that a parent not belonging to a new namespace
> > can create more than one child within that namespace
> 
> not sure I understand but this looks fine and useful to me,

I mean if only one task could be injected within a new namespace, we could be sure
that all subsequent tasks belonging to that namespace would be descendents of
that first task (the same way that every task in the default namespace is a
descendant of the real init_task) and thus we wouldn't be bothered with such
deadlocks. But I guess namespaces aren't designed to work like that. I don't
know much about them so what I'm saying is very likely irrelevant.

> > but anyway this all look like an ABI that can't be reverted now.
> 
> perhaps... But you know, I wrote my previous email because 2 weeks ago
> I had to investigate a bug report which blamed the kernel, while the
> problem (unkillable process sleeping in zap_pid_ns_processes) was caused
> by the dangling zombie injected into that process's namespace. And I am
> still trying to convince the customer they need to fix userspace.

Heh :-/

I wish we could fix this but I have no idea how. I guess the child_reaper
of an ns could avoid waiting for the rest of the ns and designate its parent
as the new child reaper.

Or we could arrange for all tasks in the ns to autoreap if they ever fall back
to be reaped by their ns->child_reaper and that child_reaper is dead.

But that would look like ABI breakages...

Thanks.

Patch

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 89b3036746d2..a19d91d5461c 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -238,6 +238,7 @@  void synchronize_rcu_tasks_rude(void);
 
 #define rcu_note_voluntary_context_switch(t) rcu_tasks_qs(t, false)
 void exit_tasks_rcu_start(void);
+void exit_tasks_rcu_stop(void);
 void exit_tasks_rcu_finish(void);
 #else /* #ifdef CONFIG_TASKS_RCU_GENERIC */
 #define rcu_tasks_classic_qs(t, preempt) do { } while (0)
@@ -246,6 +247,7 @@  void exit_tasks_rcu_finish(void);
 #define call_rcu_tasks call_rcu
 #define synchronize_rcu_tasks synchronize_rcu
 static inline void exit_tasks_rcu_start(void) { }
+static inline void exit_tasks_rcu_stop(void) { }
 static inline void exit_tasks_rcu_finish(void) { }
 #endif /* #else #ifdef CONFIG_TASKS_RCU_GENERIC */
 
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index f4f8cb0435b4..fc21c5d5fd5d 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -244,7 +244,24 @@  void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 		set_current_state(TASK_INTERRUPTIBLE);
 		if (pid_ns->pid_allocated == init_pids)
 			break;
+		/*
+		 * Release tasks_rcu_exit_srcu to avoid following deadlock:
+		 *
+		 * 1) TASK A unshare(CLONE_NEWPID)
+		 * 2) TASK A fork() twice -> TASK B (child reaper for new ns)
+		 *    and TASK C
+		 * 3) TASK B exits, kills TASK C, waits for TASK A to reap it
+		 * 4) TASK A calls synchronize_rcu_tasks()
+		 *                   -> synchronize_srcu(tasks_rcu_exit_srcu)
+		 * 5) *DEADLOCK*
+		 *
+		 * It is considered safe to release tasks_rcu_exit_srcu here
+		 * because we assume the current task can not be concurrently
+		 * reaped at this point.
+		 */
+		exit_tasks_rcu_stop();
 		schedule();
+		exit_tasks_rcu_start();
 	}
 	__set_current_state(TASK_RUNNING);
 
diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 9a8114114b48..f9dfc2ece287 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1016,12 +1016,22 @@  void exit_tasks_rcu_start(void) __acquires(&tasks_rcu_exit_srcu)
  * task is exiting and may be removed from the tasklist. See
  * corresponding synchronize_srcu() for further details.
  */
-void exit_tasks_rcu_finish(void) __releases(&tasks_rcu_exit_srcu)
+void exit_tasks_rcu_stop(void) __releases(&tasks_rcu_exit_srcu)
 {
 	struct task_struct *t = current;
 
 	__srcu_read_unlock(&tasks_rcu_exit_srcu, t->rcu_tasks_idx);
-	exit_tasks_rcu_finish_trace(t);
+}
+
+/*
+ * Contribute to protect against tasklist scan blind spot while the
+ * task is exiting and may be removed from the tasklist. See
+ * corresponding synchronize_srcu() for further details.
+ */
+void exit_tasks_rcu_finish(void)
+{
+	exit_tasks_rcu_stop();
+	exit_tasks_rcu_finish_trace(current);
 }
 
 #else /* #ifdef CONFIG_TASKS_RCU */