* Re: futex_wait_setup sleeping while atomic bug.
2014-09-11 21:52 ` Thomas Gleixner
@ 2014-09-11 22:16 ` Darren Hart
2014-09-11 23:53 ` Davidlohr Bueso
2014-09-12 20:10 ` [tip:locking/urgent] futex: Unlock hb-> lock in futex_wait_requeue_pi() error path tip-bot for Thomas Gleixner
2 siblings, 0 replies; 7+ messages in thread
From: Darren Hart @ 2014-09-11 22:16 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Dave Jones, Linux Kernel, Peter Zijlstra, Darren Hart
On Thu, Sep 11, 2014 at 11:52:02PM +0200, Thomas Gleixner wrote:
> On Thu, 11 Sep 2014, Dave Jones wrote:
>
> > Hit this overnight on Linus tree from yesterday.
> >
> > BUG: sleeping function called from invalid context at include/linux/freezer.h:56
> > in_atomic(): 1, irqs_disabled(): 0, pid: 31948, name: trinity-c121
> > Preemption disabled at:[<ffffffffb90f2512>] futex_wait_setup+0xb2/0x140
>
> Huch? So we are in a preempt disabled region in futex_wait_setup and
> we get interrupted and end up in the signal delivery path?
>
> I really love that futex stuff....
>
> But fortunately the preemption disabled hint made it into mainline so
> looking at the callsites of futex_wait_setup() makes it pretty clear
> where the shit hits the fan. Patch below.
>
> Thanks,
>
> tglx
>
> ------------------->
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 11 Sep 2014 23:44:35 +0200
> Subject: futex: Unlock hb->lock in futex_wait_requeue_pi() error path
>
> futex_wait_requeue_pi() calls futex_wait_setup(). If
> futex_wait_setup() succeeds it returns with hb->lock held and
> preemption disabled. Now the sanity check after this does:
>
> if (match_futex(&q.key, &key2)) {
> ret = -EINVAL;
> goto out_put_keys;
> }
>
> which releases the keys but does not release hb->lock. So we happily
> return to user space with hb->lock held and therefor preemption
> disabled.
>
> Unlock hb->lock before taking the exit route.
>
> Reported-by: Dave "Trinity" Jones <davej@redhat.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> diff --git a/kernel/futex.c b/kernel/futex.c
> index d3a9d946d0b7..815d7af2ffe8 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2592,6 +2592,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
> * shared futexes. We need to compare the keys:
> */
> if (match_futex(&q.key, &key2)) {
> + queue_unlock(hb);
> ret = -EINVAL;
> goto out_put_keys;
> }
I was hoping to move this before the key equivalence test so we could exit as
early as possible, but some idgit (that would be me) embedded the
get_futex_key() for uaddr into futex_wait_setup() to avoid so much duplication
of code... sigh.
Thomas's fix is the best immediate solution in my opinion.
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: futex_wait_setup sleeping while atomic bug.
2014-09-11 21:52 ` Thomas Gleixner
2014-09-11 22:16 ` Darren Hart
@ 2014-09-11 23:53 ` Davidlohr Bueso
2014-09-12 0:07 ` Darren Hart
2014-09-12 8:12 ` Thomas Gleixner
2014-09-12 20:10 ` [tip:locking/urgent] futex: Unlock hb-> lock in futex_wait_requeue_pi() error path tip-bot for Thomas Gleixner
2 siblings, 2 replies; 7+ messages in thread
From: Davidlohr Bueso @ 2014-09-11 23:53 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Dave Jones, Linux Kernel, Peter Zijlstra, Darren Hart
On Thu, 2014-09-11 at 23:52 +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 11 Sep 2014 23:44:35 +0200
> Subject: futex: Unlock hb->lock in futex_wait_requeue_pi() error path
That's the second time we are bitten by bugs in when requeing, now pi.
We need to reconsider some of our testing tools to stress these paths
better, imo.
> futex_wait_requeue_pi() calls futex_wait_setup(). If
> futex_wait_setup() succeeds it returns with hb->lock held and
> preemption disabled. Now the sanity check after this does:
>
> if (match_futex(&q.key, &key2)) {
> ret = -EINVAL;
> goto out_put_keys;
> }
>
> which releases the keys but does not release hb->lock. So we happily
> return to user space with hb->lock held and therefor preemption
> disabled.
>
> Unlock hb->lock before taking the exit route.
>
> Reported-by: Dave "Trinity" Jones <davej@redhat.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: futex_wait_setup sleeping while atomic bug.
2014-09-11 23:53 ` Davidlohr Bueso
@ 2014-09-12 0:07 ` Darren Hart
2014-09-12 8:12 ` Thomas Gleixner
1 sibling, 0 replies; 7+ messages in thread
From: Darren Hart @ 2014-09-12 0:07 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Thomas Gleixner, Dave Jones, Linux Kernel, Peter Zijlstra, Darren Hart
On Thu, Sep 11, 2014 at 04:53:38PM -0700, Davidlohr Bueso wrote:
> On Thu, 2014-09-11 at 23:52 +0200, Thomas Gleixner wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> > Date: Thu, 11 Sep 2014 23:44:35 +0200
> > Subject: futex: Unlock hb->lock in futex_wait_requeue_pi() error path
>
> That's the second time we are bitten by bugs in when requeing, now pi.
> We need to reconsider some of our testing tools to stress these paths
> better, imo.
We do, yes. Per the kselftest discussion at kernel summit, I agreed to move the
futextest testsuite into the kernel, function into kselftest and performance
into perf, then futextest can go away. From there we can look at how to improve
these tests.
Sadly, the best testing we seem to have is trinity - which does a fantastic job
at finding nasties.
If someone wanted to start having a look at migrating the futextest tests
over... I certainly wouldn't object to the help! ;-)
git://git.kernel.org/pub/scm/linux/kernel/git/dvhart/futextest.git
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: futex_wait_setup sleeping while atomic bug.
2014-09-11 23:53 ` Davidlohr Bueso
2014-09-12 0:07 ` Darren Hart
@ 2014-09-12 8:12 ` Thomas Gleixner
1 sibling, 0 replies; 7+ messages in thread
From: Thomas Gleixner @ 2014-09-12 8:12 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: Dave Jones, Linux Kernel, Peter Zijlstra, Darren Hart
On Thu, 11 Sep 2014, Davidlohr Bueso wrote:
> On Thu, 2014-09-11 at 23:52 +0200, Thomas Gleixner wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> > Date: Thu, 11 Sep 2014 23:44:35 +0200
> > Subject: futex: Unlock hb->lock in futex_wait_requeue_pi() error path
>
> That's the second time we are bitten by bugs in when requeing, now pi.
> We need to reconsider some of our testing tools to stress these paths
> better, imo.
Testing tools are nice. What we really need is more competent eyes
looking at that code ...
> > futex_wait_requeue_pi() calls futex_wait_setup(). If
> > futex_wait_setup() succeeds it returns with hb->lock held and
> > preemption disabled. Now the sanity check after this does:
> >
> > if (match_futex(&q.key, &key2)) {
> > ret = -EINVAL;
> > goto out_put_keys;
> > }
> >
> > which releases the keys but does not release hb->lock. So we happily
> > return to user space with hb->lock held and therefor preemption
> > disabled.
> >
> > Unlock hb->lock before taking the exit route.
> >
> > Reported-by: Dave "Trinity" Jones <davej@redhat.com>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip:locking/urgent] futex: Unlock hb-> lock in futex_wait_requeue_pi() error path
2014-09-11 21:52 ` Thomas Gleixner
2014-09-11 22:16 ` Darren Hart
2014-09-11 23:53 ` Davidlohr Bueso
@ 2014-09-12 20:10 ` tip-bot for Thomas Gleixner
2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Thomas Gleixner @ 2014-09-12 20:10 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, a.p.zijlstra, dvhart, davej, dave, tglx
Commit-ID: 13c42c2f43b19aab3195f2d357db00d1e885eaa8
Gitweb: http://git.kernel.org/tip/13c42c2f43b19aab3195f2d357db00d1e885eaa8
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 11 Sep 2014 23:44:35 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 12 Sep 2014 22:04:36 +0200
futex: Unlock hb->lock in futex_wait_requeue_pi() error path
futex_wait_requeue_pi() calls futex_wait_setup(). If
futex_wait_setup() succeeds it returns with hb->lock held and
preemption disabled. Now the sanity check after this does:
if (match_futex(&q.key, &key2)) {
ret = -EINVAL;
goto out_put_keys;
}
which releases the keys but does not release hb->lock.
So we happily return to user space with hb->lock held and therefor
preemption disabled.
Unlock hb->lock before taking the exit route.
Reported-by: Dave "Trinity" Jones <davej@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/alpine.DEB.2.10.1409112318500.4178@nanos
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
kernel/futex.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/futex.c b/kernel/futex.c
index d3a9d94..815d7af 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2592,6 +2592,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
* shared futexes. We need to compare the keys:
*/
if (match_futex(&q.key, &key2)) {
+ queue_unlock(hb);
ret = -EINVAL;
goto out_put_keys;
}
^ permalink raw reply related [flat|nested] 7+ messages in thread