[PATCH] Fix elision in nested trylock.

[Torvald Riegel]

Attached is a patch that fixes a race condition in how Andi's current
patches use elision in trylock.  The elision code path in trylock aborts
if it is executed in a transaction, but the non-elision code path does
no such thing.  Thus, we need to avoid that a thread acquires a lock by
using elision and then executes a nested trylock that doesn't use
elision (the nested trylock would violate the POSIX requirements in this
case).  To do that, we recheck our elision decision in the transaction,
which ensures that the nested trylock will also execute the elision
path.  Another option to solve that would be to abort any transaction in
the non-elision code path too, but that might affect non-glibc-elision
transactions that use locks even more.

This patch might not apply cleanly to Andi's branches, but the comment
in it should clarify what's going on.  (And please keep the details of
the comment if you pick it up -- or expand the comments; everyone else
looking at the code in the future will appreciate it.)

Torvald
-------------- next part --------------
commit b8484fd71bf171f871c28db0827566c821b9239c
Author: Torvald Riegel <triegel@redhat.com>
Date:   Thu Jun 27 00:49:29 2013 +0200

    Fix elision in nested trylock.

diff --git a/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c b/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c
index 5fdeb19..22c6a72 100644
--- a/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c
+++ b/nptl/sysdeps/unix/sysv/linux/x86/elision-lock.c
@@ -57,6 +57,11 @@ __lll_lock_elision (int *futex, short *try_lock, EXTRAARG int private)
 	{
 	  if ((status = _xbegin()) == _XBEGIN_STARTED)
 	    {
+	      /* Avoid a race condition on TRY_LOCK.  See
+	         __lll_trylock_elision for details.  */
+	      if (*try_lock > 0)
+		_xabort(_ABORT_LOCK_BUSY);
+
 	      if (*futex == 0)
 		return 0;
 
diff --git a/nptl/sysdeps/unix/sysv/linux/x86/elision-trylock.c b/nptl/sysdeps/unix/sysv/linux/x86/elision-trylock.c
index a14dfeb..79b16d1 100644
--- a/nptl/sysdeps/unix/sysv/linux/x86/elision-trylock.c
+++ b/nptl/sysdeps/unix/sysv/linux/x86/elision-trylock.c
@@ -46,6 +46,23 @@ __lll_trylock_elision (int *futex, short *try_lock)
 
       if ((status = _xbegin()) == _XBEGIN_STARTED)
 	{
+	  /* To make trylock work with elision, we need to ensure that a
+	     subsequent trylock will execute the elision code path if it
+	     tries to acquire the same lock we have acquired here; in
+	     particular, we need to abort in this case (see above). The value
+	     of TRY_LOCK can change between our nontransactional read at the
+	     beginning of this function and the time we start the
+	     transaction; in turn, a nested trylock might take the
+	     non-elision code path, which will incorrectly acquire the lock
+	     a second time instead of return that the lock is already
+	     acquired.  To prevent this case, we again check in the
+	     transaction that we should use elision; then, any nested trylock
+	     will observe the same value and take the elision code path as
+	     well.  We do the same in __lll_lock_elision.
+	     XXX Is this the right abort code?  */
+	  if (*try_lock > 0)
+	    _xabort(_ABORT_LOCK_BUSY);
+
 	  if (*futex == 0)
 	    return 0;