futex: Sanitize cmpxchg_futex_value_locked API

The cmpxchg_futex_value_locked API was funny in that it returned either
the original, user-exposed futex value OR an error code such as -EFAULT.
This was confusing at best, and could be a source of livelocks in places
that retry the cmpxchg_futex_value_locked after trying to fix the issue
by running fault_in_user_writeable().
    
This change makes the cmpxchg_futex_value_locked API more similar to the
get_futex_value_locked one, returning an error code and updating the
original value through a reference argument.
    
Signed-off-by: Michel Lespinasse <walken@google.com>
Acked-by: Chris Metcalf <cmetcalf@tilera.com>  [tile]
Acked-by: Tony Luck <tony.luck@intel.com>  [ia64]
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Michal Simek <monstr@monstr.eu>  [microblaze]
Acked-by: David Howells <dhowells@redhat.com> [frv]
Cc: Darren Hart <darren@dvhart.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Matt Turner <mattst88@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: "James E.J. Bottomley" <jejb@parisc-linux.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
LKML-Reference: <20110311024851.GC26122@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

1 files changed, 15 insertions, 30 deletions

diff --git a/kernel/futex.c b/kernel/futex.c
index 773815465bac..237f14bfc022 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -381,15 +381,16 @@ static struct futex_q *futex_top_waiter(struct futex_hash_bucket *hb,
         return NULL;
 }
 
-static u32 cmpxchg_futex_value_locked(u32 __user *uaddr, u32 uval, u32 newval)
+static int cmpxchg_futex_value_locked(u32 *curval, u32 __user *uaddr,
+                                      u32 uval, u32 newval)
 {
-        u32 curval;
+        int ret;
 
         pagefault_disable();
-        curval = futex_atomic_cmpxchg_inatomic(uaddr, uval, newval);
+        ret = futex_atomic_cmpxchg_inatomic(curval, uaddr, uval, newval);
         pagefault_enable();
 
-        return curval;
+        return ret;
 }
 
 static int get_futex_value_locked(u32 *dest, u32 __user *from)
@@ -688,9 +689,7 @@ retry:
         if (set_waiters)
                 newval |= FUTEX_WAITERS;
 
-        curval = cmpxchg_futex_value_locked(uaddr, 0, newval);
-
-        if (unlikely(curval == -EFAULT))
+        if (unlikely(cmpxchg_futex_value_locked(&curval, uaddr, 0, newval)))
                 return -EFAULT;
 
         /*
@@ -728,9 +727,7 @@ retry:
                 lock_taken = 1;
         }
 
-        curval = cmpxchg_futex_value_locked(uaddr, uval, newval);
-
-        if (unlikely(curval == -EFAULT))
+        if (unlikely(cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)))
                 return -EFAULT;
         if (unlikely(curval != uval))
                 goto retry;
@@ -843,9 +840,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
 
                 newval = FUTEX_WAITERS | task_pid_vnr(new_owner);
 
-                curval = cmpxchg_futex_value_locked(uaddr, uval, newval);
-
-                if (curval == -EFAULT)
+                if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval))
                         ret = -EFAULT;
                 else if (curval != uval)
                         ret = -EINVAL;
@@ -880,10 +875,8 @@ static int unlock_futex_pi(u32 __user *uaddr, u32 uval)
          * There is no waiter, so we unlock the futex. The owner died
          * bit has not to be preserved here. We are the owner:
          */
-        oldval = cmpxchg_futex_value_locked(uaddr, uval, 0);
-
-        if (oldval == -EFAULT)
-                return oldval;
+        if (cmpxchg_futex_value_locked(&oldval, uaddr, uval, 0))
+                return -EFAULT;
         if (oldval != uval)
                 return -EAGAIN;
 
@@ -1578,9 +1571,7 @@ retry:
         while (1) {
                 newval = (uval & FUTEX_OWNER_DIED) | newtid;
 
-                curval = cmpxchg_futex_value_locked(uaddr, uval, newval);
-
-                if (curval == -EFAULT)
+                if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval))
                         goto handle_fault;
                 if (curval == uval)
                         break;
@@ -2073,11 +2064,8 @@ retry:
          * again. If it succeeds then we can return without waking
          * anyone else up:
          */
-        if (!(uval & FUTEX_OWNER_DIED))
-                uval = cmpxchg_futex_value_locked(uaddr, vpid, 0);
-
-
-        if (unlikely(uval == -EFAULT))
+        if (!(uval & FUTEX_OWNER_DIED) &&
+            cmpxchg_futex_value_locked(&uval, uaddr, vpid, 0))
                 goto pi_faulted;
         /*
          * Rare case: we managed to release the lock atomically,
@@ -2464,9 +2452,7 @@ retry:
                  * userspace.
                  */
                 mval = (uval & FUTEX_WAITERS) | FUTEX_OWNER_DIED;
-                nval = futex_atomic_cmpxchg_inatomic(uaddr, uval, mval);
-
-                if (nval == -EFAULT)
+                if (futex_atomic_cmpxchg_inatomic(&nval, uaddr, uval, mval))
                         return -1;
 
                 if (nval != uval)
@@ -2679,8 +2665,7 @@ static int __init futex_init(void)
          * implementation, the non-functional ones will return
          * -ENOSYS.
          */
-        curval = cmpxchg_futex_value_locked(NULL, 0, 0);
-        if (curval == -EFAULT)
+        if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
                 futex_cmpxchg_enabled = 1;
 
         for (i = 0; i < ARRAY_SIZE(futex_queues); i++) {