Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1601805 > unrolled thread

[PATCH 1/2] kthread: add barriers to set_kthread_struct() and to_kthread()

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] kthread: add barriers to set_kthread_struct() and  to_kthread() Tejun Heo <tj@kernel.org> - 2017-03-16 00:20 +0100
    Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and         to_kthread() Oleg Nesterov <oleg@redhat.com> - 2017-03-16 16:00 +0100
      Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and  to_kthread() Tejun Heo <tj@kernel.org> - 2017-03-16 16:40 +0100
        Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and         to_kthread() Oleg Nesterov <oleg@redhat.com> - 2017-03-16 16:50 +0100
        Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and  to_kthread() Peter Zijlstra <peterz@infradead.org> - 2017-03-16 17:00 +0100
          Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and  to_kthread() Peter Zijlstra <peterz@infradead.org> - 2017-03-16 17:20 +0100
          Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and  to_kthread() Tejun Heo <tj@kernel.org> - 2017-03-16 17:20 +0100
      Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and  to_kthread() Tejun Heo <tj@kernel.org> - 2017-03-16 16:40 +0100

#1601805 — [PATCH 1/2] kthread: add barriers to set_kthread_struct() and to_kthread()

Until now, all to_kthread() users are interlocked with kthread
creation and there's no need to have explicit barriers when setting
the kthread pointer or dereferencing it.

However, There is a race condition where userland can interfere with a
kthread while it's being initialized.  To close it, to_kthread() needs
to be used from an unsynchronized context.

This patch moves struct kthread initialization before
set_kthread_struct() and adds matching barriers in
set_kthread_struct() and to_kthread(), so that dereferencing
to_kthread() always returns initialized fields.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Chris Mason <clm@fb.com>
Cc: stable@vger.kernel.org # v4.3+ (we can't close the race < v4.3)
---
 kernel/kthread.c |   30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -57,6 +57,9 @@ enum KTHREAD_BITS {
 
 static inline void set_kthread_struct(void *kthread)
 {
+	/* paired with smp_read_data_barrier_depends() in to_kthread() */
+	smp_wmb();
+
 	/*
 	 * We abuse ->set_child_tid to avoid the new member and because it
 	 * can't be wrongly copied by copy_process(). We also rely on fact
@@ -67,8 +70,19 @@ static inline void set_kthread_struct(vo
 
 static inline struct kthread *to_kthread(struct task_struct *k)
 {
+	void *ptr;
+
 	WARN_ON(!(k->flags & PF_KTHREAD));
-	return (__force void *)k->set_child_tid;
+
+	ptr = (__force void *)k->set_child_tid;
+
+	/*
+	 * Paired with smp_wmb() in set_kthread_struct() and ensures that
+	 * the caller sees initialized content of the returned kthread.
+	 */
+	smp_read_barrier_depends();
+
+	return ptr;
 }
 
 void free_kthread_struct(struct task_struct *k)
@@ -196,6 +210,14 @@ static int kthread(void *_create)
 	int ret;
 
 	self = kmalloc(sizeof(*self), GFP_KERNEL);
+	if (self) {
+		self->flags = 0;
+		self->data = data;
+		init_completion(&self->exited);
+		init_completion(&self->parked);
+		current->vfork_done = &self->exited;
+	}
+
 	set_kthread_struct(self);
 
 	/* If user was SIGKILLed, I release the structure. */
@@ -211,12 +233,6 @@ static int kthread(void *_create)
 		do_exit(-ENOMEM);
 	}
 
-	self->flags = 0;
-	self->data = data;
-	init_completion(&self->exited);
-	init_completion(&self->parked);
-	current->vfork_done = &self->exited;
-
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	create->result = current;

[toc] | [next] | [standalone]

#1602424 — Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and to_kthread()

Hi Tejun,

On 03/15, Tejun Heo wrote:
>
> Until now, all to_kthread() users are interlocked with kthread
> creation and there's no need to have explicit barriers when setting
> the kthread pointer or dereferencing it.
>
> However, There is a race condition where userland can interfere with a
> kthread while it's being initialized.  To close it, to_kthread() needs
> to be used from an unsynchronized context.

So this is preparation for 2/2... IIUC, the current code is not buggy,
just you need to add kthread_initialized() which can't work without
this change.

