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

Message ID 1436905273.19451.14.camel@localhost.localdomain
State Dropped
Headers

Commit Message

Torvald Riegel July 14, 2015, 8:21 p.m. UTC
  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.
  

Comments

Roland McGrath July 14, 2015, 8:39 p.m. UTC | #1
> 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.
  
Carlos O'Donell July 14, 2015, 8:56 p.m. UTC | #2
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.
  
David Miller July 14, 2015, 9:31 p.m. UTC | #3
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
  
David Miller July 15, 2015, 11:05 p.m. UTC | #4
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
  
Torvald Riegel July 16, 2015, 10:59 a.m. UTC | #5
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?
  
Andreas Schwab July 16, 2015, 2:09 p.m. UTC | #6
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.
  
Ondrej Bilka July 21, 2015, 12:18 p.m. UTC | #7
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?
  
Torvald Riegel July 21, 2015, 4:43 p.m. UTC | #8
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.
  
Ondrej Bilka July 21, 2015, 5:14 p.m. UTC | #9
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.
  
Torvald Riegel July 21, 2015, 8:34 p.m. UTC | #10
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

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) \