futex_wait_setup sleeping while atomic bug.

futex_wait_setup sleeping while atomic bug.

[Dave Jones]

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

Re: futex_wait_setup sleeping while atomic bug.

[Thomas Gleixner]

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

Re: futex_wait_setup sleeping while atomic bug.

[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

Re: futex_wait_setup sleeping while atomic bug.

[Davidlohr Bueso]

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>

Re: futex_wait_setup sleeping while atomic bug.

[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

Re: futex_wait_setup sleeping while atomic bug.

[Thomas Gleixner]

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

[tip:locking/urgent] futex: Unlock hb-> lock in futex_wait_requeue_pi() error path

[tip-bot for Thomas Gleixner]

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