lkml.org 
[lkml]   [2014]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 1/5] futex: Make unlock_pi more robust
On Mon, 16 Jun 2014, Darren Hart wrote:
> On Wed, 2014-06-11 at 20:45 +0000, Thomas Gleixner wrote:
> > static int futex_unlock_pi(u32 __user *uaddr, unsigned int flags)
> > @@ -2417,57 +2401,47 @@ retry:
> > return -EPERM;
> >
> > ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &key,
> > VERIFY_WRITE);
> > - if (unlikely(ret != 0))
> > - goto out;
> > + if (ret)
> > + return ret;
>
> Looks like you're also trying to move away from a single exit point to
> multiple exit points. I prefer the single exit (which you've probably
> noticed :-), but it's a subjective thing, and so long as we are not
> duplicating cleanup logic, I guess it's fine either way. This change was
> not mentioned in the commit message though.

I really did not think about mentioning that :)

> > + * Check waiters first. We do not trust user space values at
> > + * all and we at least want to know if user space fiddled
> > + * with the futex value instead of blindly unlocking.
> > + */
> > + match = futex_top_waiter(hb, &key);
> > + if (match) {
> > + ret = wake_futex_pi(uaddr, uval, match);
> > /*
> > - * The atomic access to the futex value
> > - * generated a pagefault, so retry the
> > - * user-access and the wakeup:
> > + * The atomic access to the futex value generated a
> > + * pagefault, so retry the user-access and the wakeup:
> > */
> > if (ret == -EFAULT)
> > goto pi_faulted;
> > goto out_unlock;
> > }
> > +
> > /*
> > - * No waiters - kernel unlocks the futex:
> > + * We have no kernel internal state, i.e. no waiters in the
> > + * kernel. Waiters which are about to queue themself are stuck
>
> themselves
>
> > + * on hb->lock. So we can safely ignore them. We do neither
> > + * preserve the WAITERS bit not the OWNER_DIED one. We are the
>
> We preserve neither the WAITERS bit nor the OWNER_DIED bit.
> (the above use of "do" and "not" is incorrect and could easily be
> misinterpreted).
>
> > + * owner.
>
> In wake_futex_pi we verify ownership by matching pi_state->owner ==
> current, but here the only test is the TID value, which is set by
> userspace - which we don't trust...
>
> I'm trying to determine if it matters in this case... if there are no
> waiters, is the pi_state still around? If so, it does indeed matter, and
> we should be verifying.

Erm. The whole point of this patch is to do:

- Find existing state first and handle it.

- If no state exists and TID == current, take it

- Otherwise create state

This all happens under hb->lock. So how should something create new
state after we looked up existing state?

> > */
> > - ret = unlock_futex_pi(uaddr, uval);
> > - if (ret == -EFAULT)
> > + if (cmpxchg_futex_value_locked(&curval, uaddr, uval, 0))
> > goto pi_faulted;
> >
>
> This refactoring seems like it would be best done as a prequel patch so
> as not to confuse cleanup with functional change. At least that is what
> you and others have beaten into me over the years ;-)

Well, yes and no. I'll hapilly discuss that without after clarifying
the issue below.

> > + /*
> > + * If uval has changed, let user space handle it.
> > + */
> > + ret = (curval == uval) ? 0 : -EAGAIN;
> > +
> > out_unlock:
> > spin_unlock(&hb->lock);
> > put_futex_key(&key);
> > -
> > -out:
> > return ret;
> >
>
> By dropping this you won't return ret, but rather fall through into
> pi_faulted... which certainly isn't what you wanted.

By dropping the now unused "out" label I'm not longer returning ret?

The resulting code is:

out_unlock:
spin_unlock(&hb->lock);
put_futex_key(&key);
return ret;

If you can explain me how that "return ret" falls through to
pi_faulted magically, then I'm definitely agreeing with you on this:

> The need for better test coverage is very evident now :-)

-ENOTENOUGHSLEEP or -ENOTENOUGHCOFFEE or -ENOGLASSES perhaps?

I'm omitting some other politicially incorrect speculations for now.

Thanks,

tglx


\
 
 \ /
  Last update: 2014-06-17 00:41    [W:0.070 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site