[GIT PULL] locking fixes

[GIT PULL] locking fixes

[Ingo Molnar]

Linus,

Please pull the latest locking-urgent-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-urgent-for-linus

   # HEAD: fbe0e839d1e22d88810f3ee3e2f1479be4c0aa4a futex: Prevent overflow by strengthen input validation

Two futex fixes: a input parameters robustness fix, and futex race fixes.

 Thanks,

	Ingo

------------------>
Li Jinyue (1):
      futex: Prevent overflow by strengthen input validation

Peter Zijlstra (1):
      futex: Avoid violating the 10th rule of futex


 kernel/futex.c                  | 86 +++++++++++++++++++++++++++++++++--------
 kernel/locking/rtmutex.c        | 26 +++++++++----
 kernel/locking/rtmutex_common.h |  1 +
 3 files changed, 90 insertions(+), 23 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 57d0b3657e16..8c5424dd5924 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1878,6 +1878,9 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 	struct futex_q *this, *next;
 	DEFINE_WAKE_Q(wake_q);
 
+	if (nr_wake < 0 || nr_requeue < 0)
+		return -EINVAL;
+
 	/*
 	 * When PI not supported: return -ENOSYS if requeue_pi is true,
 	 * consequently the compiler knows requeue_pi is always false past
@@ -2294,21 +2297,17 @@ static void unqueue_me_pi(struct futex_q *q)
 	spin_unlock(q->lock_ptr);
 }
 
-/*
- * Fixup the pi_state owner with the new owner.
- *
- * Must be called with hash bucket lock held and mm->sem held for non
- * private futexes.
- */
 static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
-				struct task_struct *newowner)
+				struct task_struct *argowner)
 {
-	u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
 	struct futex_pi_state *pi_state = q->pi_state;
 	u32 uval, uninitialized_var(curval), newval;
-	struct task_struct *oldowner;
+	struct task_struct *oldowner, *newowner;
+	u32 newtid;
 	int ret;
 
+	lockdep_assert_held(q->lock_ptr);
+
 	raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
 
 	oldowner = pi_state->owner;
@@ -2317,11 +2316,17 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 		newtid |= FUTEX_OWNER_DIED;
 
 	/*
-	 * We are here either because we stole the rtmutex from the
-	 * previous highest priority waiter or we are the highest priority
-	 * waiter but have failed to get the rtmutex the first time.
+	 * We are here because either:
+	 *
+	 *  - we stole the lock and pi_state->owner needs updating to reflect
+	 *    that (@argowner == current),
+	 *
+	 * or:
+	 *
+	 *  - someone stole our lock and we need to fix things to point to the
+	 *    new owner (@argowner == NULL).
 	 *
-	 * We have to replace the newowner TID in the user space variable.
+	 * Either way, we have to replace the TID in the user space variable.
 	 * This must be atomic as we have to preserve the owner died bit here.
 	 *
 	 * Note: We write the user space value _before_ changing the pi_state
@@ -2334,6 +2339,42 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	 * in the PID check in lookup_pi_state.
 	 */
 retry:
+	if (!argowner) {
+		if (oldowner != current) {
+			/*
+			 * We raced against a concurrent self; things are
+			 * already fixed up. Nothing to do.
+			 */
+			ret = 0;
+			goto out_unlock;
+		}
+
+		if (__rt_mutex_futex_trylock(&pi_state->pi_mutex)) {
+			/* We got the lock after all, nothing to fix. */
+			ret = 0;
+			goto out_unlock;
+		}
+
+		/*
+		 * Since we just failed the trylock; there must be an owner.
+		 */
+		newowner = rt_mutex_owner(&pi_state->pi_mutex);
+		BUG_ON(!newowner);
+	} else {
+		WARN_ON_ONCE(argowner != current);
+		if (oldowner == current) {
+			/*
+			 * We raced against a concurrent self; things are
+			 * already fixed up. Nothing to do.
+			 */
+			ret = 0;
+			goto out_unlock;
+		}
+		newowner = argowner;
+	}
+
+	newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
+
 	if (get_futex_value_locked(&uval, uaddr))
 		goto handle_fault;
 
@@ -2434,15 +2475,28 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
 		 * Got the lock. We might not be the anticipated owner if we
 		 * did a lock-steal - fix up the PI-state in that case:
 		 *
-		 * We can safely read pi_state->owner without holding wait_lock
-		 * because we now own the rt_mutex, only the owner will attempt
-		 * to change it.
+		 * Speculative pi_state->owner read (we don't hold wait_lock);
+		 * since we own the lock pi_state->owner == current is the
+		 * stable state, anything else needs more attention.
 		 */
 		if (q->pi_state->owner != current)
 			ret = fixup_pi_state_owner(uaddr, q, current);
 		goto out;
 	}
 
+	/*
+	 * If we didn't get the lock; check if anybody stole it from us. In
+	 * that case, we need to fix up the uval to point to them instead of
+	 * us, otherwise bad things happen. [10]
+	 *
+	 * Another speculative read; pi_state->owner == current is unstable
+	 * but needs our attention.
+	 */
+	if (q->pi_state->owner == current) {
+		ret = fixup_pi_state_owner(uaddr, q, NULL);
+		goto out;
+	}
+
 	/*
 	 * Paranoia check. If we did not take the lock, then we should not be
 	 * the owner of the rt_mutex.
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 6f3dba6e4e9e..65cc0cb984e6 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1290,6 +1290,19 @@ rt_mutex_slowlock(struct rt_mutex *lock, int state,
 	return ret;
 }
 
+static inline int __rt_mutex_slowtrylock(struct rt_mutex *lock)
+{
+	int ret = try_to_take_rt_mutex(lock, current, NULL);
+
+	/*
+	 * try_to_take_rt_mutex() sets the lock waiters bit
+	 * unconditionally. Clean this up.
+	 */
+	fixup_rt_mutex_waiters(lock);
+
+	return ret;
+}
+
 /*
  * Slow path try-lock function:
  */
