Skip to contentSkip to conversation

sched,kthread: Fix TASK_PARKED and special sleep states

PATCH In review Peter Zijlstra 2 participants 9 messages Apr 30, 2018 – May 1, 2018 Scheduler

lore
Share & tools
lore

Peter Zijlstra

author maintainer
#0 lore raw

cover sched,kthread: Fix TASK_PARKED and special sleep states

Hi,

The first patch should fix the reported TASK_PARKED problem and the second should fix any potential TASK_STOPPED, TASK_TRACED issues.

author maintainer
#1 lore raw

PATCH 1/2 kthread: Fix kthread_parkme() wait-loop

Gaurav reported a problem with __kthread_parkme() where a concurrent try_to_wake_up() could result in competing stores to ->state which, when the TASK_PARKED store got lost bad things would happen.

The comment near set_current_state() actually mentions this competing store, but only mentions the case against TASK_RUNNING. This same store, with different timing, can happen against a subsequent !RUNNING store.

This normally is not a problem, because as per that same comment, the !RUNNING state store is inside a condition based wait-loop:

  for (;;) {
    set_current_state(TASK_UNINTERRUPTIBLE);
    if (!need_sleep)
      break;
    schedule();
  }
  __set_current_state(TASK_RUNNING);

If we loose the (first) TASK_UNINTERRUPTIBLE store to a previous (concurrent) wakeup, the schedule() will NO-OP and we'll go around the loop once more.

The problem here is that the TASK_PARKED store is not inside the KTHREAD_SHOULD_PARK condition wait-loop.

There is a genuine issue with sleeps that do not have a condition; this is addressed in a subsequent patch.

---
 kernel/kthread.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
