Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jan Stancek <jstancek@redhat.com>
To:     tglx@linutronix.de, mingo@redhat.com
Cc:     peterz@infradead.org, dvhart@infradead.org, longman@redhat.com,
        linux-kernel@vger.kernel.org, jstancek@redhat.com
Subject: [RFC PATCH] futex: don't retry futex_wait() with stale uaddr/val after spurious wakeup
Date:   Wed,  6 Nov 2019 23:43:19 +0100
Message-Id: <9179dbc3505e1de99ee36b09b0a12995239d73c3.1573079868.git.jstancek@redhat.com>
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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