Hi Ingo, Peter,
On Wed, Jan 17, 2018 at 4:24 PM, Ingo Molnar <[email protected]> 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 -- [email protected]
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
