Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752879AbdI0MRP (ORCPT ); Wed, 27 Sep 2017 08:17:15 -0400 Received: from merlin.infradead.org ([205.233.59.134]:41166 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752814AbdI0MRN (ORCPT ); Wed, 27 Sep 2017 08:17:13 -0400 Date: Wed, 27 Sep 2017 14:17:06 +0200 From: Peter Zijlstra To: Junaid Shahid Cc: linux-kernel@vger.kernel.org, andreslc@google.com, gthelen@google.com, rusty@rustcorp.com.au, Thomas Gleixner Subject: Re: [PATCH] kthread: Fix race condition between kthread_parkme() and kthread_unpark() Message-ID: <20170927121706.g5qydxv3v2usmdd5@hirez.programming.kicks-ass.net> References: <20170429023236.60452-1-junaids@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170429023236.60452-1-junaids@google.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4291 Lines: 135 On Fri, Apr 28, 2017 at 07:32:36PM -0700, Junaid Shahid wrote: > In general, if kthread_unpark() and kthread_parkme() execute together, > the kthread is supposed to be in an unparked state. This is because > kthread_unpark() either wakes up the thread if it already got parked, > or it cancels a prior kthread_park() call and hence renders the > kthread_parkme() moot. > > However, if kthread_unpark() happens to execute between the time that > kthread_parkme() checks the KTHREAD_SHOULD_STOP flag and sets the > KTHREAD_IS_PARKED flag, then kthread_unpark() will not wake up the > thread and it will remain in a parked state. > > This is fixed by making the checking of KTHREAD_SHOULD_STOP and the > setting of KTHREAD_IS_PARKED atomic via a cmpxchg inside kthread_parkme. > > Signed-off-by: Junaid Shahid > --- > kernel/kthread.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/kernel/kthread.c b/kernel/kthread.c > index 26db528c1d88..651f03baf62f 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -169,12 +169,20 @@ void *kthread_probe_data(struct task_struct *task) > > static void __kthread_parkme(struct kthread *self) > { > + ulong flags; That should be: "unsigned long". > + > __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(); > + flags = self->flags; And at the very least, that should be: flags = READ_ONCE(self->flags); > + > + while (test_bit(KTHREAD_SHOULD_PARK, &flags)) { Otherwise there's very little that stops the compiler from doing a reload here. > + if (cmpxchg(&self->flags, flags, > + flags | (1 << KTHREAD_IS_PARKED)) == flags) { > + if (!test_bit(KTHREAD_IS_PARKED, &flags)) > + complete(&self->parked); > + schedule(); > + } > __set_current_state(TASK_PARKED); > + flags = self->flags; > } That said, this is really quite horrible. The alternative is of course changing kthread_unpark() to use a cmpxchg to clear SHOULD_PARK and then check IS_PARKED on its return value, but that's equally horrible. How about something like the below? I still think its overly fragile, but barring more invasive changes the below more or less does as you suggest. --- kernel/kthread.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/kernel/kthread.c b/kernel/kthread.c index 1c19edf82427..f437f53f6fbc 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -169,14 +169,38 @@ void *kthread_probe_data(struct task_struct *task) static void __kthread_parkme(struct kthread *self) { - __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(); + unsigned long flags; + + for (;;) { __set_current_state(TASK_PARKED); + + /* + * CPU0 CPU1 + * + * [S] ->state = PARKED [S] ->flags &= ~SHOULD_PARK + * [L] ->flags [RmW] ->flags &= ~IS_PARKED + * MB MB + * [RmW] ->flags |= IS_PARKED wakeup() + * + * Thus, either CPU0 sets IS_PARKED and CPU1's wakeup must observe PARKED + * or we observe !SHOULD_PARK and terminate. + */ + + flags = READ_ONCE(self->flags); + if (!test_bit(KTHREAD_SHOULD_PARK, &flags)) + break; + + /* + * Ensure SHOULD_PARK remains set while we set IS_PARKED. + * Thereby we ensure kthread_unpark() cannot clear SHOULD_PARK + * without us noticing. + */ + + if (try_cmpxchg(&self->flags, &flags, flags | (1UL << KTHREAD_IS_PARKED))) { + complete(&self->parked); + schedule(); + } } - clear_bit(KTHREAD_IS_PARKED, &self->flags); __set_current_state(TASK_RUNNING); } @@ -454,6 +478,10 @@ void kthread_unpark(struct task_struct *k) /* * Newly created kthread was parked when the CPU was offline. * The binding was lost and we need to set it again. + * + * Since we've observed IS_PARKED, the other thread will (have) + * called schedule() and thus __kthread_bind()'s + * wait_task_inactive() will succeed. */ if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags)) __kthread_bind(k, kthread->cpu, TASK_PARKED);