* mm_alloc()'ed structure leak
@ 2009-02-09 12:18 Catalin Marinas
2009-02-09 13:12 ` Catalin Marinas
2009-02-09 14:44 ` Catalin Marinas
0 siblings, 2 replies; 18+ messages in thread
From: Catalin Marinas @ 2009-02-09 12:18 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrew Morton
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: mm_alloc()'ed structure leak
2009-02-09 12:18 mm_alloc()'ed structure leak Catalin Marinas
@ 2009-02-09 13:12 ` Catalin Marinas
2009-02-09 14:44 ` Catalin Marinas
1 sibling, 0 replies; 18+ messages in thread
From: Catalin Marinas @ 2009-02-09 13:12 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrew Morton
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: mm_alloc()'ed structure leak
2009-02-09 12:18 mm_alloc()'ed structure leak Catalin Marinas
2009-02-09 13:12 ` Catalin Marinas
@ 2009-02-09 14:44 ` Catalin Marinas
2009-02-09 15:12 ` Peter Zijlstra
1 sibling, 1 reply; 18+ messages in thread
From: Catalin Marinas @ 2009-02-09 14:44 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrew Morton, Peter Zijlstra
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: mm_alloc()'ed structure leak
2009-02-09 14:44 ` Catalin Marinas
@ 2009-02-09 15:12 ` Peter Zijlstra
2009-02-09 16:45 ` Peter Zijlstra
0 siblings, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-09 15:12 UTC (permalink / raw)
To: Catalin Marinas; +Cc: linux-kernel, Andrew Morton
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: mm_alloc()'ed structure leak
2009-02-09 15:12 ` Peter Zijlstra
@ 2009-02-09 16:45 ` Peter Zijlstra
2009-02-09 16:47 ` Peter Zijlstra
0 siblings, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-09 16:45 UTC (permalink / raw)
To: Catalin Marinas; +Cc: linux-kernel, Andrew Morton, Darren Hart
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 = ¤t_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:
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: mm_alloc()'ed structure leak
2009-02-09 16:45 ` Peter Zijlstra
@ 2009-02-09 16:47 ` Peter Zijlstra
2009-02-09 17:28 ` Catalin Marinas
0 siblings, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-09 16:47 UTC (permalink / raw)
To: Catalin Marinas; +Cc: linux-kernel, Andrew Morton, Darren Hart
On Mon, 2009-02-09 at 17:45 +0100, Peter Zijlstra wrote:
> - return -ERESTART_RESTARTBLOCK;
> + ret = -ERESTARTSYS;
noted and fixed.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: mm_alloc()'ed structure leak
2009-02-09 16:47 ` Peter Zijlstra
@ 2009-02-09 17:28 ` Catalin Marinas
2009-02-09 18:26 ` [PATCH] futex: fix reference leak Peter Zijlstra
0 siblings, 1 reply; 18+ messages in thread
From: Catalin Marinas @ 2009-02-09 17:28 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-kernel, Andrew Morton, Darren Hart
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] futex: fix reference leak
2009-02-09 17:28 ` Catalin Marinas
@ 2009-02-09 18:26 ` Peter Zijlstra
2009-02-09 18:53 ` Pallipadi, Venkatesh
0 siblings, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-09 18:26 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-kernel, Andrew Morton, Darren Hart, Thomas Gleixner,
Ingo Molnar, Pallipadi, Venkatesh
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 = ¤t_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:
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] futex: fix reference leak
2009-02-09 18:26 ` [PATCH] futex: fix reference leak Peter Zijlstra
@ 2009-02-09 18:53 ` Pallipadi, Venkatesh
2009-02-09 18:57 ` [PATCH -v2] " Peter Zijlstra
0 siblings, 1 reply; 18+ messages in thread
From: Pallipadi, Venkatesh @ 2009-02-09 18:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Catalin Marinas, linux-kernel, Andrew Morton, Darren Hart,
Thomas Gleixner, Ingo Molnar
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 = ¤t_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:
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH -v2] futex: fix reference leak
2009-02-09 18:53 ` Pallipadi, Venkatesh
@ 2009-02-09 18:57 ` Peter Zijlstra
2009-02-09 20:49 ` Darren Hart
2009-02-11 12:28 ` Ingo Molnar
0 siblings, 2 replies; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-09 18:57 UTC (permalink / raw)
To: Pallipadi, Venkatesh
Cc: Catalin Marinas, linux-kernel, Andrew Morton, Darren Hart,
Thomas Gleixner, Ingo Molnar
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 = ¤t_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:
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH -v2] futex: fix reference leak
2009-02-09 18:57 ` [PATCH -v2] " Peter Zijlstra
@ 2009-02-09 20:49 ` Darren Hart
2009-02-11 12:28 ` Ingo Molnar
1 sibling, 0 replies; 18+ messages in thread
From: Darren Hart @ 2009-02-09 20:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Thomas Gleixner, Ingo Molnar
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 = ¤t_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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH -v2] futex: fix reference leak
2009-02-09 18:57 ` [PATCH -v2] " Peter Zijlstra
2009-02-09 20:49 ` Darren Hart
@ 2009-02-11 12:28 ` Ingo Molnar
2009-02-11 15:36 ` [PATCH -v3] " Peter Zijlstra
1 sibling, 1 reply; 18+ messages in thread
From: Ingo Molnar @ 2009-02-11 12:28 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Darren Hart, Thomas Gleixner
* 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 = ¤t_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 = ¤t_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 = ¤t_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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH -v3] futex: fix reference leak
2009-02-11 12:28 ` Ingo Molnar
@ 2009-02-11 15:36 ` Peter Zijlstra
2009-02-11 15:49 ` Ingo Molnar
0 siblings, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-11 15:36 UTC (permalink / raw)
To: Ingo Molnar
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Darren Hart, Thomas Gleixner
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 = ¤t_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 = ¤t_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;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH -v3] futex: fix reference leak
2009-02-11 15:36 ` [PATCH -v3] " Peter Zijlstra
@ 2009-02-11 15:49 ` Ingo Molnar
2009-02-11 15:56 ` Peter Zijlstra
0 siblings, 1 reply; 18+ messages in thread
From: Ingo Molnar @ 2009-02-11 15:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Darren Hart, Thomas Gleixner
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH -v3] futex: fix reference leak
2009-02-11 15:49 ` Ingo Molnar
@ 2009-02-11 15:56 ` Peter Zijlstra
2009-02-11 16:26 ` Ingo Molnar
0 siblings, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-11 15:56 UTC (permalink / raw)
To: Ingo Molnar
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Darren Hart, Thomas Gleixner
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;
}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH -v3] futex: fix reference leak
2009-02-11 15:56 ` Peter Zijlstra
@ 2009-02-11 16:26 ` Ingo Molnar
2009-02-11 17:10 ` [PATCH -v4] " Peter Zijlstra
0 siblings, 1 reply; 18+ messages in thread
From: Ingo Molnar @ 2009-02-11 16:26 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Darren Hart, Thomas Gleixner
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH -v4] futex: fix reference leak
2009-02-11 16:26 ` Ingo Molnar
@ 2009-02-11 17:10 ` Peter Zijlstra
2009-02-11 17:24 ` Ingo Molnar
0 siblings, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2009-02-11 17:10 UTC (permalink / raw)
To: Ingo Molnar
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Darren Hart, Thomas Gleixner
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 = ¤t_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 = ¤t_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;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH -v4] futex: fix reference leak
2009-02-11 17:10 ` [PATCH -v4] " Peter Zijlstra
@ 2009-02-11 17:24 ` Ingo Molnar
0 siblings, 0 replies; 18+ messages in thread
From: Ingo Molnar @ 2009-02-11 17:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Pallipadi, Venkatesh, Catalin Marinas, linux-kernel,
Andrew Morton, Darren Hart, Thomas Gleixner
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2009-02-11 17:25 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-02-09 12:18 mm_alloc()'ed structure leak Catalin Marinas
2009-02-09 13:12 ` Catalin Marinas
2009-02-09 14:44 ` Catalin Marinas
2009-02-09 15:12 ` Peter Zijlstra
2009-02-09 16:45 ` Peter Zijlstra
2009-02-09 16:47 ` Peter Zijlstra
2009-02-09 17:28 ` Catalin Marinas
2009-02-09 18:26 ` [PATCH] futex: fix reference leak Peter Zijlstra
2009-02-09 18:53 ` Pallipadi, Venkatesh
2009-02-09 18:57 ` [PATCH -v2] " Peter Zijlstra
2009-02-09 20:49 ` Darren Hart
2009-02-11 12:28 ` Ingo Molnar
2009-02-11 15:36 ` [PATCH -v3] " Peter Zijlstra
2009-02-11 15:49 ` Ingo Molnar
2009-02-11 15:56 ` Peter Zijlstra
2009-02-11 16:26 ` Ingo Molnar
2009-02-11 17:10 ` [PATCH -v4] " Peter Zijlstra
2009-02-11 17:24 ` Ingo Molnar
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.