linux-kernel - Re: [PATCH 4/5] futex: Avoid taking hb lock if nothing to wakeup

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: <alpine.DEB.2.02.1311251938570.30673@ionos.tec.linutronix.de>
Date:	Mon, 25 Nov 2013 20:47:38 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Davidlohr Bueso <davidlohr@...com>
cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Darren Hart <dvhart@...ux.intel.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Mike Galbraith <efault@....de>, jeffm@...e.com,
	"Norton, Scott J" <scott.norton@...com>, tom.vaden@...com,
	"Chandramouleeswaran, Aswin" <aswin@...com>,
	Waiman Long <Waiman.Long@...com>,
	Jason Low <jason.low2@...com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH 4/5] futex: Avoid taking hb lock if nothing to wakeup

On Sat, 23 Nov 2013, Thomas Gleixner wrote:
> On Fri, 22 Nov 2013, Davidlohr Bueso wrote:
> So with the atomic ops you are changing that to:
> 
> CPU 0					CPU 1
> 
>     val = *futex;
>     futex_wait(futex, val);
> 
>     spin_lock(&hb->lock);
> 
>     atomic_inc(&hb->waiters);
>     uval = *futex;
> 					*futex = newval;
> 
>     if (uval != val) {			futex_wake();
>        spin_unlock(&hb->lock);		if (!atomic_read(&hb->waiters))
>        return;				   return;
>     }
>    					spin_lock((&hb->lock);	
>     plist_add(hb, self);
>     spin_unlock(&hb->lock);
>     schedule();				wakeup_waiters(hb);
>     ...
> 
> which restores the ordering guarantee, which the hash bucket lock
> provided so far.

Actually that's not true by design, it just happens to work.

atomic_inc() on x86 is a "lock incl".

 The LOCK prefix just guarantees that the cache line which is affected
 by the INCL is locked. And it guarantees that locked operations
 serialize all outstanding load and store operations.

 But Documentation/atomic_ops.txt says about atomic_inc():

 "One very important aspect of these two routines is that they DO NOT
  require any explicit memory barriers.  They need only perform the
  atomic_t counter update in an SMP safe manner."

 So while this has a barrier on x86, it's not guaranteed.

atomic_read() is a simple read.

 This does not guarantee anything. And if you read
 Documentation/atomic_ops.txt it's well documented:

 "*** WARNING: atomic_read() and atomic_set() DO NOT IMPLY BARRIERS! ***"

So now your code melts down to:

 write(hb->waiters)    |    write(uaddr)
 mb		       |    read(hb->waiters)
 read(uaddr)

I fear you simply managed to make the window small enough that your
testing was not longer able expose it.

Thanks,

	tglx

--
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/