[RFC PATCH] futex: don't retry futex_wait() with stale uaddr/val after spurious wakeup

* [RFC PATCH] futex: don't retry futex_wait() with stale uaddr/val after spurious wakeup
@ 2019-11-06 22:43 Jan Stancek
  2019-11-07 11:11 ` Thomas Gleixner
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Jan Stancek @ 2019-11-06 22:43 UTC (permalink / raw)
  To: tglx, mingo; +Cc: peterz, dvhart, longman, linux-kernel, jstancek

Assume following scenario: process A is sleeping on futex (uaddr1)
inside futex_wait(). Process B requeues this waiter via FUTEX_CMP_REQUEUE
to uaddr2, but doesn't wake it up. Later, process A spuriously wakes up
and futex_wait() retries to queue again with same uaddr1/val1:
        if (!signal_pending(current))
                goto retry;

Problem: Userspace fails to wake process A with futex(uaddr2, FUTEX_WAKE)

Store target uaddr/val in futex_requeue() and let futex_wait()
retry after spurious wake up using stored values.

Fixes: d58e6576b0de ("futex: Handle spurious wake up")
Signed-off-by: Jan Stancek <jstancek@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Waiman Long <longman@redhat.com>
---
 kernel/futex.c | 52 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index bd18f60e4c6c..c13cfee25d35 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -237,6 +237,9 @@ struct futex_q {
 	struct rt_mutex_waiter *rt_waiter;
 	union futex_key *requeue_pi_key;
 	u32 bitset;
+
+	u32 *uaddr;
+	u32 val;
 } __randomize_layout;
 
 static const struct futex_q futex_q_init = {
@@ -1939,6 +1942,7 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 	struct futex_hash_bucket *hb1, *hb2;
 	struct futex_q *this, *next;
 	DEFINE_WAKE_Q(wake_q);
+	u32 curval, targetval;
 
 	if (nr_wake < 0 || nr_requeue < 0)
 		return -EINVAL;
@@ -2005,30 +2009,32 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 	hb_waiters_inc(hb2);
 	double_lock_hb(hb1, hb2);
 
-	if (likely(cmpval != NULL)) {
-		u32 curval;
-
+	ret = get_futex_value_locked(&targetval, uaddr2);
+	if (!ret && likely(cmpval != NULL))
 		ret = get_futex_value_locked(&curval, uaddr1);
 
-		if (unlikely(ret)) {
-			double_unlock_hb(hb1, hb2);
-			hb_waiters_dec(hb2);
+	if (unlikely(ret)) {
+		double_unlock_hb(hb1, hb2);
+		hb_waiters_dec(hb2);
 
+		ret = get_user(targetval, uaddr2);
+		if (!ret && likely(cmpval != NULL))
 			ret = get_user(curval, uaddr1);
-			if (ret)
-				goto out_put_keys;
 
-			if (!(flags & FLAGS_SHARED))
-				goto retry_private;
+		if (ret)
+			goto out_put_keys;
 
-			put_futex_key(&key2);
-			put_futex_key(&key1);
-			goto retry;
-		}
-		if (curval != *cmpval) {
-			ret = -EAGAIN;
-			goto out_unlock;
-		}
+		if (!(flags & FLAGS_SHARED))
+			goto retry_private;
+
+		put_futex_key(&key2);
+		put_futex_key(&key1);
+		goto retry;
+	}
+
+	if (likely(cmpval != NULL) && (curval != *cmpval)) {
+		ret = -EAGAIN;
+		goto out_unlock;
 	}
 
 	if (requeue_pi && (task_count - nr_wake < nr_requeue)) {
@@ -2185,6 +2191,12 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 			}
 		}
 		requeue_futex(this, hb1, hb2, &key2);
+		/*
+		 * Save target uaddr/val, in case futex_wait() wakes
+		 * up spuriously and retries to requeue.
+		 */
+		this->uaddr = uaddr2;
+		this->val = targetval;
 		drop_count++;
 	}
 
@@ -2717,6 +2729,8 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 	if (!bitset)
 		return -EINVAL;
 	q.bitset = bitset;
+	q.uaddr = uaddr;
+	q.val = val;
 
 	to = futex_setup_timer(abs_time, &timeout, flags,
 			       current->timer_slack_ns);
@@ -2725,7 +2739,7 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 	 * Prepare to wait on uaddr. On success, holds hb lock and increments
 	 * q.key refs.
 	 */
-	ret = futex_wait_setup(uaddr, val, flags, &q, &hb);
+	ret = futex_wait_setup(q.uaddr, q.val, flags, &q, &hb);
 	if (ret)
 		goto out;
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread