"struct pid" leak.

2.6.31 and later In review Tetsuo Handa 9 participants 18 messages Mar 27, 2010 – Apr 3, 2010 Char and misc drivers

Share & tools

Tetsuo Handa

author

2010-03-27 12:22 UTC #0 lore raw

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.

Catalin Marinas

2010-03-30 15:31 UTC #1 lore raw

Quoted 26 lines · 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

Quoted 8 lines

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

Signature · 1 line

Catalin

Andrew Morton

2010-03-31 22:18 UTC #2 lore raw

Quoted 36 lines · On Tue, 30 Mar 2010 16:31:13 +0100 Catalin Marinas <[email protected]>…

Quoted 26 lines · 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

Quoted 8 lines

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

Quoted 9 lines

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

Oleg Nesterov

2010-04-01 16:54 UTC #3 lore raw

Quoted 46 lines · Andrew Morton

Quoted 42 lines · On Tue, 30 Mar 2010 16:31:13 +0100 Catalin Marinas <[email protected]>…
Quoted 26 lines · 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
Quoted 8 lines
    [<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.

Quoted 5 lines

Quoted 3 lines
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.

Serge E. Hallyn

2010-04-01 17:21 UTC #4 lore raw

Quoted 55 lines · Oleg Nesterov

Quoted 46 lines · Andrew Morton
Quoted 42 lines · On Tue, 30 Mar 2010 16:31:13 +0100 Catalin Marinas <[email protected]>…
Quoted 26 lines · 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
Quoted 8 lines
    [<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.
Quoted 5 lines
Quoted 3 lines
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.

Quoted 7 lines

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

Serge E. Hallyn

2010-04-01 17:33 UTC #5 lore raw

Quoted 72 lines · Serge E. Hallyn

Quoted 55 lines · Oleg Nesterov
Quoted 46 lines · Andrew Morton
Quoted 42 lines · On Tue, 30 Mar 2010 16:31:13 +0100 Catalin Marinas <[email protected]>…
Quoted 26 lines · 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
Quoted 8 lines
    [<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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 7 lines
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?

Quoted 4 lines

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

Oleg Nesterov

2010-04-02 15:31 UTC #6 lore raw

In reply to Serge E. Hallyn

Serge, Catalin, thanks a lot for the debugging output you sent me privately ;)

Quoted 6 lines · Serge E. Hallyn

Quoted 2 lines · Oleg Nesterov
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.

Oleg Nesterov

2010-04-02 16:07 UTC #7 lore raw

cover tty: release_one_tty() forgets to put pids

In reply to Andrew Morton

Add cc's.

Quoted 12 lines · Andrew Morton

Quoted 8 lines · On Tue, 30 Mar 2010 16:31:13 +0100 Catalin Marinas <[email protected]>…
Quoted 4 lines · 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)
[... snip ...]

Quoted 8 lines

Quoted 8 lines
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.

Oleg Nesterov

2010-04-02 16:07 UTC #8 lore raw

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

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 <[email protected]>

---

 drivers/char/tty_io.c |    2 ++
 1 file changed, 2 insertions(+)

drivers/char/tty_io.c renamed from drivers/char/tty_io.c~TTY_PID_LEAK +2 -0

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

Oleg Nesterov

2010-04-02 16:21 UTC #9 lore raw

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

Quoted 7 lines · 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 <[email protected]>

I am sorry, I forgot it needs

Reported-by Catalin Marinas <[email protected]>

Reported-by Tetsuo Handa <[email protected]>

Quoted 16 lines

---

 drivers/char/tty_io.c |    2 ++
 1 file changed, 2 insertions(+)

drivers/char/tty_io.c renamed from drivers/char/tty_io.c~TTY_PID_LEAK +2 -0

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

Linus Torvalds

2010-04-02 17:51 UTC #10 lore raw

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

In reply to Oleg Nesterov

Quoted 3 lines · 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.

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:

Quoted 1 line

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

Eric W. Biederman

2010-04-02 18:22 UTC #11 lore raw

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

Quoted 17 lines · Linus Torvalds

Quoted 3 lines · 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.
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:
Quoted 1 line
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.

Quoted 6 lines

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

Oleg Nesterov

2010-04-02 18:50 UTC #12 lore raw

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

Quoted 3 lines · Eric W. Biederman

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.

Oleg Nesterov

2010-04-02 18:45 UTC #13 lore raw

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

In reply to Linus Torvalds

Quoted 9 lines · Linus Torvalds

Quoted 3 lines · 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.
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.

Alan Cox

2010-04-02 20:08 UTC #14 lore raw

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

In reply to Linus Torvalds

Quoted 1 line

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 ?

Tetsuo Handa

author

2010-04-03 02:40 UTC #15 lore raw

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

In reply to Oleg Nesterov

Quoted 24 lines · Oleg Nesterov

Add cc's.
Quoted 12 lines · Andrew Morton
Quoted 8 lines · On Tue, 30 Mar 2010 16:31:13 +0100 Catalin Marinas <[email protected]>…
Quoted 4 lines · 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)
[... snip ...]
Quoted 8 lines
Quoted 8 lines
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

Quoted 9 lines

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.

Quoted 1 line

Oleg.

Linus Torvalds

2010-04-03 03:13 UTC #16 lore raw

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

In reply to Oleg Nesterov

Ok, I committed this, but after committing I realized that the patch didn't have the "Cc: [email protected]", even if the email did.

So Greg et al: it's now commit 6da8d866d0d39e9509ff826660f6a86a6757c966 in mainline.

Linus

Greg KH

2010-04-03 05:20 UTC #17 lore raw

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

Quoted 7 lines · Linus Torvalds

Ok, I committed this, but after committing I realized that the patch didn't have the "Cc: [email protected]", 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