[2.6.31 and later] "struct pid" leak.

[2.6.31 and later] "struct pid" leak.

[Tetsuo Handa]

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.

Re: [2.6.31 and later] "struct pid" leak.

[Catalin Marinas]

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

Re: [2.6.31 and later] "struct pid" leak.

[Andrew Morton]

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

Re: [2.6.31 and later] "struct pid" leak.

[Oleg Nesterov]

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.

Re: [2.6.31 and later] "struct pid" leak.

[Serge E. Hallyn]

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

Re: [2.6.31 and later] "struct pid" leak.

[Serge E. Hallyn]

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

Re: [2.6.31 and later] "struct pid" leak.

[Oleg Nesterov]

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.

[PATCH 0/1] tty: release_one_tty() forgets to put pids

[Oleg Nesterov]

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.

[PATCH 1/1] tty: release_one_tty() forgets to put pids

[Oleg Nesterov]

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

Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids

[Oleg Nesterov]

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

Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids

[Linus Torvalds]

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

Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids

[Eric W. Biederman]

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

Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids

[Oleg Nesterov]

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.

Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids

[Oleg Nesterov]

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.

Re: [PATCH 1/1] tty: release_one_tty() forgets to put pids

[Alan Cox]

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

Re: [PATCH 0/1] tty: release_one_tty() forgets to put pids

[Tetsuo Handa]

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.

Re: [PATCH 0/1] tty: release_one_tty() forgets to put pids

[Linus Torvalds]

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

Re: [stable] [PATCH 0/1] tty: release_one_tty() forgets to put pids

[Greg KH]

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