mm_alloc()'ed structure leak

mm_alloc()'ed structure leak

[Catalin Marinas]

Hi,

I've noticed on recent kernels (currently 2.6.29-rc3) a memory leak
reported by kmemleak for an mm_struct allocated in mm_alloc(). If that's
a valid leak, it is a pretty serious one.

Basically bash forks and executes a command like "host kernel.org" which
finishes normally but the corresponding mm_struct isn't freed (I get
this consistently every time I run the above command):

unreferenced object 0xcfed4070 (size 368):
  comm "bash", pid 1674, jiffies 421592
  backtrace:
    [<c0082bd4>] kmemleak_alloc+0x140/0x2b0
    [<c007ff2c>] kmem_cache_alloc+0xd0/0x100
    [<c0036980>] mm_alloc+0x14/0x44
    [<c008a99c>] bprm_mm_init+0xc/0x13c
    [<c008ab70>] do_execve+0xa4/0x218
    [<c002718c>] sys_execve+0x34/0x54
    [<c0023e80>] ret_fast_syscall+0x0/0x28

I can't figure out why this structure isn't freed, so any help is
welcomed before I start bisecting. The platform is an ARM one but the
code in question is probably generic.

Thanks.

-- 
Catalin

Re: mm_alloc()'ed structure leak

[Catalin Marinas]

On Mon, 2009-02-09 at 12:18 +0000, Catalin Marinas wrote:
> I've noticed on recent kernels (currently 2.6.29-rc3) a memory leak
> reported by kmemleak for an mm_struct allocated in mm_alloc().

FYI, I tried 2.6.29-rc4 and the leak is still present.

-- 
Catalin

Re: mm_alloc()'ed structure leak

[Catalin Marinas]

On Mon, 2009-02-09 at 12:18 +0000, Catalin Marinas wrote:
> Basically bash forks and executes a command like "host kernel.org" which
> finishes normally but the corresponding mm_struct isn't freed (I get
> this consistently every time I run the above command):
> 
> unreferenced object 0xcfed4070 (size 368):
>   comm "bash", pid 1674, jiffies 421592
>   backtrace:
>     [<c0082bd4>] kmemleak_alloc+0x140/0x2b0
>     [<c007ff2c>] kmem_cache_alloc+0xd0/0x100
>     [<c0036980>] mm_alloc+0x14/0x44
>     [<c008a99c>] bprm_mm_init+0xc/0x13c
>     [<c008ab70>] do_execve+0xa4/0x218
>     [<c002718c>] sys_execve+0x34/0x54
>     [<c0023e80>] ret_fast_syscall+0x0/0x28

Dumping the object in question:

mm_struct.mm_users = 0
mm_struct.mm_count = 1

It looks like the mm_count didn't get to 0 hence no structure freeing
via mmdrop().

The leak disappears if I revert commit 38d47c1b7075 - "futex: rely on
get_user_pages() for shared futexes". Peter, any idea?

-- 
Catalin

Re: mm_alloc()'ed structure leak

[Peter Zijlstra]

On Mon, 2009-02-09 at 14:44 +0000, Catalin Marinas wrote:
> On Mon, 2009-02-09 at 12:18 +0000, Catalin Marinas wrote:
> > Basically bash forks and executes a command like "host kernel.org" which
> > finishes normally but the corresponding mm_struct isn't freed (I get
> > this consistently every time I run the above command):
> > 
> > unreferenced object 0xcfed4070 (size 368):
> >   comm "bash", pid 1674, jiffies 421592
> >   backtrace:
> >     [<c0082bd4>] kmemleak_alloc+0x140/0x2b0
> >     [<c007ff2c>] kmem_cache_alloc+0xd0/0x100
> >     [<c0036980>] mm_alloc+0x14/0x44
> >     [<c008a99c>] bprm_mm_init+0xc/0x13c
> >     [<c008ab70>] do_execve+0xa4/0x218
> >     [<c002718c>] sys_execve+0x34/0x54
> >     [<c0023e80>] ret_fast_syscall+0x0/0x28
> 
> Dumping the object in question:
> 
> mm_struct.mm_users = 0
> mm_struct.mm_count = 1
> 
> It looks like the mm_count didn't get to 0 hence no structure freeing
> via mmdrop().
> 
> The leak disappears if I revert commit 38d47c1b7075 - "futex: rely on
> get_user_pages() for shared futexes". Peter, any idea?

