Re: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and sched_setaffinity()

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Aug 07, 2015 at 11:16:08AM -0400, Tejun Heo wrote:
> On Fri, Aug 07, 2015 at 04:27:08PM +0200, Peter Zijlstra wrote:
> > Which is the rescue thread attaching itself to a pool that needs help,
> > and obviously the rescue thread isn't new so kthread_bind doesn't work
> > right.
> > 
> > The best I could come up with is something like the below on top; does
> > that work for you? I'll go give it some runtime.
> > 
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -1622,11 +1622,15 @@ static struct worker *alloc_worker(int n
> >   * cpu-[un]hotplugs.
> >   */
> >  static void worker_attach_to_pool(struct worker *worker,
> > -				   struct worker_pool *pool)
> > +				   struct worker_pool *pool,
> > +				   bool new)
> >  {
> >  	mutex_lock(&pool->attach_mutex);
> >  
> > -	kthread_bind_mask(worker->task, pool->attrs->cpumask);
> > +	if (new)
> > +		kthread_bind_mask(worker->task, pool->attrs->cpumask);
> > +	else
> > +		set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
> >  
> >  	/*
> >  	 * The pool->attach_mutex ensures %POOL_DISASSOCIATED remains
> > @@ -1712,7 +1716,7 @@ static struct worker *create_worker(stru
> >  	set_user_nice(worker->task, pool->attrs->nice);
> >  
> >  	/* successful, attach the worker to the pool */
> > -	worker_attach_to_pool(worker, pool);
> > +	worker_attach_to_pool(worker, pool, true);
> >  
> >  	/* start the newly created worker */
> >  	spin_lock_irq(&pool->lock);
> > @@ -2241,7 +2245,7 @@ static int rescuer_thread(void *__rescue
> >  
> >  		spin_unlock_irq(&wq_mayday_lock);
> >  
> > -		worker_attach_to_pool(rescuer, pool);
> > +		worker_attach_to_pool(rescuer, pool, false);
> 
> Hmmm... the race condition didn't exist for workqueue in the first
> place, right?

No, I think workqueues are susceptible just the same as everybody else.
By the time we call __kthread_bind() the task exists and is visible to
userspace.

        __kthread_bind()
          do_set_cpus_allowed()
                                                <SYSCALL>
                                                  sched_setaffinity()
                                                    if (p->flags & PF_NO_SETAFFINITIY) /* false-not-taken */
                                                    set_cpus_allowed_ptr()
          p->flags |= PF_NO_SETAFFINITY

> As long as the flag is set before the affinity is configured, there's
> no race condition.

Even if we were to strictly order those stores you could have (note
there is no matching barrier, as there is only the one load, so ordering
cannot help):

	__kthread_bind()
						<SYSCALL>
						  sched_setaffinity()
						    if (p->flags & PF_NO_SETAFFINITY) /* false-not-taken */
	  p->flags |= PF_NO_SETAFFINITY;
	  smp_wmb();
	  do_set_cpus_allowed();
						    set_cpus_allowed_ptr()

> I think the code was better before.  Can't we just revert workqueue.c
> part?

I agree that the new argument isn't pretty, but I cannot see how
workqueues would not be affected by this.