[RFC][BZ #13690] Always read private before lll_unlock.

[RFC][BZ #13690] Always read private before lll_unlock.

[Ondřej Bílka]

On Fri, Dec 06, 2013 at 08:01:59PM +0100, Ondřej Bílka wrote:
> Hi, a related issue to semaphore races is a race in mutex unlocking.
>
from bugzilla:
>
> On most platforms, lll_unlock() is defined as a macro like this:
> #define lll_unlock(lock, private) \
>   ((void) ({						      \
>     int *__futex = &(lock);				      \
>     int __val = atomic_exchange_rel (__futex, 0);	      \
>     if (__builtin_expect (__val > 1, 0))		      \
>       lll_futex_wake (__futex, 1, private);		      \
>   }))
> 
Which causes this problem that could be avoided by changing macro to

#define lll_unlock(lock, private) \
   ((void) ({                                                \
     int *__futex = &(lock);                                 \
     int __private = private				     \

I wrote a prelimitary patch for that, most of lll_unlock macros are
duplicates so I added a file include/futex_unlock.h with common
implementation. We should check these for more duplicates and if 
other functions need similar wrapper.

Comments?

---
 include/futex_unlock.h                                    | 15 +++++++++++++++
 nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h       |  8 +-------
 nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h          |  6 +++++-
 nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h            |  8 +++++++-
 nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h         |  6 ++++++
 ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h | 11 +----------
 ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h   | 10 +---------
 ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h     | 11 +----------
 ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h    |  8 +-------
 ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h    | 12 +-----------
 ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h    | 11 +----------
 .../unix/sysv/linux/microblaze/nptl/lowlevellock.h        | 10 +---------
 ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h    | 11 +----------
 ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h    | 11 +----------
 14 files changed, 43 insertions(+), 95 deletions(-)
 create mode 100644 include/futex_unlock.h

diff --git a/include/futex_unlock.h b/include/futex_unlock.h
new file mode 100644
index 0000000..cd497c2
--- /dev/null
+++ b/include/futex_unlock.h
@@ -0,0 +1,15 @@
+#define lll_unlock(lock, private) \
+  ((void)) ({								      \
+    int __private = private;						      \
+    __lll_unlock (lock, __private);					      \
+  })
+
+#define lll_unlock(lock, private) \
+  ((void) ({								      \
+    int *__futex = &(lock);						      \
+    int __val = atomic_exchange_rel (__futex, 0);			      \
+    if (__builtin_expect (__val > 1, 0))				      \
+      lll_futex_wake (__futex, 1, private);				      \
+  }))
+
+
diff --git a/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
index f33f703..84dea3d 100644
--- a/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
@@ -302,13 +302,7 @@ extern int __lll_robust_timedlock_wait
     __val;								      \
   })
 
-#define lll_unlock(lock, private) \
-  ((void) ({								      \
-    int *__futex = &(lock);						      \
-    int __val = atomic_exchange_rel (__futex, 0);			      \
-    if (__builtin_expect (__val > 1, 0))				      \
-      lll_futex_wake (__futex, 1, private);				      \
-  }))
+#include <futex_unlock.h>
 
 #define lll_robust_unlock(lock, private) \
   ((void) ({								      \
diff --git a/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h
index 3dab05e..911ff74 100644
--- a/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h
@@ -299,6 +299,11 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
+#define lll_unlock(futex, private) \
+  ((void)) ({								      \
+    int __private = private;						      \
+    __lll_unlock (&(futex), __private);					      \
+  })
 
 #define __lll_unlock(futex, private) \
   (void)								      \
@@ -310,7 +315,6 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
        if (__builtin_expect (__oldval > 1, 0))				      \
 	 lll_futex_wake (__futexp, 1, private);				      \
     })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
 
 
 #define __lll_robust_unlock(futex, private) \
diff --git a/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h
index 486e02c..e24cdf2 100644
--- a/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.h
@@ -284,7 +284,13 @@ extern int __lll_unlock_wake (int *__futex, int private) attribute_hidden;
 					      timeout, private); \
     __result; })
 
-#define lll_unlock(futex, private) \
+#define lll_unlock(lock, private) \
+  ((void)) ({								      \
+    int __private = private;						      \
+    __lll_unlock (lock, __private);					      \
+  })
+
+#define __lll_unlock(futex, private) \
   (void) ({ int __result, *__futex = &(futex); \
 	    __asm __volatile ("\
 		.align 2\n\
diff --git a/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
index 5ee8f6d..e435f4b 100644
--- a/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
+++ b/nptl/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
@@ -293,6 +293,12 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
 #define lll_unlock(lock, private) \
+  ((void)) ({								      \
+    int __private = private;						      \
+    __lll_unlock (lock, __private);					      \
+  })
+
+#define __lll_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
     int __val = atomic_exchange_24_rel (__futex, 0);			      \
diff --git a/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h
index 52f8a7a..0256f81 100644
--- a/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/aarch64/nptl/lowlevellock.h
@@ -232,16 +232,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
 
-#define __lll_unlock(futex, private) \
-  (void)							\
-    ({ int *__futex = (futex);					\
-       int __oldval = atomic_exchange_rel (__futex, 0);		\
-       if (__builtin_expect (__oldval > 1, 0))			\
-	 lll_futex_wake (__futex, 1, private);			\
-    })
-
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private) \
   (void)							\
diff --git a/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h
index 567f8ab..a1f1143 100644
--- a/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/alpha/nptl/lowlevellock.h
@@ -268,15 +268,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
 
-#define __lll_unlock(futex, private) \
-  (void)							\
-    ({ int *__futex = (futex);					\
-       int __oldval = atomic_exchange_rel (__futex, 0);		\
-       if (__builtin_expect (__oldval > 1, 0))			\
-	 lll_futex_wake (__futex, 1, private);			\
-    })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private) \
   (void)							\
diff --git a/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h
index a29593a..c9b3335 100644
--- a/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h
@@ -258,16 +258,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
-
-#define __lll_unlock(futex, private) \
-  (void)							\
-    ({ int *__futex = (futex);					\
-       int __oldval = atomic_exchange_rel (__futex, 0);		\
-       if (__builtin_expect (__oldval > 1, 0))			\
-	 lll_futex_wake (__futex, 1, private);			\
-    })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private) \
   (void)							\
diff --git a/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h
index 4cf8468..d76ef90 100644
--- a/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.h
@@ -288,13 +288,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
-#define __lll_unlock(futex, private) \
-  (void)					\
-  ({ int val = atomic_exchange_rel (futex, 0);  \
-     if (__builtin_expect (val > 1, 0))         \
-       lll_futex_wake (futex, 1, private);      \
-  })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
+#include <futex_unlock.h>
 
 #define  __lll_robust_unlock(futex,private) \
   (void)                                               \
diff --git a/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h
index cd36f95..3140447 100644
--- a/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/ia64/nptl/lowlevellock.h
@@ -236,17 +236,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
-
-#define __lll_unlock(futex, private)					      \
-  ((void) ({								      \
-    int *__futex = (futex);						      \
-    int __val = atomic_exchange_rel (__futex, 0);			      \
-									      \
-    if (__builtin_expect (__val > 1, 0))				      \
-      lll_futex_wake (__futex, 1, private);				      \
-  }))
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private)				      \
   ((void) ({								      \
diff --git a/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h
index 0df6604..5f683c2 100644
--- a/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/m68k/nptl/lowlevellock.h
@@ -228,16 +228,7 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
-
-#define __lll_unlock(futex, private) \
-  (void)							\
-    ({ int *__futex = (futex);					\
-       int __oldval = atomic_exchange_rel (__futex, 0);		\
-       if (__builtin_expect (__oldval > 1, 0))			\
-	 lll_futex_wake (__futex, 1, private);			\
-    })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private) \
   (void)							\
diff --git a/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h
index 70f5537..a601233 100644
--- a/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/microblaze/nptl/lowlevellock.h
@@ -256,15 +256,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_robust_timedlock(futex, abstime, id, private)                      \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
-#define __lll_unlock(futex, private)                                           \
-  ((void) ({                                                                   \
-    int *__futex = (futex);                                                    \
-    int __val = atomic_exchange_rel (__futex, 0);                              \
-                                                                               \
-    if (__builtin_expect (__val > 1, 0))                                       \
-      lll_futex_wake (__futex, 1, private);                                    \
-  }))
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private)                                    \
   ((void) ({                                                                   \
diff --git a/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h
index 208df8d..001f5f4 100644
--- a/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h
@@ -271,16 +271,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
 
-#define __lll_unlock(futex, private)					      \
-  ((void) ({								      \
-    int *__futex = (futex);						      \
-    int __val = atomic_exchange_rel (__futex, 0);			      \
-									      \
-    if (__builtin_expect (__val > 1, 0))				      \
-      lll_futex_wake (__futex, 1, private);				      \
-  }))
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private)				      \
   ((void) ({								      \
diff --git a/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h b/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h
index a9822ec..a8844f3 100644
--- a/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h
+++ b/ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h
@@ -266,16 +266,7 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
-
-#define __lll_unlock(futex, private) \
-  (void)							\
-    ({ int *__futex = (futex);					\
-       int __oldval = atomic_exchange_rel (__futex, 0);		\
-       if (__builtin_expect (__oldval > 1, 0))			\
-	 lll_futex_wake (__futex, 1, private);			\
-    })
-#define lll_unlock(futex, private) __lll_unlock(&(futex), private)
-
+#include <futex_unlock.h>
 
 #define __lll_robust_unlock(futex, private) \
   (void)							\
-- 
1.8.4.rc3

Re: [RFC][BZ #13690] Always read private before lll_unlock.

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 877 bytes --]

On Friday 06 December 2013 14:43:05 Ondřej Bílka wrote:
> --- /dev/null
> +++ b/include/futex_unlock.h

probably should live at nptl/lowlevellock_unlock

> @@ -0,0 +1,15 @@
> +#define lll_unlock(lock, private) \

all new files need a proper comment header block

> +  ((void)) ({								      \
> +    int __private = private;						      \
> +    __lll_unlock (lock, __private);					      \
> +  })
> +
> +#define lll_unlock(lock, private) \

did i misread, or are both of these macros named "lll_unlock" ?  should one 
have a __ prefix ?

> +  ((void) ({								      \
> +    int *__futex = &(lock);						      \
> +    int __val = atomic_exchange_rel (__futex, 0);			      \
> +    if (__builtin_expect (__val > 1, 0))				      \
> +      lll_futex_wake (__futex, 1, private);				      \
> +  }))
> +
> +

no trailing newlines please
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC][BZ #13690] Always read private before lll_unlock.

[Ondřej Bílka]

On Fri, Dec 06, 2013 at 04:11:58PM -0500, Mike Frysinger wrote:
> On Friday 06 December 2013 14:43:05 Ondřej Bílka wrote:
> > --- /dev/null
> > +++ b/include/futex_unlock.h
> 
> probably should live at nptl/lowlevellock_unlock
> 
or better name, see below.
> > @@ -0,0 +1,15 @@
> > +#define lll_unlock(lock, private) \
> 
> all new files need a proper comment header block
> 
> > +  ((void)) ({								      \
> > +    int __private = private;						      \
> > +    __lll_unlock (lock, __private);					      \
> > +  })
> > +
> > +#define lll_unlock(lock, private) \
> 
> did i misread, or are both of these macros named "lll_unlock" ?  should one 
> have a __ prefix ?
>
yes, i missed that, thanks.


Now when I looked to implementations we need more radical refactoring,

These implementatation were created by copying so headers are mostly
identical except of cosmetic changes like macro/inline function, added
expect and similar.

Some architectures need different primitives than atomic_compare_and_exchange_val_acq/rel but 
it looks most do not need that.

As example a diff -uw between arm and tile is following:

--- ports/sysdeps/unix/sysv/linux/tile/nptl/lowlevellock.h	2013-12-06 20:17:57.092514754 +0100
+++ ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h	2013-12-06 20:19:28.855381949 +0100
@@ -1,6 +1,5 @@
-/* Copyright (C) 2011-2013 Free Software Foundation, Inc.
+/* Copyright (C) 2005-2013 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
-   Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011.
 
    The GNU C Library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Lesser General Public
@@ -26,7 +25,6 @@
 #include <sysdep.h>
 #include <kernel-features.h>
 
-
 #define FUTEX_WAIT		0
 #define FUTEX_WAKE		1
 #define FUTEX_REQUEUE		3
@@ -83,9 +81,11 @@
 #define lll_futex_timed_wait(futexp, val, timespec, private) \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    INTERNAL_SYSCALL (futex, __err, 4, (futexp),			      \
+    long int __ret;							      \
+    __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp),		      \
 			      __lll_private_flag (FUTEX_WAIT, private),	      \
 			      (val), (timespec));			      \
+    __ret;								      \
   })
 
 #define lll_futex_timed_wait_bitset(futexp, val, timespec, clockbit, private) \
@@ -93,7 +93,6 @@
     INTERNAL_SYSCALL_DECL (__err);					      \
     long int __ret;							      \
     int __op = FUTEX_WAIT_BITSET | clockbit;				      \
-									      \
     __ret = INTERNAL_SYSCALL (futex, __err, 6, (futexp),		      \
 			      __lll_private_flag (__op, private),	      \
 			      (val), (timespec), NULL /* Unused.  */, 	      \
@@ -104,9 +103,11 @@
 #define lll_futex_wake(futexp, nr, private) \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    INTERNAL_SYSCALL (futex, __err, 4, (futexp),			      \
+    long int __ret;							      \
+    __ret = INTERNAL_SYSCALL (futex, __err, 4, (futexp),		      \
 			      __lll_private_flag (FUTEX_WAKE, private),	      \
 			      (nr), 0);					      \
+    __ret;								      \
   })
 
 #define lll_robust_dead(futexv, private) \
@@ -129,6 +130,7 @@
     INTERNAL_SYSCALL_ERROR_P (__ret, __err);				      \
   })
 
+
 /* Returns non-zero if error happened, zero if success.  */
 #define lll_futex_wake_unlock(futexp, nr_wake, nr_wake2, futexp2, private) \
   ({									      \
@@ -149,13 +151,11 @@
 					mutex, private)			      \
   ({									      \
     INTERNAL_SYSCALL_DECL (__err);					      \
-    long int __ret;							      \
     int __op = FUTEX_WAIT_REQUEUE_PI | clockbit;			      \
 									      \
-    __ret = INTERNAL_SYSCALL (futex, __err, 5, (futexp),		      \
+    INTERNAL_SYSCALL (futex, __err, 5, (futexp),			      \
 			      __lll_private_flag (__op, private),	      \
 			      (val), (timespec), mutex); 		      \
-    INTERNAL_SYSCALL_ERROR_P (__ret, __err);				      \
   })
 
 #define lll_futex_cmp_requeue_pi(futexp, nr_wake, nr_move, mutex, val, priv)  \
@@ -170,27 +170,14 @@
   })
 
 
-static inline int __attribute__ ((always_inline))
-__lll_trylock (int *futex)
-{
-  return atomic_compare_and_exchange_val_acq (futex, 1, 0) != 0;
-}
-#define lll_trylock(lock)	__lll_trylock (&(lock))
-
-
-static inline int __attribute__ ((always_inline))
-__lll_cond_trylock (int *futex)
-{
-  return atomic_compare_and_exchange_val_acq (futex, 2, 0) != 0;
-}
-#define lll_cond_trylock(lock)	__lll_cond_trylock (&(lock))
-
-
-static inline int __attribute__ ((always_inline))
-__lll_robust_trylock (int *futex, int id)
-{
-  return atomic_compare_and_exchange_val_acq (futex, id, 0) != 0;
-}
+#define lll_trylock(lock)	\
+  atomic_compare_and_exchange_val_acq(&(lock), 1, 0)
+
+#define lll_cond_trylock(lock)	\
+  atomic_compare_and_exchange_val_acq(&(lock), 2, 0)
+
+#define __lll_robust_trylock(futex, id) \
+  (atomic_compare_and_exchange_val_acq (futex, id, 0) != 0)
 #define lll_robust_trylock(lock, id) \
   __lll_robust_trylock (&(lock), id)
 
@@ -198,38 +185,41 @@
 extern void __lll_lock_wait (int *futex, int private) attribute_hidden;
 extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden;
 
-static inline void __attribute__ ((always_inline))
-__lll_lock (int *futex, int private)
-{
-  if (atomic_compare_and_exchange_bool_acq (futex, 1, 0) != 0)
-    {
-      if (__builtin_constant_p (private) && private == LLL_PRIVATE)
-	__lll_lock_wait_private (futex);
-      else
-	__lll_lock_wait (futex, private);
-    }
-}
+#define __lll_lock(futex, private)					      \
+  ((void) ({								      \
+    int *__futex = (futex);						      \
+    if (__builtin_expect (atomic_compare_and_exchange_val_acq (__futex,       \
+								1, 0), 0))    \
+      {									      \
+	if (__builtin_constant_p (private) && (private) == LLL_PRIVATE)	      \
+	  __lll_lock_wait_private (__futex);				      \
+	else								      \
+	  __lll_lock_wait (__futex, private);				      \
+      }									      \
+  }))
 #define lll_lock(futex, private) __lll_lock (&(futex), private)
 
 
-static inline int __attribute__ ((always_inline))
-__lll_robust_lock (int *futex, int id, int private)
-{
-  int result = 0;
-  if (atomic_compare_and_exchange_bool_acq (futex, id, 0) != 0)
-    result = __lll_robust_lock_wait (futex, private);
-  return result;
-}
+#define __lll_robust_lock(futex, id, private)				      \
+  ({									      \
+    int *__futex = (futex);						      \
+    int __val = 0;							      \
+									      \
+    if (__builtin_expect (atomic_compare_and_exchange_bool_acq (__futex, id,  \
+								0), 0))	      \
+      __val = __lll_robust_lock_wait (__futex, private);		      \
+    __val;								      \
+  })
 #define lll_robust_lock(futex, id, private) \
   __lll_robust_lock (&(futex), id, private)
 
 
-static inline void __attribute__ ((always_inline))
-__lll_cond_lock (int *futex, int private)
-{
-  if (atomic_compare_and_exchange_bool_acq (futex, 2, 0) != 0)
-    __lll_lock_wait (futex, private);
-}
+#define __lll_cond_lock(futex, private)					      \
+  ((void) ({								      \
+    int *__futex = (futex);						      \
+    if (__builtin_expect (atomic_exchange_acq (__futex, 2), 0))		      \
+      __lll_lock_wait (__futex, private);				      \
+  }))
 #define lll_cond_lock(futex, private) __lll_cond_lock (&(futex), private)
 
 
@@ -242,27 +232,29 @@
 extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
 					int private) attribute_hidden;
 
-static inline int __attribute__ ((always_inline))
-__lll_timedlock (int *futex, const struct timespec *abstime, int private)
-{
-  int result = 0;
-  if (atomic_compare_and_exchange_bool_acq (futex, 1, 0) != 0)
-    result = __lll_timedlock_wait (futex, abstime, private);
-  return result;
-}
+#define __lll_timedlock(futex, abstime, private)			      \
+  ({									      \
+     int *__futex = (futex);						      \
+     int __val = 0;							      \
+									      \
+     if (__builtin_expect (atomic_exchange_acq (__futex, 1), 0))	      \
+       __val = __lll_timedlock_wait (__futex, abstime, private);	      \
+     __val;								      \
+  })
 #define lll_timedlock(futex, abstime, private) \
   __lll_timedlock (&(futex), abstime, private)
 
 
-static inline int __attribute__ ((always_inline))
-__lll_robust_timedlock (int *futex, const struct timespec *abstime,
-			int id, int private)
-{
-  int result = 0;
-  if (atomic_compare_and_exchange_bool_acq (futex, id, 0) != 0)
-    result = __lll_robust_timedlock_wait (futex, abstime, private);
-  return result;
-}
+#define __lll_robust_timedlock(futex, abstime, id, private)		      \
+  ({									      \
+    int *__futex = (futex);						      \
+    int __val = 0;							      \
+									      \
+    if (__builtin_expect (atomic_compare_and_exchange_bool_acq (__futex, id,  \
+								0), 0))	      \
+      __val = __lll_robust_timedlock_wait (__futex, abstime, private);	      \
+    __val;								      \
+  })
 #define lll_robust_timedlock(futex, abstime, id, private) \
   __lll_robust_timedlock (&(futex), abstime, id, private)
 
@@ -282,10 +274,18 @@
 #define lll_islocked(futex) \
   (futex != 0)
 
+
+/* Our internal lock implementation is identical to the binary-compatible
+   mutex implementation. */
+
 /* Initializers for lock.  */
 #define LLL_LOCK_INITIALIZER		(0)
 #define LLL_LOCK_INITIALIZER_LOCKED	(1)
 
+/* The states of a lock are:
+    0  -  untaken
+    1  -  taken by one user
+   >1  -  taken by more users */
 
 /* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex
    wakeup when the clone terminates.  The memory location contains the

Re: [RFC][BZ #13690] Always read private before lll_unlock.

[Torvald Riegel]

On Fri, 2013-12-06 at 20:43 +0100, Ondřej Bílka wrote:
> On Fri, Dec 06, 2013 at 08:01:59PM +0100, Ondřej Bílka wrote:
> > Hi, a related issue to semaphore races is a race in mutex unlocking.
> >
> from bugzilla:
> >
> > On most platforms, lll_unlock() is defined as a macro like this:
> > #define lll_unlock(lock, private) \
> >   ((void) ({						      \
> >     int *__futex = &(lock);				      \
> >     int __val = atomic_exchange_rel (__futex, 0);	      \
> >     if (__builtin_expect (__val > 1, 0))		      \
> >       lll_futex_wake (__futex, 1, private);		      \
> >   }))
> > 
> Which causes this problem that could be avoided by changing macro to
> 
> #define lll_unlock(lock, private) \
>    ((void) ({                                                \
>      int *__futex = &(lock);                                 \
>      int __private = private				     \
> 
> I wrote a prelimitary patch for that, most of lll_unlock macros are
> duplicates so I added a file include/futex_unlock.h with common
> implementation. We should check these for more duplicates and if 
> other functions need similar wrapper.
> 
> Comments?

This needs clarification in POSIX; see my comment on #13690
(https://sourceware.org/bugzilla/show_bug.cgi?id=13690#c24).  Depending
on how the Austin Group decides, this is either not a bug, or we have to
fix the pending load AND investigate whether the pending futex_wake call
is harmless.  The latter might be the case for normal mutexes, but I
wouldn't be surprised to find out that PI or robust mutexes aren't as
simple and need a more complex fix.

Therefore, I think we should wait for the POSIX clarification and then
decide what the next steps should be.