Re: [GIT PULL] locking fixes

[Geert Uytterhoeven]

Hi Ingo, Peter,

On Wed, Jan 17, 2018 at 4:24 PM, Ingo Molnar <mingo@xxxxxxxxxx> 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@xxxxxxxxxxxxxx

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