Looks like the futex key references go wrong somewhere, I'll go look at
it.

Re: mm_alloc()'ed structure leak

[Peter Zijlstra]

On Mon, 2009-02-09 at 16:12 +0100, Peter Zijlstra wrote:
> On Mon, 2009-02-09 at 14:44 +0000, Catalin Marinas wrote:
> > On Mon, 2009-02-09 at 12:18 +0000, Catalin Marinas wrote:
> > > Basically bash forks and executes a command like "host kernel.org" which
> > > finishes normally but the corresponding mm_struct isn't freed (I get
> > > this consistently every time I run the above command):
> > > 
> > > unreferenced object 0xcfed4070 (size 368):
> > >   comm "bash", pid 1674, jiffies 421592
> > >   backtrace:
> > >     [<c0082bd4>] kmemleak_alloc+0x140/0x2b0
> > >     [<c007ff2c>] kmem_cache_alloc+0xd0/0x100
> > >     [<c0036980>] mm_alloc+0x14/0x44
> > >     [<c008a99c>] bprm_mm_init+0xc/0x13c
> > >     [<c008ab70>] do_execve+0xa4/0x218
> > >     [<c002718c>] sys_execve+0x34/0x54
> > >     [<c0023e80>] ret_fast_syscall+0x0/0x28
> > 
> > Dumping the object in question:
> > 
> > mm_struct.mm_users = 0
> > mm_struct.mm_count = 1
> > 
> > It looks like the mm_count didn't get to 0 hence no structure freeing
> > via mmdrop().
> > 
> > The leak disappears if I revert commit 38d47c1b7075 - "futex: rely on
> > get_user_pages() for shared futexes". Peter, any idea?
> 
> Looks like the futex key references go wrong somewhere, I'll go look at
> it.

How does this work for you?

Not-signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/futex.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index f89d373..4aecf77 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -55,6 +55,7 @@
 #include <linux/magic.h>
 #include <linux/pid.h>
 #include <linux/nsproxy.h>
+#include <linux/ftrace.h>
 
 #include <asm/futex.h>
 
@@ -156,9 +157,15 @@ static void get_futex_key_refs(union futex_key *key)
 
 	switch (key->both.offset & (FUT_OFF_INODE|FUT_OFF_MMSHARED)) {
 	case FUT_OFF_INODE:
+		ftrace_printk("(%lx:%x) inode++: %p\n", 
+				key->both.word, key->both.offset & ~3,
+				&key->shared.inode);
 		atomic_inc(&key->shared.inode->i_count);
 		break;
 	case FUT_OFF_MMSHARED:
+		ftrace_printk("(%lx:%x) mm++: %p\n", 
+				key->both.word, key->both.offset & ~3,
+				&key->shared.inode);
 		atomic_inc(&key->private.mm->mm_count);
 		break;
 	}
@@ -178,9 +185,15 @@ static void drop_futex_key_refs(union futex_key *key)
 
 	switch (key->both.offset & (FUT_OFF_INODE|FUT_OFF_MMSHARED)) {
 	case FUT_OFF_INODE:
+		ftrace_printk("(%lx:%x) inode--: %p\n", 
+				key->both.word, key->both.offset & ~3,
+				&key->shared.inode);
 		iput(key->shared.inode);
 		break;
 	case FUT_OFF_MMSHARED:
+		ftrace_printk("(%lx:%x) mm--: %p\n", 
+				key->both.word, key->both.offset & ~3,
+				&key->shared.inode);
 		mmdrop(key->private.mm);
 		break;
 	}
@@ -1284,17 +1297,20 @@ retry:
 	 */
 
 	/* If we were woken (and unqueued), we succeeded, whatever. */
