From: Thomas Gleixner <tglx@linutronix.de>
To: Darren Hart <darren@dvhart.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@kernel.org>,
Davidlohr Bueso <davidlohr@hp.com>, Kees Cook <kees@outflux.net>,
wad@chromium.org
Subject: Re: [patch 1/5] futex: Make unlock_pi more robust
Date: Tue, 17 Jun 2014 00:15:55 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.10.1406170002050.5170@nanos> (raw)
In-Reply-To: <1402935528.15603.14.camel@rage>
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
next prev parent reply other threads:[~2014-06-16 22:16 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-11 20:45 [patch 0/5] futex: More robustness tweaks Thomas Gleixner
2014-06-11 20:45 ` [patch 1/5] futex: Make unlock_pi more robust Thomas Gleixner
2014-06-16 16:18 ` Darren Hart
2014-06-16 22:15 ` Thomas Gleixner [this message]
2014-06-16 22:28 ` Thomas Gleixner
2014-06-16 22:49 ` Darren Hart
2014-06-16 22:39 ` Darren Hart
2014-06-21 20:33 ` [tip:locking/core] " tip-bot for Thomas Gleixner
2014-06-11 20:45 ` [patch 3/5] futex: Split out the waiter check from lookup_pi_state() Thomas Gleixner
2014-06-16 18:12 ` Darren Hart
2014-06-21 20:33 ` [tip:locking/core] " tip-bot for Thomas Gleixner
2014-06-11 20:45 ` [patch 2/5] futex: Use futex_top_waiter() in lookup_pi_state() Thomas Gleixner
2014-06-16 16:51 ` Darren Hart
2014-06-21 20:33 ` [tip:locking/core] " tip-bot for Thomas Gleixner
2014-06-11 20:45 ` [patch 4/5] futex: Split out the first waiter attachment from lookup_pi_state() Thomas Gleixner
2014-06-16 18:19 ` Darren Hart
2014-06-21 20:33 ` [tip:locking/core] " tip-bot for Thomas Gleixner
2014-06-11 20:45 ` [patch 5/5] futex: Simplify futex_lock_pi_atomic() and make it more robust Thomas Gleixner
2014-06-13 5:46 ` Darren Hart
2014-06-13 8:34 ` Thomas Gleixner
2014-06-13 9:36 ` Thomas Gleixner
2014-06-13 9:44 ` [patch V2 " Thomas Gleixner
2014-06-13 20:51 ` Davidlohr Bueso
2014-06-16 20:36 ` Darren Hart
2014-06-17 7:20 ` Thomas Gleixner
2014-06-21 20:34 ` [tip:locking/core] " tip-bot for Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.10.1406170002050.5170@nanos \
--to=tglx@linutronix.de \
--cc=darren@dvhart.com \
--cc=davidlohr@hp.com \
--cc=kees@outflux.net \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=wad@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).