@@ -1312,13 +1325,7 @@ static inline int rt_mutex_slowtrylock(struct rt_mutex *lock)
 	 */
 	raw_spin_lock_irqsave(&lock->wait_lock, flags);
 
-	ret = try_to_take_rt_mutex(lock, current, NULL);
-
-	/*
-	 * try_to_take_rt_mutex() sets the lock waiters bit
-	 * unconditionally. Clean this up.
-	 */
-	fixup_rt_mutex_waiters(lock);
+	ret = __rt_mutex_slowtrylock(lock);
 
 	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
@@ -1505,6 +1512,11 @@ int __sched rt_mutex_futex_trylock(struct rt_mutex *lock)
 	return rt_mutex_slowtrylock(lock);
 }
 
+int __sched __rt_mutex_futex_trylock(struct rt_mutex *lock)
+{
+	return __rt_mutex_slowtrylock(lock);
+}
+
 /**
  * rt_mutex_timed_lock - lock a rt_mutex interruptible
  *			the timeout structure is provided
diff --git a/kernel/locking/rtmutex_common.h b/kernel/locking/rtmutex_common.h
index 124e98ca0b17..68686b3ec3c1 100644
--- a/kernel/locking/rtmutex_common.h
+++ b/kernel/locking/rtmutex_common.h
@@ -148,6 +148,7 @@ extern bool rt_mutex_cleanup_proxy_lock(struct rt_mutex *lock,
 				 struct rt_mutex_waiter *waiter);
 
 extern int rt_mutex_futex_trylock(struct rt_mutex *l);
+extern int __rt_mutex_futex_trylock(struct rt_mutex *l);
 
 extern void rt_mutex_futex_unlock(struct rt_mutex *lock);
 extern bool __rt_mutex_futex_unlock(struct rt_mutex *lock,

Re: [GIT PULL] locking fixes

[Geert Uytterhoeven]

Hi Ingo, Peter,

On Wed, Jan 17, 2018 at 4:24 PM, Ingo Molnar <mingo@kernel.org> wrote:
> Please pull the latest locking-urgent-for-linus git tree from:
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-urgent-for-linus
>
>    # HEAD: fbe0e839d1e22d88810f3ee3e2f1479be4c0aa4a futex: Prevent overflow by strengthen input validation
>
> Two futex fixes: a input parameters robustness fix, and futex race fixes.

> Peter Zijlstra (1):
>       futex: Avoid violating the 10th rule of futex

> --- a/kernel/futex.c
> +++ b/kernel/futex.c

> @@ -2294,21 +2297,17 @@ static void unqueue_me_pi(struct futex_q *q)
>         spin_unlock(q->lock_ptr);
>  }
>
> -/*
> - * Fixup the pi_state owner with the new owner.
> - *
> - * Must be called with hash bucket lock held and mm->sem held for non
> - * private futexes.
> - */
>  static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
> -                               struct task_struct *newowner)
> +                               struct task_struct *argowner)
>  {
> -       u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
>         struct futex_pi_state *pi_state = q->pi_state;
>         u32 uval, uninitialized_var(curval), newval;
> -       struct task_struct *oldowner;
> +       struct task_struct *oldowner, *newowner;
> +       u32 newtid;

new tid is no longer initialized...

>         int ret;
>
> +       lockdep_assert_held(q->lock_ptr);
> +
>         raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
>
>         oldowner = pi_state->owner;
> @@ -2317,11 +2316,17 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>                 newtid |= FUTEX_OWNER_DIED;

... leading to a compiler warning with gcc 4.1.2:

    warning: ‘newtid’ is used uninitialized in this function

I guess newer compilers don't give the warning, as the result of the
assignment above is not used at all, and thus may be optimized away...

>
>         /*
> -        * We are here either because we stole the rtmutex from the
> -        * previous highest priority waiter or we are the highest priority
> -        * waiter but have failed to get the rtmutex the first time.
> +        * We are here because either:
> +        *
> +        *  - we stole the lock and pi_state->owner needs updating to reflect
> +        *    that (@argowner == current),
> +        *
> +        * or:
> +        *
> +        *  - someone stole our lock and we need to fix things to point to the
> +        *    new owner (@argowner == NULL).
>          *
> -        * We have to replace the newowner TID in the user space variable.
> +        * Either way, we have to replace the TID in the user space variable.
>          * This must be atomic as we have to preserve the owner died bit here.
>          *
>          * Note: We write the user space value _before_ changing the pi_state
> @@ -2334,6 +2339,42 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>          * in the PID check in lookup_pi_state.
>          */
>  retry:
> +       if (!argowner) {
> +               if (oldowner != current) {
> +                       /*
> +                        * We raced against a concurrent self; things are
> +                        * already fixed up. Nothing to do.
> +                        */
> +                       ret = 0;
> +                       goto out_unlock;
> +               }
> +
> +               if (__rt_mutex_futex_trylock(&pi_state->pi_mutex)) {
> +                       /* We got the lock after all, nothing to fix. */
> +                       ret = 0;
> +                       goto out_unlock;
> +               }
> +
> +               /*
> +                * Since we just failed the trylock; there must be an owner.
> +                */
> +               newowner = rt_mutex_owner(&pi_state->pi_mutex);
> +               BUG_ON(!newowner);
> +       } else {
> +               WARN_ON_ONCE(argowner != current);
> +               if (oldowner == current) {
> +                       /*
> +                        * We raced against a concurrent self; things are
> +                        * already fixed up. Nothing to do.
> +                        */
> +                       ret = 0;
> +                       goto out_unlock;
> +               }
> +               newowner = argowner;
> +       }
> +
> +       newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;

... since it is always overwritten here.

Is that intentional?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [GIT PULL] locking fixes

[Peter Zijlstra]

On Mon, Jan 22, 2018 at 10:43:36AM +0100, Geert Uytterhoeven wrote:
> >  static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
> > +                               struct task_struct *argowner)
> >  {
> >         struct futex_pi_state *pi_state = q->pi_state;
> >         u32 uval, uninitialized_var(curval), newval;
> > +       struct task_struct *oldowner, *newowner;
> > +       u32 newtid;
> 
> new tid is no longer initialized...
> 
> >         int ret;
> >
> > +       lockdep_assert_held(q->lock_ptr);
> > +
> >         raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
> >
> >         oldowner = pi_state->owner;
> > @@ -2317,11 +2316,17 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
> >                 newtid |= FUTEX_OWNER_DIED;
> 
> ... leading to a compiler warning with gcc 4.1.2:
> 
>     warning: ‘newtid’ is used uninitialized in this function
> 
> I guess newer compilers don't give the warning, as the result of the
> assignment above is not used at all, and thus may be optimized away...
> 
> >
> >         /*
> > +        * We are here because either:
> > +        *
> > +        *  - we stole the lock and pi_state->owner needs updating to reflect
> > +        *    that (@argowner == current),
> > +        *
> > +        * or:
> > +        *
> > +        *  - someone stole our lock and we need to fix things to point to the
> > +        *    new owner (@argowner == NULL).
> >          *
> > +        * Either way, we have to replace the TID in the user space variable.
> >          * This must be atomic as we have to preserve the owner died bit here.
> >          *
> >          * Note: We write the user space value _before_ changing the pi_state
> > @@ -2334,6 +2339,42 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
> >          * in the PID check in lookup_pi_state.
> >          */
> >  retry:
> > +       if (!argowner) {
> > +               if (oldowner != current) {
> > +                       /*
> > +                        * We raced against a concurrent self; things are
> > +                        * already fixed up. Nothing to do.
> > +                        */
> > +                       ret = 0;
> > +                       goto out_unlock;
> > +               }
> > +
> > +               if (__rt_mutex_futex_trylock(&pi_state->pi_mutex)) {
> > +                       /* We got the lock after all, nothing to fix. */
> > +                       ret = 0;
> > +                       goto out_unlock;
> > +               }
> > +
> > +               /*
> > +                * Since we just failed the trylock; there must be an owner.
> > +                */
> > +               newowner = rt_mutex_owner(&pi_state->pi_mutex);
> > +               BUG_ON(!newowner);
> > +       } else {
> > +               WARN_ON_ONCE(argowner != current);
> > +               if (oldowner == current) {
> > +                       /*
> > +                        * We raced against a concurrent self; things are
> > +                        * already fixed up. Nothing to do.
> > +                        */
> > +                       ret = 0;
> > +                       goto out_unlock;
> > +               }
> > +               newowner = argowner;
> > +       }
> > +
> > +       newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
> 
> ... since it is always overwritten here.
> 
> Is that intentional?

No, I think you actually spotted a bug there. We now can't set
OWNER_DIED anymore, which is bad.

I think the below fixes things, but let me go trawl through the various
futex test things, because I think I've seen a unit test for this
_somewhere_.

---
 kernel/futex.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 8c5424dd5924..7f719d110908 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2311,9 +2311,6 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
 
 	oldowner = pi_state->owner;
-	/* Owner died? */
-	if (!pi_state->owner)
-		newtid |= FUTEX_OWNER_DIED;
 
 	/*
 	 * We are here because either:
@@ -2374,6 +2371,9 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	}
 
 	newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
+	/* Owner died? */
+	if (!pi_state->owner)
+		newtid |= FUTEX_OWNER_DIED;
 
 	if (get_futex_value_locked(&uval, uaddr))
 		goto handle_fault;

[PATCH] futex: Fix OWNER_DEAD fixup

[Peter Zijlstra]

On Mon, Jan 22, 2018 at 11:39:47AM +0100, Peter Zijlstra wrote:
> No, I think you actually spotted a bug there. We now can't set
> OWNER_DIED anymore, which is bad.
> 
> I think the below fixes things, but let me go trawl through the various
> futex test things, because I think I've seen a unit test for this
> _somewhere_.

glibc has robustpi tests, but nothing there triggered this case.

---
Subject: futex: Fix OWNER_DEAD fixup
From: Peter Zijlstra <peterz@infradead.org>
Date: Mon, 22 Jan 2018 11:39:47 +0100

Both Geert and DaveJ reported that the recent futex commit:

  c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex")

introduced a problem with setting OWNER_DEAD. We set the bit on an
uninitialized variable and then entirely optimize it away as a
dead-store.

Move the setting of the bit to where it is more useful.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reported-by: Dave Jones <davej@codemonkey.org.uk>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Paul E. McKenney" <paulmck@us.ibm.com>
Fixes: c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/futex.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 8c5424dd5924..7f719d110908 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2311,9 +2311,6 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
 
 	oldowner = pi_state->owner;
-	/* Owner died? */
-	if (!pi_state->owner)
-		newtid |= FUTEX_OWNER_DIED;
 
 	/*
 	 * We are here because either:
@@ -2374,6 +2371,9 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	}
 
 	newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
+	/* Owner died? */
+	if (!pi_state->owner)
+		newtid |= FUTEX_OWNER_DIED;
 
 	if (get_futex_value_locked(&uval, uaddr))
 		goto handle_fault;

[tip:locking/urgent] futex: Fix OWNER_DEAD fixup

[tip-bot for Peter Zijlstra]

Commit-ID:  a97cb0e7b3f4c6297fd857055ae8e895f402f501
Gitweb:     https://git.kernel.org/tip/a97cb0e7b3f4c6297fd857055ae8e895f402f501
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Mon, 22 Jan 2018 11:39:47 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Jan 2018 09:58:18 +0100

futex: Fix OWNER_DEAD fixup

Both Geert and DaveJ reported that the recent futex commit:

  c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex")

introduced a problem with setting OWNER_DEAD. We set the bit on an
uninitialized variable and then entirely optimize it away as a
dead-store.

Move the setting of the bit to where it is more useful.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reported-by: Dave Jones <davej@codemonkey.org.uk>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@us.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex")
Link: http://lkml.kernel.org/r/20180122103947.GD2228@hirez.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/futex.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 8c5424d..7f719d1 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2311,9 +2311,6 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
 
 	oldowner = pi_state->owner;
-	/* Owner died? */
-	if (!pi_state->owner)
-		newtid |= FUTEX_OWNER_DIED;
 
 	/*
 	 * We are here because either:
@@ -2374,6 +2371,9 @@ retry:
 	}
 
 	newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
+	/* Owner died? */
+	if (!pi_state->owner)
+		newtid |= FUTEX_OWNER_DIED;
 
 	if (get_futex_value_locked(&uval, uaddr))
 		goto handle_fault;