Messages in this thread Patch in this message | | | Date | Fri, 7 Nov 2014 11:28:55 +0100 | From | Peter Zijlstra <> | Subject | [PATCH]: kthread: Fix memory ordering in __kthread_parkme |
| |
On Fri, Nov 07, 2014 at 10:39:48AM +0100, Thomas Gleixner wrote: > > 2) the usage of __set_current_state(TASK_PARKED) in __kthread_parkme() > > is wrong AFAICT, one should always use set_current_state() for > > setting !TASK_RUNNING state. The comment with set_current_state() > > explains why. > > > > This would've allowed the test_bit(KTHREAD_SHOULD_PARK) load to have > > been satisfied before the store of TASK_PARKED. > > My bad. Can you send a proper patch addressing that issue please? That > should be tagged stable as well I guess.
Sure thing, something like so then?
--- Subject: kthread: Fix memory ordering in __kthread_parkme
One should always use set_current_state() to set !TASK_RUNNING states.
set_current_state(TASK_*); cond = true; /* mb */ /* wmb */ if (!cond) wake_up_state(, TASK_*); schedule();
By not having the mb we allow for the cond load to be satisfied before the state store, this can result in:
if (!cond) cond = true; wake_up_state(, TASK_*); __set_current_state(TASK_*); schedule();
Which would block 'forever', since the cond is still false and the wakeup would not have seen the !TASK_RUNNING state.
Cc: stable@vger.kernel.org Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/kthread.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/kthread.c b/kernel/kthread.c index 10e489c448fe..9787244d43ec 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -156,12 +156,12 @@ void *probe_kthread_data(struct task_struct *task) static void __kthread_parkme(struct kthread *self) { - __set_current_state(TASK_PARKED); + set_current_state(TASK_PARKED); while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) { if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags)) complete(&self->parked); schedule(); - __set_current_state(TASK_PARKED); + set_current_state(TASK_PARKED); } clear_bit(KTHREAD_IS_PARKED, &self->flags); __set_current_state(TASK_RUNNING);
| |