Messages in this thread Patch in this message | | | Date | Mon, 22 Jan 2018 11:39:47 +0100 | From | Peter Zijlstra <> | Subject | Re: [GIT PULL] locking fixes |
| |
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;
| |