+	ret = 0;
 	if (!unqueue_me(&q))
-		return 0;
+		goto out_put_key;
+	ret = -ETIMEDOUT;
 	if (rem)
-		return -ETIMEDOUT;
+		goto out_put_key;
 
 	/*
 	 * We expect signal_pending(current), but another thread may
 	 * have handled it for us already.
 	 */
+	ret = -ERESTARTSYS;
 	if (!abs_time)
-		return -ERESTARTSYS;
+		goto out_put_key;
 	else {
 		struct restart_block *restart;
 		restart = &current_thread_info()->restart_block;
@@ -1309,11 +1325,13 @@ retry:
 			restart->futex.flags |= FLAGS_SHARED;
 		if (clockrt)
 			restart->futex.flags |= FLAGS_CLOCKRT;
-		return -ERESTART_RESTARTBLOCK;
+		ret = -ERESTARTSYS;
+		goto out_put_key;
 	}
 
 out_unlock_put_key:
 	queue_unlock(&q, hb);
+out_put_key:
 	put_futex_key(fshared, &q.key);
 
 out:

Re: mm_alloc()'ed structure leak

[Peter Zijlstra]

On Mon, 2009-02-09 at 17:45 +0100, Peter Zijlstra wrote:
> -               return -ERESTART_RESTARTBLOCK;
> +               ret = -ERESTARTSYS;

noted and fixed.

Re: mm_alloc()'ed structure leak

[Catalin Marinas]

On Mon, 2009-02-09 at 17:47 +0100, Peter Zijlstra wrote:
> On Mon, 2009-02-09 at 17:45 +0100, Peter Zijlstra wrote:
> > -               return -ERESTART_RESTARTBLOCK;
> > +               ret = -ERESTARTSYS;
> 
> noted and fixed.

I can confirm that with your patch, the leak is no longer reported.
Thanks.

-- 
Catalin

[PATCH] futex: fix reference leak

[Peter Zijlstra]

Venki, does this perchance solve your issue with my futex patch?

Darren, could you please have a look at this before Ingo pushes it out
to Linus?

---
Catalin noticed that (38d47c1b7075: futex: rely on
get_user_pages() for shared futexes) caused an mm_struct leak.

Some tracing with the function graph tracer quickly pointed out that
futex_wait() has exit paths with unbalanced reference counts.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/futex.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index f89d373..7b76c8e 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1284,18 +1284,23 @@ retry:
 	 */
 
 	/* If we were woken (and unqueued), we succeeded, whatever. */
-	if (!unqueue_me(&q))
-		return 0;
-	if (rem)
-		return -ETIMEDOUT;
+	if (!unqueue_me(&q)) {
+		ret = 0;
+		goto out_put_key;
+	}
+	if (rem) {
+		ret = -ETIMEDOUT;
+		goto out_put_key;
+	}
 
 	/*
 	 * We expect signal_pending(current), but another thread may
 	 * have handled it for us already.
 	 */
