This reverts commit 28319d6dc5e2ffefa452c2377dd0f71621b5bff0. The race
it fixed was subject to conditions that don't exist anymore since:
1612160b9127 ("rcu-tasks: Eliminate deadlocks involving do_exit() and RCU tasks")
This latter commit removes the use of SRCU that used to cover the
RCU-tasks blind spot on exit between the tasklist's removal and the
final preemption disabling. The task is now placed instead into a
temporary list inside which voluntary sleeps are accounted as RCU-tasks
quiescent states. This would disarm the deadlock initially reported
against PID namespace exit.
Signed-off-by: Frederic Weisbecker <[email protected]>
---
include/linux/rcupdate.h | 2 --
kernel/pid_namespace.c | 17 -----------------
kernel/rcu/tasks.h | 16 +++-------------
3 files changed, 3 insertions(+), 32 deletions(-)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 8470a85f6563..be450a3477be 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -209,7 +209,6 @@ 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)
@@ -218,7 +217,6 @@ 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 7ade20e95232..20a0b3ceb064 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -248,24 +248,7 @@ 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 445561ee4d22..8c9538b555e3 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -858,7 +858,7 @@ static void rcu_tasks_wait_gp(struct rcu_tasks *rtp)
// not know to synchronize with this RCU Tasks grace period) have
// completed exiting. The synchronize_rcu() in rcu_tasks_postgp()
// will take care of any tasks stuck in the non-preemptible region
-// of do_exit() following its call to exit_tasks_rcu_stop().
+// of do_exit() following its call to exit_tasks_rcu_finish().
// check_all_holdout_tasks(), repeatedly until holdout list is empty:
// Scans the holdout list, attempting to identify a quiescent state
// for each task on the list. If there is a quiescent state, the
@@ -1220,7 +1220,7 @@ void exit_tasks_rcu_start(void)
* Remove the task from the "yet another list" because do_exit() is now
* non-preemptible, allowing synchronize_rcu() to wait beyond this point.
*/
-void exit_tasks_rcu_stop(void)
+void exit_tasks_rcu_finish(void)
{
unsigned long flags;
struct rcu_tasks_percpu *rtpcp;
@@ -1231,22 +1231,12 @@ void exit_tasks_rcu_stop(void)
raw_spin_lock_irqsave_rcu_node(rtpcp, flags);
list_del_init(&t->rcu_tasks_exit_list);
raw_spin_unlock_irqrestore_rcu_node(rtpcp, flags);
-}
-/*
- * 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);
+ exit_tasks_rcu_finish_trace(t);
}
#else /* #ifdef CONFIG_TASKS_RCU */
void exit_tasks_rcu_start(void) { }
-void exit_tasks_rcu_stop(void) { }
void exit_tasks_rcu_finish(void) { exit_tasks_rcu_finish_trace(current); }
#endif /* #else #ifdef CONFIG_TASKS_RCU */
--
2.44.0
On 04/25, Frederic Weisbecker wrote:
>
> This reverts commit 28319d6dc5e2ffefa452c2377dd0f71621b5bff0. The race
> it fixed was subject to conditions that don't exist anymore since:
>
> 1612160b9127 ("rcu-tasks: Eliminate deadlocks involving do_exit() and RCU tasks")
>
> This latter commit removes the use of SRCU that used to cover the
> RCU-tasks blind spot on exit between the tasklist's removal and the
> final preemption disabling. The task is now placed instead into a
> temporary list inside which voluntary sleeps are accounted as RCU-tasks
> quiescent states. This would disarm the deadlock initially reported
> against PID namespace exit.
>
> Signed-off-by: Frederic Weisbecker <[email protected]>
> ---
> include/linux/rcupdate.h | 2 --
> kernel/pid_namespace.c | 17 -----------------
> kernel/rcu/tasks.h | 16 +++-------------
> 3 files changed, 3 insertions(+), 32 deletions(-)
LGTM. FWIW,
Reviewed-by: Oleg Nesterov <[email protected]>
On Mon, Apr 29, 2024 at 01:03:28PM +0200, Oleg Nesterov wrote:
> On 04/25, Frederic Weisbecker wrote:
> >
> > This reverts commit 28319d6dc5e2ffefa452c2377dd0f71621b5bff0. The race
> > it fixed was subject to conditions that don't exist anymore since:
> >
> > 1612160b9127 ("rcu-tasks: Eliminate deadlocks involving do_exit() and RCU tasks")
> >
> > This latter commit removes the use of SRCU that used to cover the
> > RCU-tasks blind spot on exit between the tasklist's removal and the
> > final preemption disabling. The task is now placed instead into a
> > temporary list inside which voluntary sleeps are accounted as RCU-tasks
> > quiescent states. This would disarm the deadlock initially reported
> > against PID namespace exit.
> >
> > Signed-off-by: Frederic Weisbecker <[email protected]>
> > ---
> > include/linux/rcupdate.h | 2 --
> > kernel/pid_namespace.c | 17 -----------------
> > kernel/rcu/tasks.h | 16 +++-------------
> > 3 files changed, 3 insertions(+), 32 deletions(-)
>
> LGTM. FWIW,
>
> Reviewed-by: Oleg Nesterov <[email protected]>
Queued, thank you both!
Thanx, Paul