Blob Blame History Raw
commit 823624bdc47f1f80109c9c52dee7939b9386d708
Author: Stefan Liebler <stli@linux.ibm.com>
Date:   Thu Feb 7 15:18:36 2019 +0100

    Add compiler barriers around modifications of the robust mutex list for pthread_mutex_trylock. [BZ #24180]

    While debugging a kernel warning, Thomas Gleixner, Sebastian Sewior and
    Heiko Carstens found a bug in pthread_mutex_trylock due to misordered
    instructions:
    140:   a5 1b 00 01             oill    %r1,1
    144:   e5 48 a0 f0 00 00       mvghi   240(%r10),0   <--- THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
    14a:   e3 10 a0 e0 00 24       stg     %r1,224(%r10) <--- last THREAD_SETMEM of ENQUEUE_MUTEX_PI

    vs (with compiler barriers):
    140:   a5 1b 00 01             oill    %r1,1
    144:   e3 10 a0 e0 00 24       stg     %r1,224(%r10)
    14a:   e5 48 a0 f0 00 00       mvghi   240(%r10),0

    Please have a look at the discussion:
    "Re: WARN_ON_ONCE(!new_owner) within wake_futex_pi() triggerede"
    (https://lore.kernel.org/lkml/20190202112006.GB3381@osiris/)

    This patch is introducing the same compiler barriers and comments
    for pthread_mutex_trylock as introduced for pthread_mutex_lock and
    pthread_mutex_timedlock by commit 8f9450a0b7a9e78267e8ae1ab1000ebca08e473e
    "Add compiler barriers around modifications of the robust mutex list."

    ChangeLog:

            [BZ #24180]
            * nptl/pthread_mutex_trylock.c (__pthread_mutex_trylock):


diff -Nrup a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c
--- a/nptl/pthread_mutex_trylock.c	2019-07-26 16:43:11.028271897 -0400
+++ b/nptl/pthread_mutex_trylock.c	2019-07-26 17:06:48.708748979 -0400
@@ -95,6 +95,9 @@ __pthread_mutex_trylock (pthread_mutex_t
     case PTHREAD_MUTEX_ROBUST_ADAPTIVE_NP:
       THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending,
 		     &mutex->__data.__list.__next);
+      /* We need to set op_pending before starting the operation.  Also
+	 see comments at ENQUEUE_MUTEX.  */
+      __asm ("" ::: "memory");
 
       oldval = mutex->__data.__lock;
       do
@@ -120,7 +123,12 @@ __pthread_mutex_trylock (pthread_mutex_t
 	      /* But it is inconsistent unless marked otherwise.  */
 	      mutex->__data.__owner = PTHREAD_MUTEX_INCONSISTENT;
 
+	      /* We must not enqueue the mutex before we have acquired it.
+		 Also see comments at ENQUEUE_MUTEX.  */
+	      __asm ("" ::: "memory");
 	      ENQUEUE_MUTEX (mutex);
+	      /* We need to clear op_pending after we enqueue the mutex.  */
+	      __asm ("" ::: "memory");
 	      THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 	      /* Note that we deliberately exist here.  If we fall
@@ -136,6 +144,8 @@ __pthread_mutex_trylock (pthread_mutex_t
 	      int kind = PTHREAD_MUTEX_TYPE (mutex);
 	      if (kind == PTHREAD_MUTEX_ROBUST_ERRORCHECK_NP)
 		{
+		  /* We do not need to ensure ordering wrt another memory
+		     access.  Also see comments at ENQUEUE_MUTEX. */
 		  THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending,
 				 NULL);
 		  return EDEADLK;
@@ -143,6 +153,8 @@ __pthread_mutex_trylock (pthread_mutex_t
 
 	      if (kind == PTHREAD_MUTEX_ROBUST_RECURSIVE_NP)
 		{
+		  /* We do not need to ensure ordering wrt another memory
+		     access.  */
 		  THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending,
 				 NULL);
 
@@ -160,6 +172,10 @@ __pthread_mutex_trylock (pthread_mutex_t
 	  oldval = lll_robust_trylock (mutex->__data.__lock, id);
 	  if (oldval != 0 && (oldval & FUTEX_OWNER_DIED) == 0)
 	    {
+             /* We haven't acquired the lock as it is already acquired by
+                another owner.  We do not need to ensure ordering wrt another
+                memory access.  */
+
 	      THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 	      return EBUSY;
@@ -173,13 +189,20 @@ __pthread_mutex_trylock (pthread_mutex_t
 	      if (oldval == id)
 		lll_unlock (mutex->__data.__lock,
 			    PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
+	      /* FIXME This violates the mutex destruction requirements.  See
+		 __pthread_mutex_unlock_full.  */
 	      THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 	      return ENOTRECOVERABLE;
 	    }
 	}
       while ((oldval & FUTEX_OWNER_DIED) != 0);
 
+      /* We must not enqueue the mutex before we have acquired it.
+	 Also see comments at ENQUEUE_MUTEX.  */
+      __asm ("" ::: "memory");
       ENQUEUE_MUTEX (mutex);
+      /* We need to clear op_pending after we enqueue the mutex.  */
+      __asm ("" ::: "memory");
       THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
       mutex->__data.__owner = id;
@@ -201,10 +224,15 @@ __pthread_mutex_trylock (pthread_mutex_t
 	int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
 
 	if (robust)
-	  /* Note: robust PI futexes are signaled by setting bit 0.  */
-	  THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending,
-			 (void *) (((uintptr_t) &mutex->__data.__list.__next)
-				   | 1));
+         {
+           /* Note: robust PI futexes are signaled by setting bit 0.  */
+           THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending,
+                          (void *) (((uintptr_t) &mutex->__data.__list.__next)
+                                    | 1));
+           /* We need to set op_pending before starting the operation.  Also
+              see comments at ENQUEUE_MUTEX.  */
+           __asm ("" ::: "memory");
+         }
 
 	oldval = mutex->__data.__lock;
 
@@ -213,12 +241,16 @@ __pthread_mutex_trylock (pthread_mutex_t
 	  {
 	    if (kind == PTHREAD_MUTEX_ERRORCHECK_NP)
 	      {
+		/* We do not need to ensure ordering wrt another memory
+		   access.  */
 		THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 		return EDEADLK;
 	      }
 
 	    if (kind == PTHREAD_MUTEX_RECURSIVE_NP)
 	      {
+		/* We do not need to ensure ordering wrt another memory
+		   access.  */
 		THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 		/* Just bump the counter.  */
@@ -240,6 +272,9 @@ __pthread_mutex_trylock (pthread_mutex_t
 	  {
 	    if ((oldval & FUTEX_OWNER_DIED) == 0)
 	      {
+		/* We haven't acquired the lock as it is already acquired by
+		   another owner.  We do not need to ensure ordering wrt another
+		   memory access.  */
 		THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 		return EBUSY;
@@ -260,6 +295,9 @@ __pthread_mutex_trylock (pthread_mutex_t
 	    if (INTERNAL_SYSCALL_ERROR_P (e, __err)
 		&& INTERNAL_SYSCALL_ERRNO (e, __err) == EWOULDBLOCK)
 	      {
+		/* The kernel has not yet finished the mutex owner death.
+		   We do not need to ensure ordering wrt another memory
+		   access.  */
 		THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 		return EBUSY;
@@ -277,7 +315,12 @@ __pthread_mutex_trylock (pthread_mutex_t
 	    /* But it is inconsistent unless marked otherwise.  */
 	    mutex->__data.__owner = PTHREAD_MUTEX_INCONSISTENT;
 
+	    /* We must not enqueue the mutex before we have acquired it.
+	       Also see comments at ENQUEUE_MUTEX.  */
+	    __asm ("" ::: "memory");
 	    ENQUEUE_MUTEX (mutex);
+	    /* We need to clear op_pending after we enqueue the mutex.  */
+	    __asm ("" ::: "memory");
 	    THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 	    /* Note that we deliberately exit here.  If we fall
@@ -300,13 +343,20 @@ __pthread_mutex_trylock (pthread_mutex_t
 						  PTHREAD_ROBUST_MUTEX_PSHARED (mutex)),
 			      0, 0);
 
+	    /* To the kernel, this will be visible after the kernel has
+	       acquired the mutex in the syscall.  */
 	    THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 	    return ENOTRECOVERABLE;
 	  }
 
 	if (robust)
 	  {
+	    /* We must not enqueue the mutex before we have acquired it.
+	       Also see comments at ENQUEUE_MUTEX.  */
+	    __asm ("" ::: "memory");
 	    ENQUEUE_MUTEX_PI (mutex);
+	    /* We need to clear op_pending after we enqueue the mutex.  */
+	    __asm ("" ::: "memory");
 	    THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 	  }