-	if (!abs_time)
-		return -ERESTARTSYS;
-	else {
+	if (!abs_time) {
+		ret = -ERESTARTSYS;
+		goto out_put_key;
+	} else {
 		struct restart_block *restart;
 		restart = &current_thread_info()->restart_block;
 		restart->fn = futex_wait_restart;
@@ -1309,11 +1314,13 @@ retry:
 			restart->futex.flags |= FLAGS_SHARED;
 		if (clockrt)
 			restart->futex.flags |= FLAGS_CLOCKRT;
-		return -ERESTART_RESTARTBLOCK;
+		ret = -RESTART_RESTARTBLOCK;
+		goto out_put_key;
 	}
 
 out_unlock_put_key:
 	queue_unlock(&q, hb);
+out_put_key:
 	put_futex_key(fshared, &q.key);
 
 out:

Re: [PATCH] futex: fix reference leak

[Pallipadi, Venkatesh]

Yes. This patch fixes the problem on my system. With git, without the
patch I see the same oops as before on reboot. With the patch, system
reboots cleanly.

A minor typo in the patch
+ ret = -RESTART_RESTARTBLOCK;
should be
+ ret = -ERESTART_RESTARTBLOCK;

Thanks,
Venki

On Mon, 2009-02-09 at 10:26 -0800, Peter Zijlstra wrote:
> Venki, does this perchance solve your issue with my futex patch?
> 
> Darren, could you please have a look at this before Ingo pushes it out
> to Linus?
> 
> ---
> Catalin noticed that (38d47c1b7075: futex: rely on
> get_user_pages() for shared futexes) caused an mm_struct leak.
> 
> Some tracing with the function graph tracer quickly pointed out that
> futex_wait() has exit paths with unbalanced reference counts.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  kernel/futex.c |   23 +++++++++++++++--------
>  1 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index f89d373..7b76c8e 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1284,18 +1284,23 @@ retry:
>  	 */
>  
>  	/* If we were woken (and unqueued), we succeeded, whatever. */
> -	if (!unqueue_me(&q))
> -		return 0;
> -	if (rem)
> -		return -ETIMEDOUT;
> +	if (!unqueue_me(&q)) {
> +		ret = 0;
> +		goto out_put_key;
> +	}
> +	if (rem) {
> +		ret = -ETIMEDOUT;
> +		goto out_put_key;
> +	}
>  
>  	/*
>  	 * We expect signal_pending(current), but another thread may
>  	 * have handled it for us already.
>  	 */
> -	if (!abs_time)
> -		return -ERESTARTSYS;
> -	else {
> +	if (!abs_time) {
> +		ret = -ERESTARTSYS;
> +		goto out_put_key;
> +	} else {
>  		struct restart_block *restart;
>  		restart = &current_thread_info()->restart_block;
>  		restart->fn = futex_wait_restart;
> @@ -1309,11 +1314,13 @@ retry:
>  			restart->futex.flags |= FLAGS_SHARED;
>  		if (clockrt)
>  			restart->futex.flags |= FLAGS_CLOCKRT;
> -		return -ERESTART_RESTARTBLOCK;
> +		ret = -RESTART_RESTARTBLOCK;
> +		goto out_put_key;
>  	}
>  
>  out_unlock_put_key:
>  	queue_unlock(&q, hb);
> +out_put_key:
>  	put_futex_key(fshared, &q.key);
>  
>  out:
> 
> 

[PATCH -v2] futex: fix reference leak

[Peter Zijlstra]

On Mon, 2009-02-09 at 10:53 -0800, Pallipadi, Venkatesh wrote:
> Yes. This patch fixes the problem on my system. With git, without the
> patch I see the same oops as before on reboot. With the patch, system
> reboots cleanly.
> 
> A minor typo in the patch
> + ret = -RESTART_RESTARTBLOCK;
> should be
> + ret = -ERESTART_RESTARTBLOCK;

Gah, so much for my copy-paste skillz ;-)

Thanks!

---
Catalin noticed that (38d47c1b7075: futex: rely on
get_user_pages() for shared futexes) caused an mm_struct leak.

Some tracing with the function graph tracer quickly pointed out that
futex_wait() has exit paths with unbalanced reference counts.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Tested-by: "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com>
---
 kernel/futex.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index f89d373..ff06c76 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1284,18 +1284,23 @@ retry:
 	 */
 
 	/* If we were woken (and unqueued), we succeeded, whatever. */
-	if (!unqueue_me(&q))
-		return 0;
-	if (rem)
-		return -ETIMEDOUT;
+	if (!unqueue_me(&q)) {
+		ret = 0;
+		goto out_put_key;
+	}
+	if (rem) {
+		ret = -ETIMEDOUT;
+		goto out_put_key;
+	}
 
 	/*
 	 * We expect signal_pending(current), but another thread may
 	 * have handled it for us already.
 	 */
-	if (!abs_time)
-		return -ERESTARTSYS;
-	else {
+	if (!abs_time) {
+		ret = -ERESTARTSYS;
+		goto out_put_key;
+	} else {
 		struct restart_block *restart;
 		restart = &current_thread_info()->restart_block;
 		restart->fn = futex_wait_restart;
@@ -1309,11 +1314,13 @@ retry:
 			restart->futex.flags |= FLAGS_SHARED;
 		if (clockrt)
 			restart->futex.flags |= FLAGS_CLOCKRT;
-		return -ERESTART_RESTARTBLOCK;
+		ret = -ERESTART_RESTARTBLOCK;
+		goto out_put_key;
 	}
 
 out_unlock_put_key:
 	queue_unlock(&q, hb);
+out_put_key:
 	put_futex_key(fshared, &q.key);
 
 out:

Re: [PATCH -v2] futex: fix reference leak

[Darren Hart]

Peter Zijlstra wrote:
> On Mon, 2009-02-09 at 10:53 -0800, Pallipadi, Venkatesh wrote:
>> Yes. This patch fixes the problem on my system. With git, without the
>> patch I see the same oops as before on reboot. With the patch, system
>> reboots cleanly.
>>


OK, I remember why I didn't do this in the set I sent to Ingo for -tip.  The unqueue_me()
 call drops the key ref for us.  Eventually I think we should take/release references and
 locks in the same function as much as possible, but for now I left them where they were.
I suspect this patch is actually dropping one extra key ref than it should, but because
Caralin has been testing on linux.git and not -tip, the get/put patches I submitted there
aren't included, so this patch compensates for one of those missing patches :-)

Caralin, can you try applying the following patches from linux-tip without this patch and see if your problem still exists?

42d35d48ce7cefb9429880af19d1c329d1554e7a - futex: make futex_(get|put)_key() calls symmetric
90621c40cc4ab7b0a414311ce37e7cc7173403b6 - futex: catch certain assymetric (get|put)_futex_key calls

I haven't tried applying these to linux-2.6.git myself, so they may not apply cleanly.

Thanks,

Darren Hart

>> A minor typo in the patch
>> + ret = -RESTART_RESTARTBLOCK;
>> should be
>> + ret = -ERESTART_RESTARTBLOCK;
> 
> Gah, so much for my copy-paste skillz ;-)
> 
> Thanks!
> 
> ---
> Catalin noticed that (38d47c1b7075: futex: rely on
> get_user_pages() for shared futexes) caused an mm_struct leak.
> 
> Some tracing with the function graph tracer quickly pointed out that
> futex_wait() has exit paths with unbalanced reference counts.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Tested-by: "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com>
> ---
>  kernel/futex.c |   23 +++++++++++++++--------
>  1 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index f89d373..ff06c76 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1284,18 +1284,23 @@ retry:
>  	 */
> 
>  	/* If we were woken (and unqueued), we succeeded, whatever. */
> -	if (!unqueue_me(&q))
> -		return 0;
> -	if (rem)
> -		return -ETIMEDOUT;
> +	if (!unqueue_me(&q)) {
> +		ret = 0;
> +		goto out_put_key;
> +	}
> +	if (rem) {
> +		ret = -ETIMEDOUT;
> +		goto out_put_key;
> +	}
> 
>  	/*
>  	 * We expect signal_pending(current), but another thread may
>  	 * have handled it for us already.
>  	 */
> -	if (!abs_time)
> -		return -ERESTARTSYS;
> -	else {
> +	if (!abs_time) {
> +		ret = -ERESTARTSYS;
> +		goto out_put_key;
> +	} else {
>  		struct restart_block *restart;
>  		restart = &current_thread_info()->restart_block;
>  		restart->fn = futex_wait_restart;
> @@ -1309,11 +1314,13 @@ retry:
>  			restart->futex.flags |= FLAGS_SHARED;
>  		if (clockrt)
>  			restart->futex.flags |= FLAGS_CLOCKRT;
> -		return -ERESTART_RESTARTBLOCK;
> +		ret = -ERESTART_RESTARTBLOCK;
> +		goto out_put_key;
>  	}
> 
>  out_unlock_put_key:
>  	queue_unlock(&q, hb);
> +out_put_key:
>  	put_futex_key(fshared, &q.key);
> 
>  out:
> 
> 


-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team

Re: [PATCH -v2] futex: fix reference leak

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> +	if (!unqueue_me(&q)) {
> +		ret = 0;
> +		goto out_put_key;
> +	}
> +	if (rem) {
> +		ret = -ETIMEDOUT;
> +		goto out_put_key;
> +	}

hm, we generally prefer to write such things as:

	ret = 0;
	if (!unqueue_me(&q))
		goto out_put_key;

	ret = -ETIMEDOUT;
	if (rem)
		goto out_put_key;

Also, while at it, the flow of control looks weird in other places too:

        if (!abs_time)
                return -ERESTARTSYS;
        else {
                struct restart_block *restart;
                restart = &current_thread_info()->restart_block;
                restart->fn = futex_wait_restart;
                restart->futex.uaddr = (u32 *)uaddr;

Shouldnt it be something like:

        if (!abs_time)
                return -ERESTARTSYS;

        restart = &current_thread_info()->restart_block;
        restart->fn = futex_wait_restart;
        restart->futex.uaddr = (u32 *)uaddr;

(with the variable definition moving up to local variables.)

and this:

> +	if (!abs_time) {
> +		ret = -ERESTARTSYS;
> +		goto out_put_key;
> +	} else {
>  		struct restart_block *restart;
>  		restart = &current_thread_info()->restart_block;
>  		restart->fn = futex_wait_restart;
> @@ -1309,11 +1314,13 @@ retry:
>  			restart->futex.flags |= FLAGS_SHARED;
>  		if (clockrt)
>  			restart->futex.flags |= FLAGS_CLOCKRT;
> -		return -ERESTART_RESTARTBLOCK;
> +		ret = -ERESTART_RESTARTBLOCK;
> +		goto out_put_key;
>  	}
>  
>  out_unlock_put_key:
>  	queue_unlock(&q, hb);
> +out_put_key:
>  	put_futex_key(fshared, &q.key);

and this looks weird too, we jump-goto over the queue_unlock() in essence.

A proper flow would be to rename the error labels as err_unlock_*[etc], move
them out of line, let them jump back to the normal labels - and let the
tail section of the code above fall through.

	Ingo

[PATCH -v3] futex: fix reference leak

[Peter Zijlstra]

So you prefer this version?

---
Subject: futex: fix reference leak
From: Peter Zijlstra <peterz@infradead.org>

Catalin noticed that (38d47c1b7075: futex: rely on get_user_pages() for
shared futexes) caused an mm_struct leak.

Some tracing with the function graph tracer quickly pointed out that
futex_wait() has exit paths with unbalanced reference counts.

This regression was discovered by kmemleak.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Tested-by: "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com>
Tested-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
---
 kernel/futex.c |   51 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index f89d373..a4b3cac 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1165,6 +1165,7 @@ static int futex_wait(u32 __user *uaddr, int fshared,
 		      u32 val, ktime_t *abs_time, u32 bitset, int clockrt)
 {
 	struct task_struct *curr = current;
+	struct restart_block *restart;
 	DECLARE_WAITQUEUE(wait, curr);
 	struct futex_hash_bucket *hb;
 	struct futex_q q;
@@ -1216,7 +1217,7 @@ retry:
 
 		if (!ret)
 			goto retry;
-		return ret;
+		goto out;
 	}
 	ret = -EWOULDBLOCK;
 	if (uval != val)
@@ -1284,40 +1285,44 @@ retry:
 	 */
 
 	/* If we were woken (and unqueued), we succeeded, whatever. */
+	ret = 0;
 	if (!unqueue_me(&q))
-		return 0;
+		goto out_put_key;
+	ret = -ETIMEDOUT;
 	if (rem)
-		return -ETIMEDOUT;
+		goto out_put_key;
 
 	/*
 	 * We expect signal_pending(current), but another thread may
 	 * have handled it for us already.
 	 */
+	ret = -ERESTARTSYS;
 	if (!abs_time)
-		return -ERESTARTSYS;
-	else {
-		struct restart_block *restart;
-		restart = &current_thread_info()->restart_block;
-		restart->fn = futex_wait_restart;
-		restart->futex.uaddr = (u32 *)uaddr;
-		restart->futex.val = val;
-		restart->futex.time = abs_time->tv64;
-		restart->futex.bitset = bitset;
-		restart->futex.flags = 0;
-
-		if (fshared)
-			restart->futex.flags |= FLAGS_SHARED;
-		if (clockrt)
-			restart->futex.flags |= FLAGS_CLOCKRT;
-		return -ERESTART_RESTARTBLOCK;
-	}
+		goto out_put_key;
 
-out_unlock_put_key:
-	queue_unlock(&q, hb);
-	put_futex_key(fshared, &q.key);
+	restart = &current_thread_info()->restart_block;
+	restart->fn = futex_wait_restart;
+	restart->futex.uaddr = (u32 *)uaddr;
+	restart->futex.val = val;
+	restart->futex.time = abs_time->tv64;
+	restart->futex.bitset = bitset;
+	restart->futex.flags = 0;
 
+	if (fshared)
+		restart->futex.flags |= FLAGS_SHARED;
+	if (clockrt)
+		restart->futex.flags |= FLAGS_CLOCKRT;
+
+	ret = -ERESTART_RESTARTBLOCK;
+
+out_put_key:
+	put_futex_key(fshared, &q.key);
 out:
 	return ret;
+
+out_unlock_put_key:
+	queue_unlock(&q, hb);
+	goto out_put_key;
 }
 
 

Re: [PATCH -v3] futex: fix reference leak

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> So you prefer this version?

yes, with s/out_unlock_put_key/err_unlock_put_key. That's the
only error path that goes via the tail of the function, correct?

	Ingo

Re: [PATCH -v3] futex: fix reference leak

[Peter Zijlstra]

On Wed, 2009-02-11 at 16:49 +0100, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > So you prefer this version?
> 
> yes, with s/out_unlock_put_key/err_unlock_put_key. That's the
> only error path that goes via the tail of the function, correct?

Right, but I think one can argue that most of the out* jumps are errors.

The only non-error return is the !unqueue_me() case.

Also, since there's only one user of out_unlock_put_key, we might also
do this:

---
Index: linux-2.6/kernel/futex.c
===================================================================
--- linux-2.6.orig/kernel/futex.c
+++ linux-2.6/kernel/futex.c
@@ -1220,8 +1220,10 @@ retry:
 		goto out;
 	}
 	ret = -EWOULDBLOCK;
-	if (uval != val)
-		goto out_unlock_put_key;
+	if (uval != val) {
+		queue_unlock(&q, hb);
+		goto out_put_key;
+	}
 
 	/* Only actually queue if *uaddr contained val.  */
 	queue_me(&q, hb);
@@ -1319,10 +1321,6 @@ out_put_key:
 	put_futex_key(fshared, &q.key);
 out:
 	return ret;
-
-out_unlock_put_key:
-	queue_unlock(&q, hb);
-	goto out_put_key;
 }
 
 

