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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150807152956.GN16853@twins.programming.kicks-ass.net>
Date:	Fri, 7 Aug 2015 17:29:56 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Tejun Heo <tj@...nel.org>
Cc:	mingo@...nel.org, riel@...hat.com, dedekind1@...il.com,
	linux-kernel@...r.kernel.org, mgorman@...e.de, rostedt@...dmis.org,
	juri.lelli@....com, Oleg Nesterov <oleg@...hat.com>
Subject: Re: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and
 sched_setaffinity()

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/