* [2.6.31 and later] "struct pid" leak.
@ 2010-03-27 12:21 Tetsuo Handa
2010-03-30 15:31 ` Catalin Marinas
0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2010-03-27 12:21 UTC (permalink / raw)
To: linux-kernel
I got below report with 2.6.33.1 .
unreferenced object 0xde144600 (size 64):
comm "init", pid 1, jiffies 4294678101 (age 291.508s)
hex dump (first 32 bytes):
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 04 76 ae de d1 76 43 c0 d6 08 00 00 .....v...vC.....
backtrace:
[<c0481704>] create_object+0x121/0x1ef
[<c05f546b>] kmemleak_alloc+0x25/0x42
[<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
[<c047e36e>] kmem_cache_alloc+0x42/0x68
[<c0437701>] alloc_pid+0x19/0x288
[<c0428acc>] copy_process+0x95a/0xdac
[<c04290d8>] do_fork+0x129/0x261
[<c0407de5>] sys_clone+0x1f/0x24
[<c040292d>] ptregs_clone+0x15/0x28
[<ffffffff>] 0xffffffff
unreferenced object 0xdfa96a40 (size 64):
comm "login", pid 2259, jiffies 4294719437 (age 250.179s)
hex dump (first 32 bytes):
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 60 39 ae de d1 76 43 c0 bb 09 00 00 ....`9...vC.....
backtrace:
[<c0481704>] create_object+0x121/0x1ef
[<c05f546b>] kmemleak_alloc+0x25/0x42
[<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
[<c047e36e>] kmem_cache_alloc+0x42/0x68
[<c0437701>] alloc_pid+0x19/0x288
[<c0428acc>] copy_process+0x95a/0xdac
[<c04290d8>] do_fork+0x129/0x261
[<c0407de5>] sys_clone+0x1f/0x24
[<c040292d>] ptregs_clone+0x15/0x28
[<ffffffff>] 0xffffffff
This report is generated whenever /sbin/mingetty (invoked by SysVinit's
/sbin/init in accordance with /etc/inittab) is terminated.
Steps to reproduce.
(1) Go to console.
(2) Try to login. /sbin/mingetty will invoke /bin/login . Terminate /bin/login
process by either "successful login and logout" or "login failure".
/sbin/mingetty process will be respawned by /sbin/init after /bin/login
terminates.
(3) Login as root.
(4) Run "echo scan > /sys/kernel/debug/kmemleak".
(5) Wait for a while.
(6) Run "cat /sys/kernel/debug/kmemleak".
I can find this report with 2.6.31.11 (by manually increasing
CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE to 10000).
unreferenced object 0xdeee2200 (size 64):
comm "init", pid 1, jiffies 4294789063
backtrace:
[<c0487114>] create_object+0x135/0x202
[<c0487206>] kmemleak_alloc+0x25/0x49
[<c048433b>] kmemleak_alloc_recursive+0x1c/0x22
[<c0484386>] kmem_cache_alloc+0x45/0xb2
[<c043826d>] alloc_pid+0x19/0x28c
[<c04286e4>] copy_process+0x929/0xe62
[<c04291cb>] do_fork+0x124/0x295
[<c040177b>] sys_clone+0x24/0x2b
[<c0402a44>] sysenter_do_call+0x12/0x22
[<ffffffff>] 0xffffffff
I can't use "git bisect" to find the origin because kmemleak is available for
2.6.31 and later.
/sbin/init calls syscalls such as setsid() which will manipulate "struct pid"
between fork() and execve(). But I haven't succeeded to create test program.
Regards.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [2.6.31 and later] "struct pid" leak.
2010-03-27 12:21 [2.6.31 and later] "struct pid" leak Tetsuo Handa
@ 2010-03-30 15:31 ` Catalin Marinas
2010-03-31 22:17 ` Andrew Morton
0 siblings, 1 reply; 18+ messages in thread
From: Catalin Marinas @ 2010-03-30 15:31 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-kernel
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> I got below report with 2.6.33.1 .
>
> unreferenced object 0xde144600 (size 64):
> comm "init", pid 1, jiffies 4294678101 (age 291.508s)
> hex dump (first 32 bytes):
> 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 04 76 ae de d1 76 43 c0 d6 08 00 00 .....v...vC.....
> backtrace:
> [<c0481704>] create_object+0x121/0x1ef
> [<c05f546b>] kmemleak_alloc+0x25/0x42
> [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> [<c047e36e>] kmem_cache_alloc+0x42/0x68
> [<c0437701>] alloc_pid+0x19/0x288
> [<c0428acc>] copy_process+0x95a/0xdac
> [<c04290d8>] do_fork+0x129/0x261
> [<c0407de5>] sys_clone+0x1f/0x24
> [<c040292d>] ptregs_clone+0x15/0x28
> [<ffffffff>] 0xffffffff
> unreferenced object 0xdfa96a40 (size 64):
> comm "login", pid 2259, jiffies 4294719437 (age 250.179s)
> hex dump (first 32 bytes):
> 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 60 39 ae de d1 76 43 c0 bb 09 00 00 ....`9...vC.....
> backtrace:
> [<c0481704>] create_object+0x121/0x1ef
> [<c05f546b>] kmemleak_alloc+0x25/0x42
> [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> [<c047e36e>] kmem_cache_alloc+0x42/0x68
> [<c0437701>] alloc_pid+0x19/0x288
> [<c0428acc>] copy_process+0x95a/0xdac
> [<c04290d8>] do_fork+0x129/0x261
> [<c0407de5>] sys_clone+0x1f/0x24
> [<c040292d>] ptregs_clone+0x15/0x28
> [<ffffffff>] 0xffffffff
I reported similar leaks last year -
http://lkml.org/lkml/2009/7/8/422. There is some analysis in the thread
above of the reference counting but I couldn't figure out where it goes
wrong. It looks to me like there isn't any reference to a struct pid
block but its reference count is 2.
There is a bugzilla entry as well -
https://bugzilla.kernel.org/show_bug.cgi?id=13868
--
Catalin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [2.6.31 and later] "struct pid" leak.
2010-03-30 15:31 ` Catalin Marinas
@ 2010-03-31 22:17 ` Andrew Morton
2010-04-01 16:52 ` Oleg Nesterov
2010-04-02 16:04 ` [PATCH 0/1] tty: release_one_tty() forgets to put pids Oleg Nesterov
0 siblings, 2 replies; 18+ messages in thread
From: Andrew Morton @ 2010-03-31 22:17 UTC (permalink / raw)
To: Catalin Marinas
Cc: Tetsuo Handa, linux-kernel, Oleg Nesterov, Serge Hallyn,
Eric W. Biederman, Sukadev Bhattiprolu
On Tue, 30 Mar 2010 16:31:13 +0100
Catalin Marinas <catalin.marinas@arm.com> wrote:
> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > I got below report with 2.6.33.1 .
> >
> > unreferenced object 0xde144600 (size 64):
> > comm "init", pid 1, jiffies 4294678101 (age 291.508s)
> > hex dump (first 32 bytes):
> > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00 00 00 00 04 76 ae de d1 76 43 c0 d6 08 00 00 .....v...vC.....
> > backtrace:
> > [<c0481704>] create_object+0x121/0x1ef
> > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > [<c0437701>] alloc_pid+0x19/0x288
> > [<c0428acc>] copy_process+0x95a/0xdac
> > [<c04290d8>] do_fork+0x129/0x261
> > [<c0407de5>] sys_clone+0x1f/0x24
> > [<c040292d>] ptregs_clone+0x15/0x28
> > [<ffffffff>] 0xffffffff
> > unreferenced object 0xdfa96a40 (size 64):
> > comm "login", pid 2259, jiffies 4294719437 (age 250.179s)
> > hex dump (first 32 bytes):
> > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00 00 00 00 60 39 ae de d1 76 43 c0 bb 09 00 00 ....`9...vC.....
> > backtrace:
> > [<c0481704>] create_object+0x121/0x1ef
> > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > [<c0437701>] alloc_pid+0x19/0x288
> > [<c0428acc>] copy_process+0x95a/0xdac
> > [<c04290d8>] do_fork+0x129/0x261
> > [<c0407de5>] sys_clone+0x1f/0x24
> > [<c040292d>] ptregs_clone+0x15/0x28
> > [<ffffffff>] 0xffffffff
>
> I reported similar leaks last year -
> http://lkml.org/lkml/2009/7/8/422. There is some analysis in the thread
> above of the reference counting but I couldn't figure out where it goes
> wrong. It looks to me like there isn't any reference to a struct pid
> block but its reference count is 2.
>
> There is a bugzilla entry as well -
> https://bugzilla.kernel.org/show_bug.cgi?id=13868
>
Let's bug some people by cc'ing them ;)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [2.6.31 and later] "struct pid" leak.
2010-03-31 22:17 ` Andrew Morton
@ 2010-04-01 16:52 ` Oleg Nesterov
2010-04-01 17:21 ` Serge E. Hallyn
2010-04-02 16:04 ` [PATCH 0/1] tty: release_one_tty() forgets to put pids Oleg Nesterov
1 sibling, 1 reply; 18+ messages in thread
From: Oleg Nesterov @ 2010-04-01 16:52 UTC (permalink / raw)
To: Andrew Morton
Cc: Catalin Marinas, Tetsuo Handa, linux-kernel, Serge Hallyn,
Eric W. Biederman, Sukadev Bhattiprolu
On 03/31, Andrew Morton wrote:
>
> On Tue, 30 Mar 2010 16:31:13 +0100
> Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > I got below report with 2.6.33.1 .
> > >
> > > unreferenced object 0xde144600 (size 64):
> > > comm "init", pid 1, jiffies 4294678101 (age 291.508s)
> > > hex dump (first 32 bytes):
> > > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > > 00 00 00 00 04 76 ae de d1 76 43 c0 d6 08 00 00 .....v...vC.....
> > > backtrace:
> > > [<c0481704>] create_object+0x121/0x1ef
> > > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > > [<c0437701>] alloc_pid+0x19/0x288
> > > [<c0428acc>] copy_process+0x95a/0xdac
> > > [<c04290d8>] do_fork+0x129/0x261
> > > [<c0407de5>] sys_clone+0x1f/0x24
> > > [<c040292d>] ptregs_clone+0x15/0x28
> > > [<ffffffff>] 0xffffffff
> > > unreferenced object 0xdfa96a40 (size 64):
> > > comm "login", pid 2259, jiffies 4294719437 (age 250.179s)
> > > hex dump (first 32 bytes):
> > > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > > 00 00 00 00 60 39 ae de d1 76 43 c0 bb 09 00 00 ....`9...vC.....
> > > backtrace:
> > > [<c0481704>] create_object+0x121/0x1ef
> > > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > > [<c0437701>] alloc_pid+0x19/0x288
> > > [<c0428acc>] copy_process+0x95a/0xdac
> > > [<c04290d8>] do_fork+0x129/0x261
> > > [<c0407de5>] sys_clone+0x1f/0x24
> > > [<c040292d>] ptregs_clone+0x15/0x28
> > > [<ffffffff>] 0xffffffff
> >
> > I reported similar leaks last year -
> > http://lkml.org/lkml/2009/7/8/422. There is some analysis in the thread
> > above of the reference counting but I couldn't figure out where it goes
> > wrong. It looks to me like there isn't any reference to a struct pid
> > block but its reference count is 2.
> >
> > There is a bugzilla entry as well -
> > https://bugzilla.kernel.org/show_bug.cgi?id=13868
> >
>
> Let's bug some people by cc'ing them ;)
Oh. It is hardly possibly to find the unbalanced get_pid() via grep.
IIRC, I sent the debugging patch which tracks get/put pid, but I can't
recall if anybody tried this patch. Hmm, and I can't find that patch
or the previous discussion in my maildir...
Catalin, Tetsuo, any chance you can remind me if this patch was used?
Oleg.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [2.6.31 and later] "struct pid" leak.
2010-04-01 16:52 ` Oleg Nesterov
@ 2010-04-01 17:21 ` Serge E. Hallyn
2010-04-01 17:33 ` Serge E. Hallyn
2010-04-02 15:29 ` Oleg Nesterov
0 siblings, 2 replies; 18+ messages in thread
From: Serge E. Hallyn @ 2010-04-01 17:21 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Catalin Marinas, Tetsuo Handa, linux-kernel,
Eric W. Biederman, Sukadev Bhattiprolu
Quoting Oleg Nesterov (oleg@redhat.com):
> On 03/31, Andrew Morton wrote:
> >
> > On Tue, 30 Mar 2010 16:31:13 +0100
> > Catalin Marinas <catalin.marinas@arm.com> wrote:
> >
> > > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > > I got below report with 2.6.33.1 .
> > > >
> > > > unreferenced object 0xde144600 (size 64):
> > > > comm "init", pid 1, jiffies 4294678101 (age 291.508s)
> > > > hex dump (first 32 bytes):
> > > > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > > > 00 00 00 00 04 76 ae de d1 76 43 c0 d6 08 00 00 .....v...vC.....
> > > > backtrace:
> > > > [<c0481704>] create_object+0x121/0x1ef
> > > > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > > > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > > > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > > > [<c0437701>] alloc_pid+0x19/0x288
> > > > [<c0428acc>] copy_process+0x95a/0xdac
> > > > [<c04290d8>] do_fork+0x129/0x261
> > > > [<c0407de5>] sys_clone+0x1f/0x24
> > > > [<c040292d>] ptregs_clone+0x15/0x28
> > > > [<ffffffff>] 0xffffffff
> > > > unreferenced object 0xdfa96a40 (size 64):
> > > > comm "login", pid 2259, jiffies 4294719437 (age 250.179s)
> > > > hex dump (first 32 bytes):
> > > > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > > > 00 00 00 00 60 39 ae de d1 76 43 c0 bb 09 00 00 ....`9...vC.....
> > > > backtrace:
> > > > [<c0481704>] create_object+0x121/0x1ef
> > > > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > > > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > > > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > > > [<c0437701>] alloc_pid+0x19/0x288
> > > > [<c0428acc>] copy_process+0x95a/0xdac
> > > > [<c04290d8>] do_fork+0x129/0x261
> > > > [<c0407de5>] sys_clone+0x1f/0x24
> > > > [<c040292d>] ptregs_clone+0x15/0x28
> > > > [<ffffffff>] 0xffffffff
> > >
> > > I reported similar leaks last year -
> > > http://lkml.org/lkml/2009/7/8/422. There is some analysis in the thread
> > > above of the reference counting but I couldn't figure out where it goes
> > > wrong. It looks to me like there isn't any reference to a struct pid
> > > block but its reference count is 2.
> > >
> > > There is a bugzilla entry as well -
> > > https://bugzilla.kernel.org/show_bug.cgi?id=13868
> > >
> >
> > Let's bug some people by cc'ing them ;)
>
> Oh. It is hardly possibly to find the unbalanced get_pid() via grep.
>
> IIRC, I sent the debugging patch which tracks get/put pid, but I can't
> recall if anybody tried this patch. Hmm, and I can't find that patch
> or the previous discussion in my maildir...
>
> Catalin, Tetsuo, any chance you can remind me if this patch was used?
>
> Oleg.
[ probably sounding like a moron, but... ]
Looking through vt_ioctl.c, I get the feeling that the ttys will
hang onto vc->vt_pid until either getting a SAK or until someone
new logs in. I don't see where logging out will cause a reset_vc().
So when the logged in task logs out, does vt_pid keep a ref to the
pid which now no longer exists?
Came to mind bc I notice that every trace you've sent has included
/bin/login or X...
-serge
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [2.6.31 and later] "struct pid" leak.
2010-04-01 17:21 ` Serge E. Hallyn
@ 2010-04-01 17:33 ` Serge E. Hallyn
2010-04-02 15:29 ` Oleg Nesterov
1 sibling, 0 replies; 18+ messages in thread
From: Serge E. Hallyn @ 2010-04-01 17:33 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Catalin Marinas, Tetsuo Handa, linux-kernel,
Eric W. Biederman, Sukadev Bhattiprolu
Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 03/31, Andrew Morton wrote:
> > >
> > > On Tue, 30 Mar 2010 16:31:13 +0100
> > > Catalin Marinas <catalin.marinas@arm.com> wrote:
> > >
> > > > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > > > I got below report with 2.6.33.1 .
> > > > >
> > > > > unreferenced object 0xde144600 (size 64):
> > > > > comm "init", pid 1, jiffies 4294678101 (age 291.508s)
> > > > > hex dump (first 32 bytes):
> > > > > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > > > > 00 00 00 00 04 76 ae de d1 76 43 c0 d6 08 00 00 .....v...vC.....
> > > > > backtrace:
> > > > > [<c0481704>] create_object+0x121/0x1ef
> > > > > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > > > > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > > > > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > > > > [<c0437701>] alloc_pid+0x19/0x288
> > > > > [<c0428acc>] copy_process+0x95a/0xdac
> > > > > [<c04290d8>] do_fork+0x129/0x261
> > > > > [<c0407de5>] sys_clone+0x1f/0x24
> > > > > [<c040292d>] ptregs_clone+0x15/0x28
> > > > > [<ffffffff>] 0xffffffff
> > > > > unreferenced object 0xdfa96a40 (size 64):
> > > > > comm "login", pid 2259, jiffies 4294719437 (age 250.179s)
> > > > > hex dump (first 32 bytes):
> > > > > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > > > > 00 00 00 00 60 39 ae de d1 76 43 c0 bb 09 00 00 ....`9...vC.....
> > > > > backtrace:
> > > > > [<c0481704>] create_object+0x121/0x1ef
> > > > > [<c05f546b>] kmemleak_alloc+0x25/0x42
> > > > > [<c047e326>] kmemleak_alloc_recursive+0x1c/0x22
> > > > > [<c047e36e>] kmem_cache_alloc+0x42/0x68
> > > > > [<c0437701>] alloc_pid+0x19/0x288
> > > > > [<c0428acc>] copy_process+0x95a/0xdac
> > > > > [<c04290d8>] do_fork+0x129/0x261
> > > > > [<c0407de5>] sys_clone+0x1f/0x24
> > > > > [<c040292d>] ptregs_clone+0x15/0x28
> > > > > [<ffffffff>] 0xffffffff
> > > >
> > > > I reported similar leaks last year -
> > > > http://lkml.org/lkml/2009/7/8/422. There is some analysis in the thread
> > > > above of the reference counting but I couldn't figure out where it goes
> > > > wrong. It looks to me like there isn't any reference to a struct pid
> > > > block but its reference count is 2.
> > > >
> > > > There is a bugzilla entry as well -
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=13868
> > > >
> > >
> > > Let's bug some people by cc'ing them ;)
> >
> > Oh. It is hardly possibly to find the unbalanced get_pid() via grep.
> >
> > IIRC, I sent the debugging patch which tracks get/put pid, but I can't
> > recall if anybody tried this patch. Hmm, and I can't find that patch
> > or the previous discussion in my maildir...
> >
> > Catalin, Tetsuo, any chance you can remind me if this patch was used?
> >
> > Oleg.
>
> [ probably sounding like a moron, but... ]
>
> Looking through vt_ioctl.c, I get the feeling that the ttys will
> hang onto vc->vt_pid until either getting a SAK or until someone
> new logs in. I don't see where logging out will cause a reset_vc().
> So when the logged in task logs out, does vt_pid keep a ref to the
> pid which now no longer exists?
>
> Came to mind bc I notice that every trace you've sent has included
> /bin/login or X...
>
> -serge
In particular, if vt_ioctl is called with VT_RELDISP, then
complete_change_console() will get called, which will kill vc->vt_pid,
but it will only call reset_vc(vc) if that kill_pid failed. reset_vc()
is needed to do our last put_pid().
I could be way off base here, but...
-serge
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [2.6.31 and later] "struct pid" leak.
2010-04-01 17:21 ` Serge E. Hallyn
2010-04-01 17:33 ` Serge E. Hallyn
@ 2010-04-02 15:29 ` Oleg Nesterov
1 sibling, 0 replies; 18+ messages in thread
From: Oleg Nesterov @ 2010-04-02 15:29 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Andrew Morton, Catalin Marinas, Tetsuo Handa, linux-kernel,
Eric W. Biederman, Sukadev Bhattiprolu
Serge, Catalin, thanks a lot for the debugging output you sent
me privately ;)
On 04/01, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> >
> > Oh. It is hardly possibly to find the unbalanced get_pid() via grep.
>
> Looking through vt_ioctl.c,
Yes, my first reaction was "it must be tty" too ;)
I seem to understand what happens. If I am right, it is possible
to leak the pid even without X.
I'll try to check my theory and send the patch soon...
Oleg.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/1] tty: release_one_tty() forgets to put pids
2010-03-31 22:17 ` Andrew Morton
2010-04-01 16:52 ` Oleg Nesterov
@ 2010-04-02 16:04 ` Oleg Nesterov
2010-04-02 16:05 ` [PATCH 1/1] " Oleg Nesterov
` (2 more replies)
1 sibling, 3 replies; 18+ messages in thread
From: Oleg Nesterov @ 2010-04-02 16:04 UTC (permalink / raw)
To: Andrew Morton, Alan Cox, Greg KH, Linus Torvalds
Cc: Catalin Marinas, Tetsuo Handa, linux-kernel, Serge Hallyn,
Eric W. Biederman, Sukadev Bhattiprolu, stable
Add cc's.
On 03/31, Andrew Morton wrote:
>
> On Tue, 30 Mar 2010 16:31:13 +0100
> Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > I got below report with 2.6.33.1 .
> > >
> > > unreferenced object 0xde144600 (size 64):
> > > comm "init", pid 1, jiffies 4294678101 (age 291.508s)
> >
> > [... snip ...]
> >
> > I reported similar leaks last year -
> > http://lkml.org/lkml/2009/7/8/422. There is some analysis in the thread
> > above of the reference counting but I couldn't figure out where it goes
> > wrong. It looks to me like there isn't any reference to a struct pid
> > block but its reference count is 2.
> >
> > There is a bugzilla entry as well -
> > https://bugzilla.kernel.org/show_bug.cgi?id=13868
OK. I do not undertand ttys, absolutely. This means the patch
should not be applied without acks. And in fact I feel the patch
probably fixes the symptom, not the problem. But the logic in
disassociate_ctty() is beyond my understanding.
However, I think it is easy to explain the leak.
Catalin, Tetsuo, could you try this patch?
Oleg.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/1] tty: release_one_tty() forgets to put pids
2010-04-02 16:04 ` [PATCH 0/1] tty: release_one_tty() forgets to put pids Oleg Nesterov
@ 2010-04-02 16:05 ` Oleg Nesterov
2010-04-02 16:19 ` Oleg Nesterov
2010-04-02 17:46 ` Linus Torvalds
2010-04-03 2:40 ` [PATCH 0/1] " Tetsuo Handa
2010-04-03 3:08 ` Linus Torvalds
2 siblings, 2 replies; 18+ messages in thread
From: Oleg Nesterov @ 2010-04-02 16:05 UTC (permalink / raw)
To: Andrew Morton, Alan Cox, Greg KH, Linus Torvalds
Cc: Catalin Marinas, Tetsuo Handa, linux-kernel, Serge Hallyn,
Eric W. Biederman, Sukadev Bhattiprolu, stable
release_one_tty(tty) can be called when tty still has a reference
to pgrp/session. In this case we leak the pid.
The patch needs the ack from someone who understand tty magic.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
drivers/char/tty_io.c | 2 ++
1 file changed, 2 insertions(+)
--- TTT/drivers/char/tty_io.c~TTY_PID_LEAK 2010-03-17 16:00:59.000000000 +0100
+++ TTT/drivers/char/tty_io.c 2010-04-02 17:23:07.000000000 +0200
@@ -1423,6 +1423,8 @@ static void release_one_tty(struct work_
list_del_init(&tty->tty_files);
file_list_unlock();
+ put_pid(tty->pgrp);
+ put_pid(tty->session);
free_tty_struct(tty);
}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids
2010-04-02 16:05 ` [PATCH 1/1] " Oleg Nesterov
@ 2010-04-02 16:19 ` Oleg Nesterov
2010-04-02 17:46 ` Linus Torvalds
1 sibling, 0 replies; 18+ messages in thread
From: Oleg Nesterov @ 2010-04-02 16:19 UTC (permalink / raw)
To: Andrew Morton, Alan Cox, Greg KH, Linus Torvalds
Cc: Catalin Marinas, Tetsuo Handa, linux-kernel, Serge Hallyn,
Eric W. Biederman, Sukadev Bhattiprolu, stable
On 04/02, Oleg Nesterov wrote:
>
> release_one_tty(tty) can be called when tty still has a reference
> to pgrp/session. In this case we leak the pid.
>
> The patch needs the ack from someone who understand tty magic.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
I am sorry, I forgot it needs
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>
> drivers/char/tty_io.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> --- TTT/drivers/char/tty_io.c~TTY_PID_LEAK 2010-03-17 16:00:59.000000000 +0100
> +++ TTT/drivers/char/tty_io.c 2010-04-02 17:23:07.000000000 +0200
> @@ -1423,6 +1423,8 @@ static void release_one_tty(struct work_
> list_del_init(&tty->tty_files);
> file_list_unlock();
>
> + put_pid(tty->pgrp);
> + put_pid(tty->session);
> free_tty_struct(tty);
> }
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids
2010-04-02 16:05 ` [PATCH 1/1] " Oleg Nesterov
2010-04-02 16:19 ` Oleg Nesterov
@ 2010-04-02 17:46 ` Linus Torvalds
2010-04-02 18:22 ` Eric W. Biederman
` (2 more replies)
1 sibling, 3 replies; 18+ messages in thread
From: Linus Torvalds @ 2010-04-02 17:46 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Alan Cox, Greg KH, Catalin Marinas, Tetsuo Handa,
Linux Kernel Mailing List, Serge Hallyn, Eric W. Biederman,
Sukadev Bhattiprolu, stable
On Fri, 2 Apr 2010, Oleg Nesterov wrote:
>
> release_one_tty(tty) can be called when tty still has a reference
> to pgrp/session. In this case we leak the pid.
Hmm. Maybe we should have cleared this in tty_release() already. We
already do some of the session clearing there (but we clear the session in
the _tasks_ associated with the tty, not the tty session pointer).
But:
> The patch needs the ack from someone who understand tty magic.
I think the patch is simpler than worrying about the much more complex
release logic. So I think I actually prefer this patch over something that
tries to be clever in tty_release.
We might even push it into "free_tty_struct()", although I think that the
only non-release_one_tty() callers of that are the ones that allocated the
tty but due to some failure never connected it to anything. So on the
whole I think you picked the right spot.
So I'll ACK it. But maybe Alan sees some problem/issue I didn't see.
Linus
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids
2010-04-02 17:46 ` Linus Torvalds
@ 2010-04-02 18:22 ` Eric W. Biederman
2010-04-02 18:48 ` Oleg Nesterov
2010-04-02 18:43 ` Oleg Nesterov
2010-04-02 20:09 ` Alan Cox
2 siblings, 1 reply; 18+ messages in thread
From: Eric W. Biederman @ 2010-04-02 18:22 UTC (permalink / raw)
To: Linus Torvalds
Cc: Oleg Nesterov, Andrew Morton, Alan Cox, Greg KH, Catalin Marinas,
Tetsuo Handa, Linux Kernel Mailing List, Serge Hallyn,
Sukadev Bhattiprolu, stable
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Fri, 2 Apr 2010, Oleg Nesterov wrote:
>>
>> release_one_tty(tty) can be called when tty still has a reference
>> to pgrp/session. In this case we leak the pid.
>
> Hmm. Maybe we should have cleared this in tty_release() already. We
> already do some of the session clearing there (but we clear the session in
> the _tasks_ associated with the tty, not the tty session pointer).
>
> But:
>
>> The patch needs the ack from someone who understand tty magic.
>
> I think the patch is simpler than worrying about the much more complex
> release logic. So I think I actually prefer this patch over something that
> tries to be clever in tty_release.
>
> We might even push it into "free_tty_struct()", although I think that the
> only non-release_one_tty() callers of that are the ones that allocated the
> tty but due to some failure never connected it to anything. So on the
> whole I think you picked the right spot.
>
> So I'll ACK it. But maybe Alan sees some problem/issue I didn't see.
I agree. However we made it to release_one_tty with pids we need
to free them, before we free the tty structure itself.
My general paranoia would suggest setting the pids to NULL. So that
we don't have the chance of a use after free.
Eric
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids
2010-04-02 17:46 ` Linus Torvalds
2010-04-02 18:22 ` Eric W. Biederman
@ 2010-04-02 18:43 ` Oleg Nesterov
2010-04-02 20:09 ` Alan Cox
2 siblings, 0 replies; 18+ messages in thread
From: Oleg Nesterov @ 2010-04-02 18:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Alan Cox, Greg KH, Catalin Marinas, Tetsuo Handa,
Linux Kernel Mailing List, Serge Hallyn, Eric W. Biederman,
Sukadev Bhattiprolu, stable
On 04/02, Linus Torvalds wrote:
>
> On Fri, 2 Apr 2010, Oleg Nesterov wrote:
> >
> > release_one_tty(tty) can be called when tty still has a reference
> > to pgrp/session. In this case we leak the pid.
>
> Hmm. Maybe we should have cleared this in tty_release() already. We
> already do some of the session clearing there (but we clear the session in
> the _tasks_ associated with the tty, not the tty session pointer).
Yes, probably we can free them earlier.
But I am very nervous about this change, I tried to defer put_pid()
as much as possible, in case something else uses ->prgp/session
before free_tty_struct().
Oleg.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids
2010-04-02 18:22 ` Eric W. Biederman
@ 2010-04-02 18:48 ` Oleg Nesterov
0 siblings, 0 replies; 18+ messages in thread
From: Oleg Nesterov @ 2010-04-02 18:48 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, Andrew Morton, Alan Cox, Greg KH,
Catalin Marinas, Tetsuo Handa, Linux Kernel Mailing List,
Serge Hallyn, Sukadev Bhattiprolu, stable
On 04/02, Eric W. Biederman wrote:
>
> My general paranoia would suggest setting the pids to NULL. So that
> we don't have the chance of a use after free.
In this case, I don't think this is needed. We are doing
free_tty_struct()->kfree(tty) right after put_pid()s, nobody
can use these pointers or we have another bug.
Most probably this patch is correct (but perhaps it is not the best fix).
Every time tty does put_pid() it should also clear the pointer. But I am
not sure I grepped enough.
Oleg.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids
2010-04-02 17:46 ` Linus Torvalds
2010-04-02 18:22 ` Eric W. Biederman
2010-04-02 18:43 ` Oleg Nesterov
@ 2010-04-02 20:09 ` Alan Cox
2 siblings, 0 replies; 18+ messages in thread
From: Alan Cox @ 2010-04-02 20:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Oleg Nesterov, Andrew Morton, Alan Cox, Greg KH, Catalin Marinas,
Tetsuo Handa, Linux Kernel Mailing List, Serge Hallyn,
Eric W. Biederman, Sukadev Bhattiprolu, stable
> So I'll ACK it. But maybe Alan sees some problem/issue I didn't see.
I have no idea. Someone with the time (not me right now) will have to
trace the error paths or perhaps play safe and NULL any pointers so that
they can free it in the destructor only if one was set ?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/1] tty: release_one_tty() forgets to put pids
2010-04-02 16:04 ` [PATCH 0/1] tty: release_one_tty() forgets to put pids Oleg Nesterov
2010-04-02 16:05 ` [PATCH 1/1] " Oleg Nesterov
@ 2010-04-03 2:40 ` Tetsuo Handa
2010-04-03 3:08 ` Linus Torvalds
2 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2010-04-03 2:40 UTC (permalink / raw)
To: oleg, akpm, alan, greg, torvalds
Cc: catalin.marinas, linux-kernel, serue, ebiederm, sukadev, stable
Oleg Nesterov wrote:
> Add cc's.
>
> On 03/31, Andrew Morton wrote:
> >
> > On Tue, 30 Mar 2010 16:31:13 +0100
> > Catalin Marinas <catalin.marinas@arm.com> wrote:
> >
> > > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > > I got below report with 2.6.33.1 .
> > > >
> > > > unreferenced object 0xde144600 (size 64):
> > > > comm "init", pid 1, jiffies 4294678101 (age 291.508s)
> > >
> > > [... snip ...]
> > >
> > > I reported similar leaks last year -
> > > http://lkml.org/lkml/2009/7/8/422. There is some analysis in the thread
> > > above of the reference counting but I couldn't figure out where it goes
> > > wrong. It looks to me like there isn't any reference to a struct pid
> > > block but its reference count is 2.
> > >
> > > There is a bugzilla entry as well -
> > > https://bugzilla.kernel.org/show_bug.cgi?id=13868
>
> OK. I do not undertand ttys, absolutely. This means the patch
> should not be applied without acks. And in fact I feel the patch
> probably fixes the symptom, not the problem. But the logic in
> disassociate_ctty() is beyond my understanding.
>
> However, I think it is easy to explain the leak.
>
> Catalin, Tetsuo, could you try this patch?
>
I confirmed using 2.6.34-rc3 that this patch solved the leak.
Thank you.
> Oleg.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/1] tty: release_one_tty() forgets to put pids
2010-04-02 16:04 ` [PATCH 0/1] tty: release_one_tty() forgets to put pids Oleg Nesterov
2010-04-02 16:05 ` [PATCH 1/1] " Oleg Nesterov
2010-04-03 2:40 ` [PATCH 0/1] " Tetsuo Handa
@ 2010-04-03 3:08 ` Linus Torvalds
2010-04-03 5:15 ` [stable] " Greg KH
2 siblings, 1 reply; 18+ messages in thread
From: Linus Torvalds @ 2010-04-03 3:08 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Alan Cox, Greg KH, Catalin Marinas, Tetsuo Handa,
linux-kernel, Serge Hallyn, Eric W. Biederman,
Sukadev Bhattiprolu, stable
Ok, I committed this, but after committing I realized that the patch
didn't have the "Cc: stable@kernel.org", even if the email did.
So Greg et al: it's now commit 6da8d866d0d39e9509ff826660f6a86a6757c966 in
mainline.
Linus
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [stable] [PATCH 0/1] tty: release_one_tty() forgets to put pids
2010-04-03 3:08 ` Linus Torvalds
@ 2010-04-03 5:15 ` Greg KH
0 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2010-04-03 5:15 UTC (permalink / raw)
To: Linus Torvalds
Cc: Oleg Nesterov, Tetsuo Handa, linux-kernel, Sukadev Bhattiprolu,
Eric W. Biederman, Catalin Marinas, Serge Hallyn, Andrew Morton,
stable, Alan Cox
On Fri, Apr 02, 2010 at 08:08:55PM -0700, Linus Torvalds wrote:
>
>
> Ok, I committed this, but after committing I realized that the patch
> didn't have the "Cc: stable@kernel.org", even if the email did.
>
> So Greg et al: it's now commit 6da8d866d0d39e9509ff826660f6a86a6757c966 in
> mainline.
Thanks, I'll queue it up in the morning.
greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2010-04-03 5:20 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-27 12:21 [2.6.31 and later] "struct pid" leak Tetsuo Handa
2010-03-30 15:31 ` Catalin Marinas
2010-03-31 22:17 ` Andrew Morton
2010-04-01 16:52 ` Oleg Nesterov
2010-04-01 17:21 ` Serge E. Hallyn
2010-04-01 17:33 ` Serge E. Hallyn
2010-04-02 15:29 ` Oleg Nesterov
2010-04-02 16:04 ` [PATCH 0/1] tty: release_one_tty() forgets to put pids Oleg Nesterov
2010-04-02 16:05 ` [PATCH 1/1] " Oleg Nesterov
2010-04-02 16:19 ` Oleg Nesterov
2010-04-02 17:46 ` Linus Torvalds
2010-04-02 18:22 ` Eric W. Biederman
2010-04-02 18:48 ` Oleg Nesterov
2010-04-02 18:43 ` Oleg Nesterov
2010-04-02 20:09 ` Alan Cox
2010-04-03 2:40 ` [PATCH 0/1] " Tetsuo Handa
2010-04-03 3:08 ` Linus Torvalds
2010-04-03 5:15 ` [stable] " Greg KH
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.