kernel/kthread.c +4 -3
@@ -177,12 +177,13 @@ void *kthread_probe_data(struct task_str
 
 static void __kthread_parkme(struct kthread *self)
 {
-	__set_current_state(TASK_PARKED);
-	while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
+	for (;;) {
+		set_current_state(TASK_PARKED);
+		if (!test_bit(KTHREAD_SHOULD_PARK, &self->flags))
+			break;
 		if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
 			complete(&self->parked);
 		schedule();
-		__set_current_state(TASK_PARKED);
 	}
 	clear_bit(KTHREAD_IS_PARKED, &self->flags);
 	__set_current_state(TASK_RUNNING);
author maintainer
#2 lore raw

PATCH 2/2 sched: Introduce set_special_state()

In reply to Peter Zijlstra

Gaurav reported a perceived problem with TASK_PARKED, which turned out to be a broken wait-loop pattern in __kthread_parkme(), but the reported issue can (and does) in fact happen for states that do not do condition based sleeps.

When the 'current->state = TASK_RUNNING' store of a previous (concurrent) try_to_wake_up() collides with the setting of a 'special' sleep state, we can loose the sleep state.

Normal condition based wait-loops are immune to this problem, but for sleep states that are not condition based are subject to this problem.

There already is a fix for TASK_DEAD. Abstract that and also apply it to TASK_STOPPED and TASK_TRACED, both of which are also without condition based wait-loop.

---
 include/linux/sched.h        |   52 ++++++++++++++++++++++++++++++++++++++-----
 include/linux/sched/signal.h |    2 -
 kernel/sched/core.c          |   17 --------------
 kernel/signal.c              |    4 +--
 4 files changed, 51 insertions(+), 24 deletions(-)
include/linux/sched.h +47 -5
@@ -112,17 +112,38 @@ struct task_group;
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
+/*
+ * Special states are those that do not use the normal wait-loop pattern. See
+ * the comment with set_special_state().
+ */
+#define is_special_task_state(state)	\
+	((state) == TASK_DEAD    ||	\
+	 (state) == TASK_STOPPED ||	\
+	 (state) == TASK_TRACED)
+
 #define __set_current_state(state_value)			\
 	do {							\
+		WARN_ON_ONCE(is_special_task_state(state));	\
 		current->task_state_change = _THIS_IP_;		\
 		current->state = (state_value);			\
 	} while (0)
+
 #define set_current_state(state_value)				\
 	do {							\
+		WARN_ON_ONCE(is_special_task_state(state));	\
 		current->task_state_change = _THIS_IP_;		\
 		smp_store_mb(current->state, (state_value));	\
 	} while (0)
 
+#define set_special_state(state_value)					\
+	do {								\
+		unsigned long flags; /* may shadow */			\
+		WARN_ON_ONCE(!is_special_task_state(state_value));	\
+		raw_spin_lock_irqsave(&current->pi_lock, flags);	\
+		current->task_state_change = _THIS_IP_;			\
+		current->state = (state_value);				\
+		raw_spin_unlock_irqrestore(&current->pi_lock, flags);	\
+	} while (0)
 #else
 /*
  * set_current_state() includes a barrier so that the write of current->state
@@ -144,8 +165,8 @@ struct task_group;
  *
  * The above is typically ordered against the wakeup, which does:
  *
- *	need_sleep = false;
- *	wake_up_state(p, TASK_UNINTERRUPTIBLE);
+ *   need_sleep = false;
+ *   wake_up_state(p, TASK_UNINTERRUPTIBLE);
  *
  * Where wake_up_state() (and all other wakeup primitives) imply enough
  * barriers to order the store of the variable against wakeup.
@@ -154,12 +175,33 @@ struct task_group;
  * once it observes the TASK_UNINTERRUPTIBLE store the waking CPU can issue a
  * TASK_RUNNING store which can collide with __set_current_state(TASK_RUNNING).
  *
- * This is obviously fine, since they both store the exact same value.
+ * However, with slightly different timing the wakeup TASK_RUNNING store can
+ * also collide with the TASK_UNINTERRUPTIBLE store. Loosing that store is not
+ * a problem either because that will result in one extra go around the loop
+ * and our @cond test will save the day.
  *
  * Also see the comments of try_to_wake_up().
  */
-#define __set_current_state(state_value) do { current->state = (state_value); } while (0)
-#define set_current_state(state_value)	 smp_store_mb(current->state, (state_value))
+#define __set_current_state(state_value)				\
+	current->state = (state_value)
+
+#define set_current_state(state_value)					\
+	smp_store_mb(current->state, (state_value))
+
+/*
+ * set_special_state() should be used for those states when the blocking task
+ * can not use the regular condition based wait-loop. In that case we must
+ * serialize against wakeups such that any possible in-flight TASK_RUNNING stores
+ * will not collide with our state change.
+ */
+#define set_special_state(state_value)					\
+	do {								\
+		unsigned long flags; /* may shadow */			\
+		raw_spin_lock_irqsave(&current->pi_lock, flags);	\
+		current->state = (state_value);				\
+		raw_spin_unlock_irqrestore(&current->pi_lock, flags);	\
+	} while (0)
+
 #endif
 
 /* Task command name length: */
include/linux/sched/signal.h +1 -1
@@ -280,7 +280,7 @@ static inline void kernel_signal_stop(vo
 {
 	spin_lock_irq(&current->sighand->siglock);
 	if (current->jobctl & JOBCTL_STOP_DEQUEUED)
-		__set_current_state(TASK_STOPPED);
+		set_special_state(TASK_STOPPED);
 	spin_unlock_irq(&current->sighand->siglock);
 
 	schedule();
kernel/sched/core.c +1 -16
@@ -3498,23 +3498,8 @@ static void __sched notrace __schedule(b
 
 void __noreturn do_task_dead(void)
 {
-	/*
-	 * The setting of TASK_RUNNING by try_to_wake_up() may be delayed
-	 * when the following two conditions become true.
-	 *   - There is race condition of mmap_sem (It is acquired by
-	 *     exit_mm()), and
-	 *   - SMI occurs before setting TASK_RUNINNG.
-	 *     (or hypervisor of virtual machine switches to other guest)
-	 *  As a result, we may become TASK_RUNNING after becoming TASK_DEAD
-	 *
-	 * To avoid it, we have to wait for releasing tsk->pi_lock which
-	 * is held by try_to_wake_up()
-	 */
-	raw_spin_lock_irq(&current->pi_lock);
-	raw_spin_unlock_irq(&current->pi_lock);
-
 	/* Causes final put_task_struct in finish_task_switch(): */
-	__set_current_state(TASK_DEAD);
+	set_special_state(TASK_DEAD);
 
 	/* Tell freezer to ignore us: */
 	current->flags |= PF_NOFREEZE;
kernel/signal.c +2 -2
@@ -1968,7 +1968,7 @@ static void ptrace_stop(int exit_code, i
 	 * atomic with respect to siglock and should be done after the arch
 	 * hook as siglock is released and regrabbed across it.
 	 */
-	set_current_state(TASK_TRACED);
+	set_special_state(TASK_TRACED);
 
 	current->last_siginfo = info;
 	current->exit_code = exit_code;
@@ -2176,7 +2176,7 @@ static bool do_signal_stop(int signr)
 		if (task_participate_group_stop(current))
 			notify = CLD_STOPPED;
 
-		__set_current_state(TASK_STOPPED);
+		set_special_state(TASK_STOPPED);
 		spin_unlock_irq(&current->sighand->siglock);
 
 		/*
Reviewed-by on v1 2/2
#3 lore raw

Re: [PATCH 2/2] sched: Introduce set_special_state()

kernel/signal.c static void ptrace_stop(int exit_code, i · line 1971

Quoted 9 lines · Peter Zijlstra
kernel/signal.c +1 -1
@@ -1968,7 +1968,7 @@ static void ptrace_stop(int exit_code, i
 	 * atomic with respect to siglock and should be done after the arch
 	 * hook as siglock is released and regrabbed across it.
 	 */
-	set_current_state(TASK_TRACED);
+	set_special_state(TASK_TRACED);

Yes, but please note the comment above, we need a barrier after state = TASK_TRACED, that is why ptrace_stop() does set_current_state(), not __set_current_state().

Otherwise both patches look good to me, feel free to add

author maintainer
#4 lore raw

Re: [PATCH 2/2] sched: Introduce set_special_state()

Quoted 13 lines · Oleg Nesterov

kernel/signal.c static void ptrace_stop(int exit_code, i · line 1971

Quoted 9 lines · Peter Zijlstra
kernel/signal.c +1 -1
@@ -1968,7 +1968,7 @@ static void ptrace_stop(int exit_code, i
 	 * atomic with respect to siglock and should be done after the arch
 	 * hook as siglock is released and regrabbed across it.
 	 */
-	set_current_state(TASK_TRACED);
+	set_special_state(TASK_TRACED);

Yes, but please note the comment above, we need a barrier after state = TASK_TRACED, that is why ptrace_stop() does set_current_state(), not __set_current_state().

OK, so I got properly lost in that stuff.

The comment says it we need to set TASK_TRACED before clearing JOBCTL_TRAPPING because of do_wait(), but I cannot seem to locate code in do_wait() and below that cares about JOBCTL_TRAPPING.

Could you elucidate my tired brain?

author maintainer
#5 lore raw

Re: [PATCH 2/2] sched: Introduce set_special_state()

Quoted 15 lines · Peter Zijlstra
Quoted 13 lines · Oleg Nesterov

kernel/signal.c static void ptrace_stop(int exit_code, i · line 1971

Quoted 9 lines · Peter Zijlstra
kernel/signal.c +1 -1
@@ -1968,7 +1968,7 @@ static void ptrace_stop(int exit_code, i
 	 * atomic with respect to siglock and should be done after the arch
 	 * hook as siglock is released and regrabbed across it.
 	 */
-	set_current_state(TASK_TRACED);
+	set_special_state(TASK_TRACED);

Yes, but please note the comment above, we need a barrier after state = TASK_TRACED, that is why ptrace_stop() does set_current_state(), not __set_current_state().

Quoted 7 lines

OK, so I got properly lost in that stuff.

The comment says it we need to set TASK_TRACED before clearing JOBCTL_TRAPPING because of do_wait(), but I cannot seem to locate code in do_wait() and below that cares about JOBCTL_TRAPPING.

Could you elucidate my tired brain?

The only code I found that seems to care is ptrace_attach(), where we wait for JOBCTL_TRAPPING to get cleared. That same function has a comment about hiding the STOPPED -> RUNNING -> TRACED transition. So I'm assuming it needs to observe TRACED if it observes !TRAPPING.

But I don't think there's enough barriers on that end to guarantee this. Any ->state load after wait_on_bit() is, afact, free to have happened before the ->jobctl load.

#6 lore raw

Re: [PATCH 2/2] sched: Introduce set_special_state()

Quoted 5 lines · Peter Zijlstra

The only code I found that seems to care is ptrace_attach(), where we wait for JOBCTL_TRAPPING to get cleared. That same function has a comment about hiding the STOPPED -> RUNNING -> TRACED transition. So I'm assuming it needs to observe TRACED if it observes !TRAPPING.

Yes, exactly.

Quoted 3 lines

But I don't think there's enough barriers on that end to guarantee this. Any ->state load after wait_on_bit() is, afact, free to have happened before the ->jobctl load.

do_wait() does set_current_state() before it checks ->state or anything else.

Oleg.

author maintainer
#7 lore raw

Re: [PATCH 2/2] sched: Introduce set_special_state()

Quoted 7 lines · Oleg Nesterov
Quoted 5 lines · Peter Zijlstra

The only code I found that seems to care is ptrace_attach(), where we wait for JOBCTL_TRAPPING to get cleared. That same function has a comment about hiding the STOPPED -> RUNNING -> TRACED transition. So I'm assuming it needs to observe TRACED if it observes !TRAPPING.

Quoted 7 lines

Yes, exactly.

Quoted 3 lines

But I don't think there's enough barriers on that end to guarantee this. Any ->state load after wait_on_bit() is, afact, free to have happened before the ->jobctl load.

do_wait() does set_current_state() before it checks ->state or anything else.

But how are ptrace_attach() and do_wait() related? I guess I'm missing something fairly fundamental here. Is it userspace doing these two system calls back-to-back or something?

Is that not a little bit fragile, to rely on a barrier in do_wait() for this?

Anyway, does the below look ok?

---

kernel/signal.c +13 -1
@@ -1961,14 +1961,26 @@ static void ptrace_stop(int exit_code, i
 			return;
 	}
 
+	set_special_state(TASK_TRACED);
+
 	/*
 	 * We're committing to trapping.  TRACED should be visible before
 	 * TRAPPING is cleared; otherwise, the tracer might fail do_wait().
 	 * Also, transition to TRACED and updates to ->jobctl should be
 	 * atomic with respect to siglock and should be done after the arch
 	 * hook as siglock is released and regrabbed across it.
+	 *
+	 *     TRACER				    TRACEE
+	 *     ptrace_attach()
+	 * [L]   wait_on_bit(JOBCTL_TRAPPING)	[S] set_special_state(TRACED)
+	 *     do_wait()
+	 *       set_current_state()                smp_wmb();
+	 *       ptrace_do_wait()
+	 *         wait_task_stopped()
+	 *           task_stopped_code()
+	 * [L]         task_is_traced()		[S] task_clear_jobctl_trapping();
 	 */
-	set_special_state(TASK_TRACED);
+	smp_wmb();
 
 	current->last_siginfo = info;
 	current->exit_code = exit_code;
#8 lore raw

Re: [PATCH 2/2] sched: Introduce set_special_state()

Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Oleg Nesterov
Quoted 5 lines · Peter Zijlstra

The only code I found that seems to care is ptrace_attach(), where we wait for JOBCTL_TRAPPING to get cleared. That same function has a comment about hiding the STOPPED -> RUNNING -> TRACED transition. So I'm assuming it needs to observe TRACED if it observes !TRAPPING.

Yes, exactly.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

But I don't think there's enough barriers on that end to guarantee this. Any ->state load after wait_on_bit() is, afact, free to have happened before the ->jobctl load.

do_wait() does set_current_state() before it checks ->state or anything else.

But how are ptrace_attach() and do_wait() related?

Yes.

Quoted 2 lines

I guess I'm missing
something fairly fundamental here.

You are missing the fact that ptrace API is very old and ugly ;)

Just one example. If the debugger knows that the task is STOPPED then it has all rights to do, say,

	ptrace(PTRACE_ATTACH, pid);
	BUG_ON(pid != waitpid(pid, WNOHANG));

Or even do another ptrace() request right after ptrace(PTRACE_ATTACH) returns, without do_wait().

And unless my memory fools me, gdb even has some test-cases for this... Not sure, but it certainly looks at tracee->state in /proc before it does PTRACE_ATTACH, because if it was already STOPPED then gdb won't have any notification from the tracee.

Quoted 1 line

Anyway, does the below look ok?

Yes, thanks.

Oleg.