[PATCH] pthread_cond_wait: close window against priority inversion

[Steven Walter <stevenrwalter at gmail dot com>]

From: Steven Walter <swalter@lpdev.prtdev.lexmark.com>

Consider the situation where you have a condvar that is used in
conjunction with a priority-inheriting mutex.  When you call
pthread_cond_wait, the priority-inheriting mutex gets dropped, but not
before we grab the internal condvar lock.  If the PI mutex was
contended, we will get preempted with no PI protection.  If another
thread then calls pthread_cond_wait, it will block on the internal condvar
lock, while holding the PI mutex.  We are now priority inverted,
dependent on the scheduler to run the low-priority holder of the condvar
lock.

Fortunately, it seems that we can simply hold the PI lock for longer (in
fact, for the entire time we hold the internal condvar lock) to protect
against this window.


2010-09-24  Steven Walter  <stevenrwalter@gmail.com>

	* nptl/pthread_cond_wait.c (__pthread_cond_wait):  Hold
	priority-inheriting mutexes until after dropping
	cond->__data.__lock.  Holding the PI mutex longer prevents
	priority-inversion from occurring in __lock, which is not
	priority-inheriting.
---
 nptl/pthread_cond_wait.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
index 670fba5..71a3273 100644
--- a/nptl/pthread_cond_wait.c
+++ b/nptl/pthread_cond_wait.c
@@ -98,6 +98,7 @@ __pthread_cond_wait (cond, mutex)
   struct _pthread_cleanup_buffer buffer;
   struct _condvar_cleanup_buffer cbuffer;
   int err;
+  int unlock = 1;
   int pshared = (cond->__data.__mutex == (void *) ~0l)
		? LLL_SHARED : LLL_PRIVATE;
 
@@ -105,11 +106,18 @@ __pthread_cond_wait (cond, mutex)
   lll_lock (cond->__data.__lock, pshared);
 
   /* Now we can release the mutex.  */
-  err = __pthread_mutex_unlock_usercnt (mutex, 0);
-  if (__builtin_expect (err, 0))
+  /* SRW: we only want to release the mutex now if it is not
+   * priority-inheriting.  If it *is* priority inheriting, we want to release
+   * it at the last possible moment so that we remain protected against PI
+   * for as long as possible */
+  if (!(mutex->__data.__kind & PTHREAD_MUTEX_PRIO_INHERIT_NP))
     {
-      lll_unlock (cond->__data.__lock, pshared);
-      return err;
+      err = __pthread_mutex_unlock_usercnt (mutex, 0);
+      if (__builtin_expect (err, 0))
+	{
+          lll_unlock (cond->__data.__lock, pshared);
+	  return err;
+	}
     }
 
   /* We have one new user of the condvar.  */
@@ -146,6 +154,19 @@ __pthread_cond_wait (cond, mutex)
       /* Prepare to wait.  Release the condvar futex.  */
       lll_unlock (cond->__data.__lock, pshared);
 
+      /* SRW: go ahead and release the PI mutex now that we're no longer
+       * holding the condvar's mutex. */
+      if ((mutex->__data.__kind & PTHREAD_MUTEX_PRIO_INHERIT_NP) && unlock)
+	{
+	  unlock = 0;
+	  err = __pthread_mutex_unlock_usercnt (mutex, 0);
+	  if (__builtin_expect (err, 0))
+	    {
+	      lll_unlock (cond->__data.__lock, pshared);
+	      return err;
+	    }
+	}
+
       /* Enable asynchronous cancellation.  Required by the standard.  */
       cbuffer.oldtype = __pthread_enable_asynccancel ();
 
-- 
1.7.3.4.g4d78d