Re: [PATCH] futex: avoid race between requeue and wake

[Linus Torvalds]

On Tue, Apr 8, 2014 at 9:20 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> However, one exception to this is "requeue_futex()". Which is in fact
> the test-case that Jan points to. There, when we move a futex from one
> hash bucket to another, we do the increment inside the spinlock.
>
> So I think the change is correct, although the commit message might
> need a bit of improvement.

Hmm. And let's think about that powerpc race, where "spin_is_locked()"
can be false when somebody is waiting to get the lock..

We're good on the *normal* paths, exactly because we do that
hb_waiters_inc() before getting the lock (which also forces a memory
barrier), so the spin_is_locked() test is unnecessary and we have no
race.

But now Jan's patch is re-introducing spin_is_locked(), so let's think
about what that means for the requeue_futex() case. Especially since
the two accesses in

    spin_is_locked(&hb->lock) || atomic_read(&hb->waiters)

are not ordered any way wrt each other (they are both just
unconstrained reads). How does this all work? This is making me
nervous.

Moving the hb_waiters_inc() to before taking the spinlock (inside
double_lock_hb() or whatever) would make me more comfortable wrt this
whole situation, if only because now the rules would be the same for
that requeue case as for adding the futex in the first place.

But maybe somebody can just show that Jan's simpler patch is
sufficient for that case for some fundamental reason that I
overlooked. In which case I would just want a comment.

It's a day for misquoting Star Wars: "Help me, Davidlohr Bueso. You
are my only hope"

Although comments from others would be good too. PeterZ? He wasn't
originally cc'd, but was involved with the original ordering
discussion. Adding..

                    Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/