Re: [PATCH -v3] futex: fix reference leak

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, 2009-02-11 at 16:49 +0100, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > So you prefer this version?
> > 
> > yes, with s/out_unlock_put_key/err_unlock_put_key. That's the
> > only error path that goes via the tail of the function, correct?
> 
> Right, but I think one can argue that most of the out* jumps are errors.
> 
> The only non-error return is the !unqueue_me() case.
> 
> Also, since there's only one user of out_unlock_put_key, we might also
> do this:

yeah. Also mark it via unlikely().

	Ingo

[PATCH -v4] futex: fix reference leak

[Peter Zijlstra]

Subject: futex: fix reference leak
From: Peter Zijlstra <peterz@infradead.org>

Catalin noticed that (38d47c1b7075: futex: rely on get_user_pages() for
shared futexes) caused an mm_struct leak.

Some tracing with the function graph tracer quickly pointed out that
futex_wait() has exit paths with unbalanced reference counts.

This regression was discovered by kmemleak.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Tested-by: "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com>
Tested-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
---
 kernel/futex.c |   53 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index f89d373..438701a 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1165,6 +1165,7 @@ static int futex_wait(u32 __user *uaddr, int fshared,
 		      u32 val, ktime_t *abs_time, u32 bitset, int clockrt)
 {
 	struct task_struct *curr = current;
+	struct restart_block *restart;
 	DECLARE_WAITQUEUE(wait, curr);
 	struct futex_hash_bucket *hb;
 	struct futex_q q;
@@ -1216,11 +1217,13 @@ retry:
 
 		if (!ret)
 			goto retry;
-		return ret;
+		goto out;
 	}
 	ret = -EWOULDBLOCK;
