sched,kthread: Fix TASK_PARKED and special sleep states
Peter Zijlstra
maintainercover 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.
Peter Zijlstra
maintainerPATCH 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_strstatic 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 );
Peter Zijlstra
maintainerPATCH 2/2 sched: Introduce set_special_state()
In reply to Peter ZijlstraGaurav 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 (¤t->pi_lock, flags); \ + current->task_state_change = _THIS_IP_; \ + current->state = (state_value); \ +raw_spin_unlock_irqrestore (¤t->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 (¤t->pi_lock, flags); \ + current->state = (state_value); \ +raw_spin_unlock_irqrestore (¤t->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 (¤t->sighand->siglock);if (current->jobctl &JOBCTL_STOP_DEQUEUED ) -__set_current_state (TASK_STOPPED ); +set_special_state (TASK_STOPPED );spin_unlock_irq (¤t->sighand->siglock);schedule ();
kernel/sched/core.c +1 -16
@@ -3498,23 +3498,8 @@ static void __sched notrace __schedule(bvoid __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 (¤t->pi_lock); -raw_spin_unlock_irq (¤t->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 (¤t->sighand->siglock);/*
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
Peter Zijlstra
maintainerRe: [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 whyptrace_stop()doesset_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?
Peter Zijlstra
maintainerRe: [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 whyptrace_stop()doesset_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_TRACEDbefore clearingJOBCTL_TRAPPINGbecause ofdo_wait(), but I cannot seem to locate code indo_wait()and below that cares aboutJOBCTL_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.
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 forJOBCTL_TRAPPINGto 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.
Peter Zijlstra
maintainerRe: [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 forJOBCTL_TRAPPINGto 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()doesset_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, ireturn ; } +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;
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 forJOBCTL_TRAPPINGto 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()doesset_current_state()before it checks ->state or anything else.But how are
ptrace_attach()anddo_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.