[PATCH][BZ 13690] Do not violate mutex destruction requirements.

[PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

POSIX and C++11 require that a thread can destroy a mutex if no thread
owns the mutex, is blocked on the mutex, or will try to acquire it in
the future.  After destroying the mutex, it can reuse or unmap the
underlying memory.  Thus, we must not access a mutex' memory after
releasing it.  Currently, we can load the private flag after releasing
the mutex, which is fixed by this patch.

See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
background.
    
We need to call futex_wake on the lock after releasing it, however.
This is by design, and can lead to spurious wake-ups on unrelated futex
words (e.g., when the mutex memory is reused for another mutex).  This
behavior is documented in the glibc-internal futex API and in recent
drafts of the Linux kernel's futex documentation (see the draft_futex
branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).

Carlos, do you want to consider this for 2.22, or should this rather
target 2.23?  The actual change is simple.

Not tested.  Could someone test this on a non-x86-linux machine (I don't
have one handy).  Dave, could you test on sparc, please?


2015-07-14  Torvald Riegel  <triegel@redhat.com>

	[BZ #13690]
	* sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock
	after releasing it.
	(__lll_robust_unlock): Likewise.
	* nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise.
	(lll_robust_unlock): Likewise.


[-- Attachment #2: mutex-destruction.patch --]
[-- Type: text/x-patch, Size: 6242 bytes --]

commit abff5f4160ad21dcc71841c5e92dac8db0b81cff
Author: Torvald Riegel <triegel@redhat.com>
Date:   Tue Jul 14 21:58:34 2015 +0200

    Do not violate mutex destruction requirements.
    
    POSIX and C++11 require that a thread can destroy a mutex if no other
    thread owns the mutex, is blocked on the mutex, or will try to acquire
    it in the future.  After destroying the mutex, it can reuse or unmap the
    underlying memory.  Thus, we must not access a mutex' memory after
    releasing it.  Currently, we can load the private flag after releasing
    the mutex, which is fixed by this patch.
    See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
    background.
    
    We need to call futex_wake on the lock after releasing it, however.  This
    is by design, and can lead to spurious wake-ups on unrelated futex words
    (e.g., when the mutex memory is reused for another mutex).  This behavior
    is documented in the glibc-internal futex API and in recent drafts of the
    Linux kernel's futex documentation (see the draft_futex branch of
    git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).

diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 80939ba..42e4ec9 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -232,16 +232,18 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
 	/* One less user.  */
 	--mutex->__data.__nusers;
 
-      /* Unlock.  */
+      /* Unlock.  Load all necessary mutex data before releasing the mutex
+	 to not violate the mutex destruction requirements (see
+	 lll_unlock).  */
+      int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
+      int private = (robust
+		     ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
+		     : PTHREAD_MUTEX_PSHARED (mutex));
       if ((mutex->__data.__lock & FUTEX_WAITERS) != 0
 	  || atomic_compare_and_exchange_bool_rel (&mutex->__data.__lock, 0,
 						   THREAD_GETMEM (THREAD_SELF,
 								  tid)))
 	{
-	  int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
-	  int private = (robust
-			 ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
-			 : PTHREAD_MUTEX_PSHARED (mutex));
 	  INTERNAL_SYSCALL_DECL (__err);
 	  INTERNAL_SYSCALL (futex, __err, 2, &mutex->__data.__lock,
 			    __lll_private_flag (FUTEX_UNLOCK_PI, private));
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
index 27f4142..d7eb282 100644
--- a/sysdeps/nptl/lowlevellock.h
+++ b/sysdeps/nptl/lowlevellock.h
@@ -191,14 +191,21 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    was >1 (acquired, possibly with waiters), then wake any waiters.  The waiter
-   that acquires the lock will set FUTEX to >1.  */
+   that acquires the lock will set FUTEX to >1.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements.  Specifically, we need to ensure that
+   another thread can destroy the mutex (and reuse its memory) once it
+   acquires the lock and when there will be no further lock acquisitions;
+   thus, we must not access the lock after releasing it, or those accesses
+   could be concurrent with mutex destruction or reuse of the memory.  */
 #define __lll_unlock(futex, private)                    \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int *__private = (private);                        \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval > 1))               \
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_unlock(futex, private)	\
   __lll_unlock (&(futex), private)
@@ -209,14 +216,17 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    had FUTEX_WAITERS set then wake any waiters.  The waiter that acquires the
-   lock will set FUTEX_WAITERS.  */
+   lock will set FUTEX_WAITERS.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements (see __lll_unlock).  */
 #define __lll_robust_unlock(futex, private)             \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int *__private = (private);                        \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval & FUTEX_WAITERS))	\
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_robust_unlock(futex, private)       \
   __lll_robust_unlock (&(futex), private)
diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
index 7608c01..9fa7337 100644
--- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
@@ -126,17 +126,19 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_24_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val > 1))					      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_robust_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val & FUTEX_WAITERS))			      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_islocked(futex) \

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Roland McGrath]

> Not tested.  Could someone test this on a non-x86-linux machine (I don't
> have one handy).  Dave, could you test on sparc, please?

