All of lore.kernel.org
 help / color / mirror / Atom feed
* futex_wait_setup sleeping while atomic bug.
@ 2014-09-11 15:10 Dave Jones
  2014-09-11 21:52 ` Thomas Gleixner
  0 siblings, 1 reply; 7+ messages in thread
From: Dave Jones @ 2014-09-11 15:10 UTC (permalink / raw)
  To: Linux Kernel; +Cc: Peter Zijlstra, Thomas Gleixner

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

CPU: 0 PID: 31948 Comm: trinity-c121 Not tainted 3.17.0-rc4+ #52
 ffffffffb9a52f2a 00000000d3f40479 ffff88008a32fd90 ffffffffb9771ba4
 0000000000000000 ffff88008a32fdb8 ffffffffb909f02c ffff88008a32ff58
 ffff88008a32ff58 0000000000000010 ffff88008a32fe30 ffffffffb90887bc
Call Trace:
 [<ffffffffb9771ba4>] dump_stack+0x4e/0x7a
 [<ffffffffb909f02c>] __might_sleep+0x11c/0x1b0
 [<ffffffffb90887bc>] get_signal+0x6c/0x720
 [<ffffffffb9002577>] do_signal+0x37/0x6f0
 [<ffffffffb9126ddb>] ? __acct_update_integrals+0x8b/0x120
 [<ffffffffb912728c>] ? acct_account_cputime+0x1c/0x20
 [<ffffffffb90ab1ab>] ? account_user_time+0x9b/0xb0
 [<ffffffffb9002c8c>] do_notify_resume+0x5c/0xa0
 [<ffffffffb977be23>] retint_signal+0x46/0x83
BUG: scheduling while atomic: trinity-c121/31948/0x00000002
Preemption disabled at:[<ffffffffb90f2512>] futex_wait_setup+0xb2/0x140

CPU: 0 PID: 31948 Comm: trinity-c121 Not tainted 3.17.0-rc4+ #52
 ffff88024d013740 00000000d3f40479 ffff88008a32fec8 ffffffffb9771ba4
 ffff8802403eee00 ffff88008a32fee0 ffffffffb909e725 0000000000000000
 ffff88008a32ff60 ffffffffb9774db7 000000000000a000 ffff8802403eee00
Call Trace:
 [<ffffffffb9771ba4>] dump_stack+0x4e/0x7a
 [<ffffffffb909e725>] __schedule_bug+0x65/0xc0
 [<ffffffffb9774db7>] __schedule+0x6a7/0xa70
 [<ffffffffb97756ee>] schedule_user+0x2e/0x90
 [<ffffffffb977bdc1>] retint_careful+0x12/0x2e
note: trinity-c121[31948] exited with preempt_count 1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: futex_wait_setup sleeping while atomic bug.
  2014-09-11 15:10 futex_wait_setup sleeping while atomic bug Dave Jones
@ 2014-09-11 21:52 ` Thomas Gleixner
  2014-09-11 22:16   ` Darren Hart
                     ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Thomas Gleixner @ 2014-09-11 21:52 UTC (permalink / raw)
  To: Dave Jones; +Cc: Linux Kernel, Peter Zijlstra, Darren Hart

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;
 	}

^ 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 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	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-09-12 20:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-11 15:10 futex_wait_setup sleeping while atomic bug Dave Jones
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.