>  static inline void set_kthread_struct(void *kthread)
>  {
> +	/* paired with smp_read_data_barrier_depends() in to_kthread() */
> +	smp_wmb();
> +
>  	/*
>  	 * We abuse ->set_child_tid to avoid the new member and because it
>  	 * can't be wrongly copied by copy_process(). We also rely on fact
> @@ -67,8 +70,19 @@ static inline void set_kthread_struct(vo
>
>  static inline struct kthread *to_kthread(struct task_struct *k)
>  {
> +	void *ptr;
> +
>  	WARN_ON(!(k->flags & PF_KTHREAD));
> -	return (__force void *)k->set_child_tid;
> +
> +	ptr = (__force void *)k->set_child_tid;
> +
> +	/*
> +	 * Paired with smp_wmb() in set_kthread_struct() and ensures that
> +	 * the caller sees initialized content of the returned kthread.
> +	 */
> +	smp_read_barrier_depends();
> +
> +	return ptr;

This is almost off-topic, but I think lockless_dereference() will look
better in to_kthread().

And perhaps we should add another helper, say,

	#define lockless_assign_pointer(ptr, val)	\
		smp_store_release(&ptr, val)

for set_kthread_struct() ? it can have more users.

Not that I think you should change your patch, I am just asking.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1602548

On Thu, Mar 16, 2017 at 11:33:01AM -0400, Tejun Heo wrote:
> > And perhaps we should add another helper, say,
> > 
> > 	#define lockless_assign_pointer(ptr, val)	\
> > 		smp_store_release(&ptr, val)
> > 
> > for set_kthread_struct() ? it can have more users.
> > 
> > Not that I think you should change your patch, I am just asking.
> 
> Ah yeah, that would look better.  I vaguely remembered the new macro
> but couldn't quite remember it fully. :)  Will update the patch.

Oops, as for adding lockless_assign_pointer(), wouldn't smp_wmb() be a
better match for smp_read_barrier_depends()?  ISTR acquire/release
pairs being more expensive on some archs.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1602578 — Re: [PATCH 1/2] kthread: add barriers to set_kthread_struct() and to_kthread()

On 03/16, Tejun Heo wrote:
>
> On Thu, Mar 16, 2017 at 11:33:01AM -0400, Tejun Heo wrote:
> > > And perhaps we should add another helper, say,
> > >
> > > 	#define lockless_assign_pointer(ptr, val)	\
> > > 		smp_store_release(&ptr, val)
> > >
> > > for set_kthread_struct() ? it can have more users.
> > >
> > > Not that I think you should change your patch, I am just asking.
> >
> > Ah yeah, that would look better.  I vaguely remembered the new macro
> > but couldn't quite remember it fully. :)  Will update the patch.
>
> Oops, as for adding lockless_assign_pointer(), wouldn't smp_wmb() be a
> better match for smp_read_barrier_depends()?  ISTR acquire/release
> pairs being more expensive on some archs.

No, no, don't ask me, I can't know ;)

But. Note that rcu_assign_pointer() (which should pair with
smp_read_barrier_depends/lockless_dereference too) uses
smp_store_release(), and the changelog says "potentially less overhead".
See 88c1863066ccfa456 "rcu: Define rcu_assign_pointer() in terms of
smp_store_release()".

And this discussion is another argument to add the new helper,
we can always change it to use wmb or store_release, or whatever else.
Plus arch/ can overwrite it.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1602598

On Thu, Mar 16, 2017 at 11:38:43AM -0400, Tejun Heo wrote:
> On Thu, Mar 16, 2017 at 11:33:01AM -0400, Tejun Heo wrote:
> > > And perhaps we should add another helper, say,
> > > 
> > > 	#define lockless_assign_pointer(ptr, val)	\
> > > 		smp_store_release(&ptr, val)
> > > 
> > > for set_kthread_struct() ? it can have more users.
> > > 
> > > Not that I think you should change your patch, I am just asking.
> > 
> > Ah yeah, that would look better.  I vaguely remembered the new macro
> > but couldn't quite remember it fully. :)  Will update the patch.
> 
> Oops, as for adding lockless_assign_pointer(), wouldn't smp_wmb() be a
> better match for smp_read_barrier_depends()?  ISTR acquire/release
> pairs being more expensive on some archs.

88c1863066cc ("rcu: Define rcu_assign_pointer() in terms of smp_store_release()")

[toc] | [prev] | [next] | [standalone]

#1602612

On Thu, Mar 16, 2017 at 12:09:44PM -0400, Tejun Heo wrote:
> On Thu, Mar 16, 2017 at 04:55:34PM +0100, Peter Zijlstra wrote:
> > > Oops, as for adding lockless_assign_pointer(), wouldn't smp_wmb() be a
> > > better match for smp_read_barrier_depends()?  ISTR acquire/release
> > > pairs being more expensive on some archs.
> > 
> > 88c1863066cc ("rcu: Define rcu_assign_pointer() in terms of smp_store_release()")
> 
> Hmmm, nice, can we always prefer store_release over wmb from now on?

I would advocate using whichever barrier is most natural for the
occasion.

Only if there really is a very very compelling performance argument
should we look further.

[toc] | [prev] | [next] | [standalone]

#1602618

On Thu, Mar 16, 2017 at 04:55:34PM +0100, Peter Zijlstra wrote:
> > Oops, as for adding lockless_assign_pointer(), wouldn't smp_wmb() be a
> > better match for smp_read_barrier_depends()?  ISTR acquire/release
> > pairs being more expensive on some archs.
> 
> 88c1863066cc ("rcu: Define rcu_assign_pointer() in terms of smp_store_release()")

Hmmm, nice, can we always prefer store_release over wmb from now on?

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1602570

Hello, Oleg.

On Thu, Mar 16, 2017 at 03:54:36PM +0100, Oleg Nesterov wrote:
> On 03/15, Tejun Heo wrote:
> >
> > Until now, all to_kthread() users are interlocked with kthread
> > creation and there's no need to have explicit barriers when setting
> > the kthread pointer or dereferencing it.
> >
> > However, There is a race condition where userland can interfere with a
> > kthread while it's being initialized.  To close it, to_kthread() needs
> > to be used from an unsynchronized context.
> 
> So this is preparation for 2/2... IIUC, the current code is not buggy,
> just you need to add kthread_initialized() which can't work without
> this change.

Yeah, I could have been clearer.

> > +	/*
> > +	 * Paired with smp_wmb() in set_kthread_struct() and ensures that
> > +	 * the caller sees initialized content of the returned kthread.
> > +	 */
> > +	smp_read_barrier_depends();
> > +
> > +	return ptr;
> 
> This is almost off-topic, but I think lockless_dereference() will look
> better in to_kthread().
> 
> And perhaps we should add another helper, say,
> 
> 	#define lockless_assign_pointer(ptr, val)	\
> 		smp_store_release(&ptr, val)
> 
> for set_kthread_struct() ? it can have more users.
> 
> Not that I think you should change your patch, I am just asking.

Ah yeah, that would look better.  I vaguely remembered the new macro
but couldn't quite remember it fully. :)  Will update the patch.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web