You should always at least build-test your changes.

>  #define __lll_unlock(futex, private)                    \
>    ((void)                                               \
>     ({                                                   \
>       int *__futex = (futex);                            \
> +     int *__private = (private);                        \

Should be int, not int *.

>  #define __lll_robust_unlock(futex, private)             \
>    ((void)                                               \
>     ({                                                   \
>       int *__futex = (futex);                            \
> +     int *__private = (private);                        \

Same here.

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

On Tue, 2015-07-14 at 13:39 -0700, Roland McGrath wrote:
> > Not tested.  Could someone test this on a non-x86-linux machine (I don't
> > have one handy).  Dave, could you test on sparc, please?
> 
> You should always at least build-test your changes.

It does build on x86_64-linux, because those macros aren't used there ;)
I don't have a cross-build compiler ready, so I didn't try to build for
other archs.

> >  #define __lll_unlock(futex, private)                    \
> >    ((void)                                               \
> >     ({                                                   \
> >       int *__futex = (futex);                            \
> > +     int *__private = (private);                        \
> 
> Should be int, not int *.
> 
> >  #define __lll_robust_unlock(futex, private)             \
> >    ((void)                                               \
> >     ({                                                   \
> >       int *__futex = (futex);                            \
> > +     int *__private = (private);                        \
> 
> Same here.

Sorry.  Thanks for spotting these.  It seems I should also not try to
copy and paste late in the evening ;)

Updated patch attached.



[-- Attachment #2: mutex-destruction.patch --]
[-- Type: text/x-patch, Size: 6242 bytes --]

commit f727387d874ab9d76835d1aa99b6caf47d3e26f7
Author: Torvald Riegel <triegel@redhat.com>
Date:   Tue Jul 14 21:58:34 2015 +0200

    Do not violate mutex destruction requirements.
    
    POSIX and C++11 require that a thread can destroy a mutex if no other
    thread owns the mutex, is blocked on the mutex, or will try to acquire
    it in the future.  After destroying the mutex, it can reuse or unmap the
    underlying memory.  Thus, we must not access a mutex' memory after
    releasing it.  Currently, we can load the private flag after releasing
    the mutex, which is fixed by this patch.
    See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
    background.
    
    We need to call futex_wake on the lock after releasing it, however.  This
    is by design, and can lead to spurious wake-ups on unrelated futex words
    (e.g., when the mutex memory is reused for another mutex).  This behavior
    is documented in the glibc-internal futex API and in recent drafts of the
    Linux kernel's futex documentation (see the draft_futex branch of
    git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).

diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 80939ba..42e4ec9 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -232,16 +232,18 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
 	/* One less user.  */
 	--mutex->__data.__nusers;
 
-      /* Unlock.  */
+      /* Unlock.  Load all necessary mutex data before releasing the mutex
+	 to not violate the mutex destruction requirements (see
+	 lll_unlock).  */
+      int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
+      int private = (robust
+		     ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
+		     : PTHREAD_MUTEX_PSHARED (mutex));
       if ((mutex->__data.__lock & FUTEX_WAITERS) != 0
 	  || atomic_compare_and_exchange_bool_rel (&mutex->__data.__lock, 0,
 						   THREAD_GETMEM (THREAD_SELF,
 								  tid)))
 	{
-	  int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
-	  int private = (robust
-			 ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
-			 : PTHREAD_MUTEX_PSHARED (mutex));
 	  INTERNAL_SYSCALL_DECL (__err);
 	  INTERNAL_SYSCALL (futex, __err, 2, &mutex->__data.__lock,
 			    __lll_private_flag (FUTEX_UNLOCK_PI, private));
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
index 27f4142..7d41ef0 100644
--- a/sysdeps/nptl/lowlevellock.h
+++ b/sysdeps/nptl/lowlevellock.h
@@ -191,14 +191,21 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    was >1 (acquired, possibly with waiters), then wake any waiters.  The waiter
-   that acquires the lock will set FUTEX to >1.  */
+   that acquires the lock will set FUTEX to >1.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements.  Specifically, we need to ensure that
+   another thread can destroy the mutex (and reuse its memory) once it
+   acquires the lock and when there will be no further lock acquisitions;
+   thus, we must not access the lock after releasing it, or those accesses
+   could be concurrent with mutex destruction or reuse of the memory.  */
 #define __lll_unlock(futex, private)                    \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int __private = (private);                         \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval > 1))               \
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_unlock(futex, private)	\
   __lll_unlock (&(futex), private)
@@ -209,14 +216,17 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    had FUTEX_WAITERS set then wake any waiters.  The waiter that acquires the
-   lock will set FUTEX_WAITERS.  */
+   lock will set FUTEX_WAITERS.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements (see __lll_unlock).  */
 #define __lll_robust_unlock(futex, private)             \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int __private = (private);                         \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval & FUTEX_WAITERS))	\
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_robust_unlock(futex, private)       \
   __lll_robust_unlock (&(futex), private)
diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
index 7608c01..9fa7337 100644
--- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
@@ -126,17 +126,19 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_24_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val > 1))					      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_robust_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val & FUTEX_WAITERS))			      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_islocked(futex) \

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Carlos O'Donell]

On 07/14/2015 04:21 PM, Torvald Riegel wrote:
> Carlos, do you want to consider this for 2.22, or should this rather
> target 2.23?  The actual change is simple.

This is too late for 2.22.

Please discuss and push for 2.23.

Cheers,
Carlos.

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[David Miller]

From: Torvald Riegel <triegel@redhat.com>
Date: Tue, 14 Jul 2015 22:21:13 +0200

> Dave, could you test on sparc, please?

I would if the tree actually compiled on sparc.

In file included from initgroups.c:30:0:
../nscd/nscd-client.h: In function ‘__nscd_acquire_maplock’:
../nscd/nscd-client.h:381:7: error: expected expression before ‘)’ token
make[2]: *** [/home/davem/src/GIT/GLIBC/build-sparcv9/grp/initgroups.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from initgroups.c:30:0:
../nscd/nscd-client.h: In function ‘__nscd_acquire_maplock’:
../nscd/nscd-client.h:381:7: error: expected expression before ‘)’ token
make[2]: *** [/home/davem/src/GIT/GLIBC/build-sparcv9/grp/initgroups.os] Error 1
make[1]: *** [grp/subdir_lib] Error 2
make: *** [all] Error 2

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[David Miller]

From: Torvald Riegel <triegel@redhat.com>
Date: Tue, 14 Jul 2015 22:21:13 +0200

> Dave, could you test on sparc, please?

This breaks the sparc build:

gconv_db.c: In function ‘__gconv_find_transform’:
gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c: In function ‘__gconv_close_transform’:
gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
cc1: all warnings being treated as errors
make[2]: *** [/home/davem/src/GIT/GLIBC/build-sparcv9/iconv/gconv_db.o] Error 1
make[2]: *** Waiting for unfinished jobs....
gconv_db.c: In function ‘__gconv_find_transform’:
gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
gconv_db.c: In function ‘__gconv_close_transform’:
gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
cc1: all warnings being treated as errors

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

On Wed, 2015-07-15 at 16:05 -0700, David Miller wrote:
> From: Torvald Riegel <triegel@redhat.com>
> Date: Tue, 14 Jul 2015 22:21:13 +0200
> 
> > Dave, could you test on sparc, please?
> 
> This breaks the sparc build:
> 
> gconv_db.c: In function ‘__gconv_find_transform’:
> gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c: In function ‘__gconv_close_transform’:
> gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> cc1: all warnings being treated as errors
> make[2]: *** [/home/davem/src/GIT/GLIBC/build-sparcv9/iconv/gconv_db.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> gconv_db.c: In function ‘__gconv_find_transform’:
> gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c: In function ‘__gconv_close_transform’:
> gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> cc1: all warnings being treated as errors

This is surprising.  Unless I'm missing something else, it seems the
compiler is inferring that a lock is never in contended state (the only
use of __private is in the futex_wake call) -- but for that the compiler
would either have to analyze multi-threaded executions, or there are
cases when the lock isn't used.

It would be straight-forward to put an attribute((unused)) on __private,
but maybe we should investigate further what's really going on there.

Thoughts?

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Andreas Schwab]

David Miller <davem@davemloft.net> writes:

> From: Torvald Riegel <triegel@redhat.com>
> Date: Tue, 14 Jul 2015 22:21:13 +0200
>
>> Dave, could you test on sparc, please?
>
> This breaks the sparc build:
>
> gconv_db.c: In function ‘__gconv_find_transform’:
> gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c: In function ‘__gconv_close_transform’:
> gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> cc1: all warnings being treated as errors
> make[2]: *** [/home/davem/src/GIT/GLIBC/build-sparcv9/iconv/gconv_db.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> gconv_db.c: In function ‘__gconv_find_transform’:
> gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> gconv_db.c: In function ‘__gconv_close_transform’:
> gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> cc1: all warnings being treated as errors

#if IS_IN (libc) || IS_IN (rtld)
/* In libc.so or ld.so all futexes are private.  */
                       ^^^^^^^^^^^^^^^^^^^^^^^
# ifdef __ASSUME_PRIVATE_FUTEX
#  define __lll_private_flag(fl, private) \
  ((fl) | FUTEX_PRIVATE_FLAG)
# else
#  define __lll_private_flag(fl, private) \
  ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))
# endif

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Ondřej Bílka]

On Tue, Jul 14, 2015 at 10:21:13PM +0200, Torvald Riegel wrote:
> POSIX and C++11 require that a thread can destroy a mutex if no thread
> owns the mutex, is blocked on the mutex, or will try to acquire it in
> the future.  After destroying the mutex, it can reuse or unmap the
> underlying memory.  Thus, we must not access a mutex' memory after
> releasing it.  Currently, we can load the private flag after releasing
> the mutex, which is fixed by this patch.
> 
> See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
> background.
>     
> We need to call futex_wake on the lock after releasing it, however.
> This is by design, and can lead to spurious wake-ups on unrelated futex
> words (e.g., when the mutex memory is reused for another mutex).  This
> behavior is documented in the glibc-internal futex API and in recent
> drafts of the Linux kernel's futex documentation (see the draft_futex
> branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).
> 
> Carlos, do you want to consider this for 2.22, or should this rather
> target 2.23?  The actual change is simple.
> 
> Not tested.  Could someone test this on a non-x86-linux machine (I don't
> have one handy).  Dave, could you test on sparc, please?
> 
> 
> 2015-07-14  Torvald Riegel  <triegel@redhat.com>
> 
> 	[BZ #13690]
> 	* sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock
> 	after releasing it.
> 	(__lll_robust_unlock): Likewise.
> 	* nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
> 	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise.
> 	(lll_robust_unlock): Likewise.
>

So this is equivalent of my patch from 2013 where you said that you need
to ask posix with fixed bitrot?

 

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

On Tue, 2015-07-21 at 14:18 +0200, Ondřej Bílka wrote:
> On Tue, Jul 14, 2015 at 10:21:13PM +0200, Torvald Riegel wrote:
> > POSIX and C++11 require that a thread can destroy a mutex if no thread
> > owns the mutex, is blocked on the mutex, or will try to acquire it in
> > the future.  After destroying the mutex, it can reuse or unmap the
> > underlying memory.  Thus, we must not access a mutex' memory after
> > releasing it.  Currently, we can load the private flag after releasing
> > the mutex, which is fixed by this patch.
> > 
> > See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
> > background.
> >     
> > We need to call futex_wake on the lock after releasing it, however.
> > This is by design, and can lead to spurious wake-ups on unrelated futex
> > words (e.g., when the mutex memory is reused for another mutex).  This
> > behavior is documented in the glibc-internal futex API and in recent
> > drafts of the Linux kernel's futex documentation (see the draft_futex
> > branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).
> > 
> > Carlos, do you want to consider this for 2.22, or should this rather
> > target 2.23?  The actual change is simple.
> > 
> > Not tested.  Could someone test this on a non-x86-linux machine (I don't
> > have one handy).  Dave, could you test on sparc, please?
> > 
> > 
> > 2015-07-14  Torvald Riegel  <triegel@redhat.com>
> > 
> > 	[BZ #13690]
> > 	* sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock
> > 	after releasing it.
> > 	(__lll_robust_unlock): Likewise.
> > 	* nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
> > 	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise.
> > 	(lll_robust_unlock): Likewise.
> >
> 
> So this is equivalent of my patch from 2013

I don't remember such a patch, and you didn't gave a citation.

I thought we had fixed this already, saw that we didn't when browsing
through the concurrency-related bugs, and thus proposed the patch.

> where you said that you need
> to ask posix with fixed bitrot?

I don't understand this, sorry.

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Ondřej Bílka]

On Tue, Jul 21, 2015 at 06:43:47PM +0200, Torvald Riegel wrote:
> On Tue, 2015-07-21 at 14:18 +0200, Ondřej Bílka wrote:
> > On Tue, Jul 14, 2015 at 10:21:13PM +0200, Torvald Riegel wrote:
> > > POSIX and C++11 require that a thread can destroy a mutex if no thread
> > > owns the mutex, is blocked on the mutex, or will try to acquire it in
> > > the future.  After destroying the mutex, it can reuse or unmap the
> > > underlying memory.  Thus, we must not access a mutex' memory after
> > > releasing it.  Currently, we can load the private flag after releasing
> > > the mutex, which is fixed by this patch.
> > > 
> > > See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
> > > background.
> > >     
> > > We need to call futex_wake on the lock after releasing it, however.
> > > This is by design, and can lead to spurious wake-ups on unrelated futex
> > > words (e.g., when the mutex memory is reused for another mutex).  This
> > > behavior is documented in the glibc-internal futex API and in recent
> > > drafts of the Linux kernel's futex documentation (see the draft_futex
> > > branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).
> > > 
> > > Carlos, do you want to consider this for 2.22, or should this rather
> > > target 2.23?  The actual change is simple.
> > > 
> > > Not tested.  Could someone test this on a non-x86-linux machine (I don't
> > > have one handy).  Dave, could you test on sparc, please?
> > > 
> > > 
> > > 2015-07-14  Torvald Riegel  <triegel@redhat.com>
> > > 
> > > 	[BZ #13690]
> > > 	* sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock
> > > 	after releasing it.
> > > 	(__lll_robust_unlock): Likewise.
> > > 	* nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
> > > 	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise.
> > > 	(lll_robust_unlock): Likewise.
> > >
> > 
> > So this is equivalent of my patch from 2013
> 
> I don't remember such a patch, and you didn't gave a citation.
> 
> I thought we had fixed this already, saw that we didn't when browsing
> through the concurrency-related bugs, and thus proposed the patch.
> 
> > where you said that you need
> > to ask posix with fixed bitrot?
> 
> I don't understand this, sorry.
> 
As for bitrot my patch isn't valid as we meanwhile removed duplicate
lowlevellock.h

Then you said here following:

https://sourceware.org/ml/libc-ports/2013-12/msg00029.html


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.

Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

On Tue, 2015-07-21 at 19:14 +0200, Ondřej Bílka wrote:
> On Tue, Jul 21, 2015 at 06:43:47PM +0200, Torvald Riegel wrote:
> > On Tue, 2015-07-21 at 14:18 +0200, Ondřej Bílka wrote:
> > > On Tue, Jul 14, 2015 at 10:21:13PM +0200, Torvald Riegel wrote:
> > > > POSIX and C++11 require that a thread can destroy a mutex if no thread
> > > > owns the mutex, is blocked on the mutex, or will try to acquire it in
> > > > the future.  After destroying the mutex, it can reuse or unmap the
> > > > underlying memory.  Thus, we must not access a mutex' memory after
> > > > releasing it.  Currently, we can load the private flag after releasing
> > > > the mutex, which is fixed by this patch.
> > > > 
> > > > See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
> > > > background.
> > > >     
> > > > We need to call futex_wake on the lock after releasing it, however.
> > > > This is by design, and can lead to spurious wake-ups on unrelated futex
> > > > words (e.g., when the mutex memory is reused for another mutex).  This
> > > > behavior is documented in the glibc-internal futex API and in recent
> > > > drafts of the Linux kernel's futex documentation (see the draft_futex
> > > > branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).
> > > > 
> > > > Carlos, do you want to consider this for 2.22, or should this rather
> > > > target 2.23?  The actual change is simple.
> > > > 
> > > > Not tested.  Could someone test this on a non-x86-linux machine (I don't
> > > > have one handy).  Dave, could you test on sparc, please?
> > > > 
> > > > 
> > > > 2015-07-14  Torvald Riegel  <triegel@redhat.com>
> > > > 
> > > > 	[BZ #13690]
> > > > 	* sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock
> > > > 	after releasing it.
> > > > 	(__lll_robust_unlock): Likewise.
> > > > 	* nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
> > > > 	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise.
> > > > 	(lll_robust_unlock): Likewise.
> > > >
> > > 
> > > So this is equivalent of my patch from 2013
> > 
> > I don't remember such a patch, and you didn't gave a citation.
> > 
> > I thought we had fixed this already, saw that we didn't when browsing
> > through the concurrency-related bugs, and thus proposed the patch.
> > 
> > > where you said that you need
> > > to ask posix with fixed bitrot?
> > 
> > I don't understand this, sorry.
> > 
> As for bitrot my patch isn't valid as we meanwhile removed duplicate
> lowlevellock.h

It also seems to redefine lll_unlock, and it doesn't cover all the
problematic cases (see my patch, there are more than in lll_unlock).

I also didn't remember that you had posted a patch about that, and you
didn't follow up on it.  

> Then you said here following:
> 
> https://sourceware.org/ml/libc-ports/2013-12/msg00029.html
> 
> 
> 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.
> 
> 

POSIX has clarified (http://austingroupbugs.net/view.php?id=811) and we
have resolved the futex spurious wake-up issue (or, agreed on a status
quo with the kernel folks).  Thus, those things are settled and what
remained was to fix the order of the loads from private together and the
actual unlock atomic operation.

[PATCH v2][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

[-- Attachment #1: Type: text/plain, Size: 2926 bytes --]

On Thu, 2015-07-16 at 16:09 +0200, Andreas Schwab wrote:
> David Miller <davem@davemloft.net> writes:
> 
> > From: Torvald Riegel <triegel@redhat.com>
> > Date: Tue, 14 Jul 2015 22:21:13 +0200
> >
> >> Dave, could you test on sparc, please?
> >
> > This breaks the sparc build:
> >
> > gconv_db.c: In function ‘__gconv_find_transform’:
> > gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c: In function ‘__gconv_close_transform’:
> > gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> > cc1: all warnings being treated as errors
> > make[2]: *** [/home/davem/src/GIT/GLIBC/build-sparcv9/iconv/gconv_db.o] Error 1
> > make[2]: *** Waiting for unfinished jobs....
> > gconv_db.c: In function ‘__gconv_find_transform’:
> > gconv_db.c:734:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c:741:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c:760:7: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c:768:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> > gconv_db.c: In function ‘__gconv_close_transform’:
> > gconv_db.c:802:3: error: unused variable ‘__private’ [-Werror=unused-variable]
> > cc1: all warnings being treated as errors
> 
> #if IS_IN (libc) || IS_IN (rtld)
> /* In libc.so or ld.so all futexes are private.  */
>                        ^^^^^^^^^^^^^^^^^^^^^^^
> # ifdef __ASSUME_PRIVATE_FUTEX
> #  define __lll_private_flag(fl, private) \
>   ((fl) | FUTEX_PRIVATE_FLAG)
> # else
> #  define __lll_private_flag(fl, private) \
>   ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))
> # endif
> 
> Andreas.
> 

Updated patch attached.  I tried to prevent this in __lll_private_flag
by assigning PRIVATE to another variable that's marked as unused.
Dave, could you please give this another test as I still don't have a
cross-compiler handy and thus haven't tested it?  Thanks!

OK for trunk if Dave's testing is OK?

2015-08-24  Torvald Riegel  <triegel@redhat.com>

        [BZ #13690]
        * sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock
        after releasing it.
        (__lll_robust_unlock): Likewise.
        * nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
        * sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise.
        (lll_robust_unlock): Likewise.
        * sysdeps/unix/sysv/linux/lowlevellock-futex.h (__lll_private_flag):
        Prevent warnings in callers.


[-- Attachment #2: mutex-destruction.patch --]
[-- Type: text/x-patch, Size: 7079 bytes --]

commit dece11a4200d1be946332c2b62f2444b4881d08b
Author: Torvald Riegel <triegel@redhat.com>
Date:   Tue Jul 14 21:58:34 2015 +0200

    Do not violate mutex destruction requirements.
    
    POSIX and C++11 require that a thread can destroy a mutex if no other
    thread owns the mutex, is blocked on the mutex, or will try to acquire
    it in the future.  After destroying the mutex, it can reuse or unmap the
    underlying memory.  Thus, we must not access a mutex' memory after
    releasing it.  Currently, we can load the private flag after releasing
    the mutex, which is fixed by this patch.
    See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
    background.
    
    We need to call futex_wake on the lock after releasing it, however.  This
    is by design, and can lead to spurious wake-ups on unrelated futex words
    (e.g., when the mutex memory is reused for another mutex).  This behavior
    is documented in the glibc-internal futex API and in recent drafts of the
    Linux kernel's futex documentation (see the draft_futex branch of
    git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).

diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 80939ba..42e4ec9 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -232,16 +232,18 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
 	/* One less user.  */
 	--mutex->__data.__nusers;
 
-      /* Unlock.  */
+      /* Unlock.  Load all necessary mutex data before releasing the mutex
+	 to not violate the mutex destruction requirements (see
+	 lll_unlock).  */
+      int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
+      int private = (robust
+		     ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
+		     : PTHREAD_MUTEX_PSHARED (mutex));
       if ((mutex->__data.__lock & FUTEX_WAITERS) != 0
 	  || atomic_compare_and_exchange_bool_rel (&mutex->__data.__lock, 0,
 						   THREAD_GETMEM (THREAD_SELF,
 								  tid)))
 	{
-	  int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
-	  int private = (robust
-			 ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
-			 : PTHREAD_MUTEX_PSHARED (mutex));
 	  INTERNAL_SYSCALL_DECL (__err);
 	  INTERNAL_SYSCALL (futex, __err, 2, &mutex->__data.__lock,
 			    __lll_private_flag (FUTEX_UNLOCK_PI, private));
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
index 27f4142..7d41ef0 100644
--- a/sysdeps/nptl/lowlevellock.h
+++ b/sysdeps/nptl/lowlevellock.h
@@ -191,14 +191,21 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    was >1 (acquired, possibly with waiters), then wake any waiters.  The waiter
-   that acquires the lock will set FUTEX to >1.  */
+   that acquires the lock will set FUTEX to >1.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements.  Specifically, we need to ensure that
+   another thread can destroy the mutex (and reuse its memory) once it
+   acquires the lock and when there will be no further lock acquisitions;
+   thus, we must not access the lock after releasing it, or those accesses
+   could be concurrent with mutex destruction or reuse of the memory.  */
 #define __lll_unlock(futex, private)                    \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int __private = (private);                         \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval > 1))               \
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_unlock(futex, private)	\
   __lll_unlock (&(futex), private)
@@ -209,14 +216,17 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    had FUTEX_WAITERS set then wake any waiters.  The waiter that acquires the
-   lock will set FUTEX_WAITERS.  */
+   lock will set FUTEX_WAITERS.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements (see __lll_unlock).  */
 #define __lll_robust_unlock(futex, private)             \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int __private = (private);                         \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval & FUTEX_WAITERS))	\
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_robust_unlock(futex, private)       \
   __lll_robust_unlock (&(futex), private)
diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
index 59f6627..e1a9c58 100644
--- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
+++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
@@ -54,8 +54,13 @@
 #if IS_IN (libc) || IS_IN (rtld)
 /* In libc.so or ld.so all futexes are private.  */
 # ifdef __ASSUME_PRIVATE_FUTEX
-#  define __lll_private_flag(fl, private) \
-  ((fl) | FUTEX_PRIVATE_FLAG)
+#  define __lll_private_flag(fl, private)			\
+  ({								\
+    /* Prevent warnings in callers of this macro.  */		\
+    int __lll_private_flag_priv __attribute__ ((unused));	\
+    __lll_private_flag_priv = private;				\
+    ((fl) | FUTEX_PRIVATE_FLAG);				\
+  })
 # else
 #  define __lll_private_flag(fl, private) \
   ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))
diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
index 7608c01..9fa7337 100644
--- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
@@ -126,17 +126,19 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_24_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val > 1))					      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_robust_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val & FUTEX_WAITERS))			      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_islocked(futex) \

Re: [PATCH v2][BZ 13690] Do not violate mutex destruction requirements.

[Andreas Schwab]

Torvald Riegel <triegel@redhat.com> writes:

> diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> index 59f6627..e1a9c58 100644
> --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> @@ -54,8 +54,13 @@
>  #if IS_IN (libc) || IS_IN (rtld)
>  /* In libc.so or ld.so all futexes are private.  */
>  # ifdef __ASSUME_PRIVATE_FUTEX
> -#  define __lll_private_flag(fl, private) \
> -  ((fl) | FUTEX_PRIVATE_FLAG)
> +#  define __lll_private_flag(fl, private)			\
> +  ({								\
> +    /* Prevent warnings in callers of this macro.  */		\
> +    int __lll_private_flag_priv __attribute__ ((unused));	\
> +    __lll_private_flag_priv = private;				\

Macro parameters should be parenthesized.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH v3][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On Mon, 2015-08-24 at 09:32 +0200, Andreas Schwab wrote:
> Torvald Riegel <triegel@redhat.com> writes:
> 
> > diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > index 59f6627..e1a9c58 100644
> > --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > @@ -54,8 +54,13 @@
> >  #if IS_IN (libc) || IS_IN (rtld)
> >  /* In libc.so or ld.so all futexes are private.  */
> >  # ifdef __ASSUME_PRIVATE_FUTEX
> > -#  define __lll_private_flag(fl, private) \
> > -  ((fl) | FUTEX_PRIVATE_FLAG)
> > +#  define __lll_private_flag(fl, private)			\
> > +  ({								\
> > +    /* Prevent warnings in callers of this macro.  */		\
> > +    int __lll_private_flag_priv __attribute__ ((unused));	\
> > +    __lll_private_flag_priv = private;				\
> 
> Macro parameters should be parenthesized.

You're right, obviously.  Update attached.

On the positive side, this patch is small enough that I should
eventually run out of opportunities for stupid mistakes like this one ;)

[-- Attachment #2: mutex-destruction.patch --]
[-- Type: text/x-patch, Size: 7080 bytes --]

commit 7f57dfafecd59f326213ca743c0ee07c4d227af8
Author: Torvald Riegel <triegel@redhat.com>
Date:   Tue Jul 14 21:58:34 2015 +0200

    Do not violate mutex destruction requirements.
    
    POSIX and C++11 require that a thread can destroy a mutex if no other
    thread owns the mutex, is blocked on the mutex, or will try to acquire
    it in the future.  After destroying the mutex, it can reuse or unmap the
    underlying memory.  Thus, we must not access a mutex' memory after
    releasing it.  Currently, we can load the private flag after releasing
    the mutex, which is fixed by this patch.
    See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
    background.
    
    We need to call futex_wake on the lock after releasing it, however.  This
    is by design, and can lead to spurious wake-ups on unrelated futex words
    (e.g., when the mutex memory is reused for another mutex).  This behavior
    is documented in the glibc-internal futex API and in recent drafts of the
    Linux kernel's futex documentation (see the draft_futex branch of
    git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).

diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 80939ba..42e4ec9 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -232,16 +232,18 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
 	/* One less user.  */
 	--mutex->__data.__nusers;
 
-      /* Unlock.  */
+      /* Unlock.  Load all necessary mutex data before releasing the mutex
+	 to not violate the mutex destruction requirements (see
+	 lll_unlock).  */
+      int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
+      int private = (robust
+		     ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
+		     : PTHREAD_MUTEX_PSHARED (mutex));
       if ((mutex->__data.__lock & FUTEX_WAITERS) != 0
 	  || atomic_compare_and_exchange_bool_rel (&mutex->__data.__lock, 0,
 						   THREAD_GETMEM (THREAD_SELF,
 								  tid)))
 	{
-	  int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
-	  int private = (robust
-			 ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
-			 : PTHREAD_MUTEX_PSHARED (mutex));
 	  INTERNAL_SYSCALL_DECL (__err);
 	  INTERNAL_SYSCALL (futex, __err, 2, &mutex->__data.__lock,
 			    __lll_private_flag (FUTEX_UNLOCK_PI, private));
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
index 27f4142..7d41ef0 100644
--- a/sysdeps/nptl/lowlevellock.h
+++ b/sysdeps/nptl/lowlevellock.h
@@ -191,14 +191,21 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    was >1 (acquired, possibly with waiters), then wake any waiters.  The waiter
-   that acquires the lock will set FUTEX to >1.  */
+   that acquires the lock will set FUTEX to >1.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements.  Specifically, we need to ensure that
+   another thread can destroy the mutex (and reuse its memory) once it
+   acquires the lock and when there will be no further lock acquisitions;
+   thus, we must not access the lock after releasing it, or those accesses
+   could be concurrent with mutex destruction or reuse of the memory.  */
 #define __lll_unlock(futex, private)                    \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int __private = (private);                         \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval > 1))               \
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_unlock(futex, private)	\
   __lll_unlock (&(futex), private)
@@ -209,14 +216,17 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
    that's cast to void.  */
 /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock.  If FUTEX
    had FUTEX_WAITERS set then wake any waiters.  The waiter that acquires the
-   lock will set FUTEX_WAITERS.  */
+   lock will set FUTEX_WAITERS.
+   Evaluate PRIVATE before releasing the lock so that we do not violate the
+   mutex destruction requirements (see __lll_unlock).  */
 #define __lll_robust_unlock(futex, private)             \
   ((void)                                               \
    ({                                                   \
      int *__futex = (futex);                            \
+     int __private = (private);                         \
      int __oldval = atomic_exchange_rel (__futex, 0);   \
      if (__glibc_unlikely (__oldval & FUTEX_WAITERS))	\
-       lll_futex_wake (__futex, 1, private);            \
+       lll_futex_wake (__futex, 1, __private);          \
    }))
 #define lll_robust_unlock(futex, private)       \
   __lll_robust_unlock (&(futex), private)
diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
index 59f6627..40825f0 100644
--- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
+++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
@@ -54,8 +54,13 @@
 #if IS_IN (libc) || IS_IN (rtld)
 /* In libc.so or ld.so all futexes are private.  */
 # ifdef __ASSUME_PRIVATE_FUTEX
-#  define __lll_private_flag(fl, private) \
-  ((fl) | FUTEX_PRIVATE_FLAG)
+#  define __lll_private_flag(fl, private)			\
+  ({								\
+    /* Prevent warnings in callers of this macro.  */		\
+    int __lll_private_flag_priv __attribute__ ((unused));	\
+    __lll_private_flag_priv = (private);			\
+    ((fl) | FUTEX_PRIVATE_FLAG);				\
+  })
 # else
 #  define __lll_private_flag(fl, private) \
   ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))
diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
index 7608c01..9fa7337 100644
--- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
@@ -126,17 +126,19 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime,
 #define lll_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_24_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val > 1))					      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_robust_unlock(lock, private) \
   ((void) ({								      \
     int *__futex = &(lock);						      \
+    int __private = (private);						      \
     int __val = atomic_exchange_rel (__futex, 0);			      \
     if (__glibc_unlikely (__val & FUTEX_WAITERS))			      \
-      lll_futex_wake (__futex, 1, private);				      \
+      lll_futex_wake (__futex, 1, __private);				      \
   }))
 
 #define lll_islocked(futex) \

Re: [PATCH v3][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

Ping.

On Mon, 2015-08-24 at 09:43 +0200, Torvald Riegel wrote:
> On Mon, 2015-08-24 at 09:32 +0200, Andreas Schwab wrote:
> > Torvald Riegel <triegel@redhat.com> writes:
> > 
> > > diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > > index 59f6627..e1a9c58 100644
> > > --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > > +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > > @@ -54,8 +54,13 @@
> > >  #if IS_IN (libc) || IS_IN (rtld)
> > >  /* In libc.so or ld.so all futexes are private.  */
> > >  # ifdef __ASSUME_PRIVATE_FUTEX
> > > -#  define __lll_private_flag(fl, private) \
> > > -  ((fl) | FUTEX_PRIVATE_FLAG)
> > > +#  define __lll_private_flag(fl, private)			\
> > > +  ({								\
> > > +    /* Prevent warnings in callers of this macro.  */		\
> > > +    int __lll_private_flag_priv __attribute__ ((unused));	\
> > > +    __lll_private_flag_priv = private;				\
> > 
> > Macro parameters should be parenthesized.
> 
> You're right, obviously.  Update attached.
> 
> On the positive side, this patch is small enough that I should
> eventually run out of opportunities for stupid mistakes like this one ;)

Re: [PATCH v3][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel]

On Mon, 2015-08-24 at 09:43 +0200, Torvald Riegel wrote:
> On Mon, 2015-08-24 at 09:32 +0200, Andreas Schwab wrote:
> > Torvald Riegel <triegel@redhat.com> writes:
> > 
> > > diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > > index 59f6627..e1a9c58 100644
> > > --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > > +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> > > @@ -54,8 +54,13 @@
> > >  #if IS_IN (libc) || IS_IN (rtld)
> > >  /* In libc.so or ld.so all futexes are private.  */
> > >  # ifdef __ASSUME_PRIVATE_FUTEX
> > > -#  define __lll_private_flag(fl, private) \
> > > -  ((fl) | FUTEX_PRIVATE_FLAG)
> > > +#  define __lll_private_flag(fl, private)			\
> > > +  ({								\
> > > +    /* Prevent warnings in callers of this macro.  */		\
> > > +    int __lll_private_flag_priv __attribute__ ((unused));	\
> > > +    __lll_private_flag_priv = private;				\
> > 
> > Macro parameters should be parenthesized.
> 
> You're right, obviously.  Update attached.
> 
> On the positive side, this patch is small enough that I should
> eventually run out of opportunities for stupid mistakes like this one ;)

I have committed this now, given that I addressed Andreas' feedback,
lack of more recent reviews, and that we really want this fixed.