Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Mon, 22 Jan 2018 10:43:36 +0100 | Subject | Re: [GIT PULL] locking fixes |
| |
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
| |