-	if (uval != val)
-		goto out_unlock_put_key;
+	if (unlikely(uval != val)) {
+		queue_unlock(&q, hb);
+		goto out_put_key;
+	}
 
 	/* Only actually queue if *uaddr contained val.  */
 	queue_me(&q, hb);
@@ -1284,38 +1287,38 @@ retry:
 	 */
 
 	/* If we were woken (and unqueued), we succeeded, whatever. */
+	ret = 0;
 	if (!unqueue_me(&q))
-		return 0;
+		goto out_put_key;
+	ret = -ETIMEDOUT;
 	if (rem)
-		return -ETIMEDOUT;
+		goto out_put_key;
 
 	/*
 	 * We expect signal_pending(current), but another thread may
 	 * have handled it for us already.
 	 */
+	ret = -ERESTARTSYS;
 	if (!abs_time)
-		return -ERESTARTSYS;
-	else {
-		struct restart_block *restart;
-		restart = &current_thread_info()->restart_block;
-		restart->fn = futex_wait_restart;
-		restart->futex.uaddr = (u32 *)uaddr;
-		restart->futex.val = val;
-		restart->futex.time = abs_time->tv64;
-		restart->futex.bitset = bitset;
-		restart->futex.flags = 0;
-
-		if (fshared)
-			restart->futex.flags |= FLAGS_SHARED;
-		if (clockrt)
-			restart->futex.flags |= FLAGS_CLOCKRT;
-		return -ERESTART_RESTARTBLOCK;
-	}
+		goto out_put_key;
 
-out_unlock_put_key:
-	queue_unlock(&q, hb);
-	put_futex_key(fshared, &q.key);
+	restart = &current_thread_info()->restart_block;
+	restart->fn = futex_wait_restart;
+	restart->futex.uaddr = (u32 *)uaddr;
+	restart->futex.val = val;
+	restart->futex.time = abs_time->tv64;
+	restart->futex.bitset = bitset;
+	restart->futex.flags = 0;
+
+	if (fshared)
+		restart->futex.flags |= FLAGS_SHARED;
+	if (clockrt)
+		restart->futex.flags |= FLAGS_CLOCKRT;
 
+	ret = -ERESTART_RESTARTBLOCK;
+
+out_put_key:
+	put_futex_key(fshared, &q.key);
 out:
 	return ret;
 }

Re: [PATCH -v4] futex: fix reference leak

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> Subject: futex: fix reference leak
> From: Peter Zijlstra <peterz@infradead.org>
> 
> Catalin noticed that (38d47c1b7075: futex: rely on get_user_pages() for
> shared futexes) caused an mm_struct leak.
> 
> Some tracing with the function graph tracer quickly pointed out that
> futex_wait() has exit paths with unbalanced reference counts.
> 
> This regression was discovered by kmemleak.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Tested-by: "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com>
> Tested-by: Catalin Marinas <catalin.marinas@arm.com>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  kernel/futex.c |   53 ++++++++++++++++++++++++++++-------------------------
>  1 files changed, 28 insertions(+), 25 deletions(-)

Applied to tip:core/urgent, thanks guys!

	Ingo