Possible "struct pid" leak from tty_io.c

Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

Hi Eric,

I'm trying to track down a kmemleak report (on an ARM platform) which
seems to have appeared with commit
ab521dc0f8e117fd808d3e425216864d60390500. As I'm not familiar with the
TTY layer at all, is it possible that the above commit missed a
put_pid() call on some path?

The /sbin/init application calls sys_clone() a few times but only one
leak is reported (see below). Looking at the reported pid object (at
0xc7c14500), count is 2 and nr is 296 but no process with pid 296
exists any more.

unreferenced object 0xc7c14500 (size 36):
  comm "init", pid 245, jiffies 4294939289
  backtrace:
    [<c0070c18>] kmem_cache_alloc
    [<c003a528>] alloc_pid
    [<c0026468>] do_fork
    [<c00153b0>] sys_clone
    [<c0010f80>] ret_fast_syscall

Thanks.

-- 
Catalin

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> Hi Eric,
>
> I'm trying to track down a kmemleak report (on an ARM platform) which
> seems to have appeared with commit
> ab521dc0f8e117fd808d3e425216864d60390500. As I'm not familiar with the
> TTY layer at all, is it possible that the above commit missed a
> put_pid() call on some path?

I won't arbitrarily rule a missing put_pid out.  I have been know to
goof up upon occasion.

I just did a quick look to see what kmemleak is.  A conservative
tracing leak detector sounds interesting.  Except for all of the list
heads which lead to container_of calls I don't know of anything in the
struct pid implementation that would be difficult for it to work with.
Well that and there is some rcu access protection which can delay the
free by a bit.

> The /sbin/init application calls sys_clone() a few times but only one
> leak is reported (see below). Looking at the reported pid object (at
> 0xc7c14500), count is 2 and nr is 296 but no process with pid 296
> exists any more.

It could still be a valid session or a process group id.
If you examine the struct pid you can test for this be examining all
of the list heads it keeps.  If there is something on any of the
lists that would account a count of 1.  How we have a count of 2
I don't have enough information to guess.

Core tty layer handling stops having an remembering pids when the
session or process group exits so it is relatively safe from pid wrap
around issues without my changes and should make the kind of thing you
are reporting very unlikely in a correctly functioning system.

In most any other layer we cache pids indefinitely and a situation
where we have a pointer to a struct pid with a ref count of 1 long
after the process goes away is expected.  In this case it is better
to hold a reference to a struct pid so we don't do the wrong thing
when pid wrap around occurs then to hold a reference to an entire
task_struct and lock that in place.

I don't understand your situation enough to guess what is going wrong
yet.  Hopefully I have given you enough information to get started.


> unreferenced object 0xc7c14500 (size 36):
>  comm "init", pid 245, jiffies 4294939289
>  backtrace:
>    [<c0070c18>] kmem_cache_alloc
>    [<c003a528>] alloc_pid
>    [<c0026468>] do_fork
>    [<c00153b0>] sys_clone
>    [<c0010f80>] ret_fast_syscall

I think this is the path that all pid structures come from so
unfortunately that doesn't help tracing this problem down.

Eric

Re: Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

On 08/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> > I'm trying to track down a kmemleak report (on an ARM platform) which
> > seems to have appeared with commit
> > ab521dc0f8e117fd808d3e425216864d60390500. As I'm not familiar with the
> > TTY layer at all, is it possible that the above commit missed a
> > put_pid() call on some path?
>
> I won't arbitrarily rule a missing put_pid out.  I have been know to
> goof up upon occasion.

I'm not entirely sure it's this part of the code, I would have to do
some more investigations (I didn't get this leak before). An
"unscientific" test shows that if I define get_pid/put_pid in the
tty_io.c file so that pid->count is not affected, the leak disappears.
This doesn't necessarily prove that the fault is here though.

> I just did a quick look to see what kmemleak is.  A conservative
> tracing leak detector sounds interesting.  Except for all of the list
> heads which lead to container_of calls I don't know of anything in the
> struct pid implementation that would be difficult for it to work with.
> Well that and there is some rcu access protection which can delay the
> free by a bit.

Kmemleak can cope with list heads and rcu delayed freeing as it also
checks for pointer aliases (those accessible via container_of).

> > The /sbin/init application calls sys_clone() a few times but only one
> > leak is reported (see below). Looking at the reported pid object (at
> > 0xc7c14500), count is 2 and nr is 296 but no process with pid 296
> > exists any more.
>
> It could still be a valid session or a process group id.
> If you examine the struct pid you can test for this be examining all
> of the list heads it keeps.  If there is something on any of the
> lists that would account a count of 1.  How we have a count of 2
> I don't have enough information to guess.

I think it's only the pid_chain and rcu member that could be placed in
a list and kmemleak scans the memory for these two offsets as well.
I'll check those lists anyway but I doubt it's a more fundamental
problem with how kmemleak handles struct pid as I should've probably
got more reports.

> In most any other layer we cache pids indefinitely and a situation
> where we have a pointer to a struct pid with a ref count of 1 long
> after the process goes away is expected.

Yes, indeed, but what kmemleak reports is that the pid structure
wasn't freed yet and there is no way to determine its pointer directly
or via container_of on members (by scanning the memory), hence it is
considered a leak.

> I don't understand your situation enough to guess what is going wrong
> yet.  Hopefully I have given you enough information to get started.

Yes, many thanks. I'll dig further and let you know.

-- 
Catalin

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 08/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>
> I think it's only the pid_chain and rcu member that could be placed in
> a list and kmemleak scans the memory for these two offsets as well.
> I'll check those lists anyway but I doubt it's a more fundamental
> problem with how kmemleak handles struct pid as I should've probably
> got more reports.

Right.  I was pointing out the possibilities but because we do
some tricky things.  Mostly I was wondering about the hlist for
the list of tasks.  Now if a task is on that list we should have
a struct pid_link pointing at our struct pid, so it shouldn't fool
kmemleak but I'm still a little curious if all of those hlist_heads are
NULL pointers.

>> In most any other layer we cache pids indefinitely and a situation
>> where we have a pointer to a struct pid with a ref count of 1 long
>> after the process goes away is expected.
>
> Yes, indeed, but what kmemleak reports is that the pid structure
> wasn't freed yet and there is no way to determine its pointer directly
> or via container_of on members (by scanning the memory), hence it is
> considered a leak.

Yes that sounds like a leak.

>> I don't understand your situation enough to guess what is going wrong
>> yet.  Hopefully I have given you enough information to get started.
>
> Yes, many thanks. I'll dig further and let you know.

Thanks....

Eric

Re: Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

Eric,

For a longer explanation, see the second part of this e-mail. In
short, the patch below seems to fix this particular leak. I'm not sure
that's the correct/complete fix as I seem to still get a 2nd report.
Any info is welcomed.

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index e453268..4e33dc2 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1375,6 +1375,9 @@ static void do_tty_hangup(struct work_struct *work)
 	}
 	read_unlock(&tasklist_lock);

+	put_pid(tty->session);
+	put_pid(tty->pgrp);
+
 	tty->flags = 0;
 	tty->session = NULL;
 	tty->pgrp = NULL;

On 08/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> > The /sbin/init application calls sys_clone() a few times but only one
> > leak is reported (see below). Looking at the reported pid object (at
> > 0xc7c14500), count is 2 and nr is 296 but no process with pid 296
> > exists any more.
[...]
> > unreferenced object 0xc7c14500 (size 36):
> >  comm "init", pid 245, jiffies 4294939289
> >  backtrace:
> >    [<c0070c18>] kmem_cache_alloc
> >    [<c003a528>] alloc_pid
> >    [<c0026468>] do_fork
> >    [<c00153b0>] sys_clone
> >    [<c0010f80>] ret_fast_syscall
>
> I think this is the path that all pid structures come from so
> unfortunately that doesn't help tracing this problem down.

No, indeed, but that's the only thing kmemleak can report. Anyway, I
got some more information now, after adding several printk's:

The difference from other pid objects is that this one (with nr 296)
is passed as a parameter to proc_set_tty(). The __proc_set_tty()
function increments the pid->count twice via get_pid(), and, with two
other get_pid calls, the pid->count for this object gets to 5 (1 being
the initial value). The prints below are function name, struct pid
address (different from the runs yesterday though), pid->nr and
pid->count (after get_pid incrementing). It also show the return
address and symbol (the calling function):

  alloc_pid: c7c149d8, 296, 1
  get_pid: c7c149d8, 296, 2
    return: c0122d64 (proc_set_tty+0x34/0x54)
  get_pid: c7c149d8, 296, 3
    return: c0122d64 (proc_set_tty+0x34/0x54)
  get_pid: c7c149d8, 296, 4
    return: c002b328 (do_exit+0x2e4/0x7f8) - this is actually the get_pid
      in disassociate_ctty but it is reported like this because of get_pid
      inlining
  get_pid: c7c149d8, 296, 5
    return: c0124a0c (tty_vhangup+0x14/0x18)

On the exit path (see below), however, put_pid is called twice before
free_pid and once via release_task -> detach_pid -> free_pid -> ... ->
__rcu_process_callbacks -> delayed_put_pid -> put_pid. Note that
free_pid is called with pid->nr == 3 and the last put_pid gets called
with nr == 3 as well (but it decrements it to 2 and that's what I find
at that memory location). In the trace below, the pid->count is
printed before put_pid modifies it:

  put_pid: c7c149d8, 296, 5
    return: c0124b5c (disassociate_ctty+0x14c/0x230)
  put_pid: c7c149d8, 296, 4
    return: c0124ba8 (disassociate_ctty+0x198/0x230)
  detach_pid: c7c149d8, 296, 3
    return: c002a230 (release_task+0x1c0/0x358)
  detach_pid: c7c149d8, 296, 3
    return: c002a248 (release_task+0x1d8/0x358)
  detach_pid: c7c149d8, 296, 3
    return: c002a254 (release_task+0x1e4/0x358)
  free_pid: c7c149d8, 296, 3
    return: c003a990 (detach_pid+0xac/0xc8)
  ...
  delayed_put_pid: c7c149d8, 296, 3
    return: c003af68 (__rcu_process_callbacks+0x19c/0x25c)
  put_pid: c7c149d8, 296, 3
    return: c003a8cc (delayed_put_pid+0x54/0x6c)

In the above disassociate_ctty() function the code below (line 1542)
doesn't seem to get called:

	tty = get_current_tty();
	if (tty) {
		put_pid(tty->session);
		put_pid(tty->pgrp);
		tty->session = NULL;
		tty->pgrp = NULL;
	} else {

and I get the following error if TTY_DEBUG_HANGUP is defined - "error
attempted to write to tty [0x00000000] = NULL".

It looks like the tty_vhangup() call in in disassociate_ctty() sets
current->signal->tty to NULL in the do_each_pid_task loop in
do_tty_hangup (p->signal->tty = NULL). The second call to
get_current_tty() in disassociate_ctty() return NULL and therefore no
put_pid on tty->session and tty->pgrp (which are also set to NULL in
the previous function).

Regards.

-- 
Catalin

Re: Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

On 09/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>
> > On 08/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> >> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> >
> > I think it's only the pid_chain and rcu member that could be placed in
> > a list and kmemleak scans the memory for these two offsets as well.
> > I'll check those lists anyway but I doubt it's a more fundamental
> > problem with how kmemleak handles struct pid as I should've probably
> > got more reports.
>
> Right.  I was pointing out the possibilities but because we do
> some tricky things.  Mostly I was wondering about the hlist for
> the list of tasks.  Now if a task is on that list we should have
> a struct pid_link pointing at our struct pid, so it shouldn't fool
> kmemleak but I'm still a little curious if all of those hlist_heads are
> NULL pointers.

Yes, all the 3 hlist_head tasks are NULL pointers on the reported object.

-- 
Catalin

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> Eric,
>
> For a longer explanation, see the second part of this e-mail. In
> short, the patch below seems to fix this particular leak. I'm not sure
> that's the correct/complete fix as I seem to still get a 2nd report.
> Any info is welcomed.

Sure.  I was starting to suspect that location myself.

> diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
> index e453268..4e33dc2 100644
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -1375,6 +1375,9 @@ static void do_tty_hangup(struct work_struct *work)
> 	}
> 	read_unlock(&tasklist_lock);
>
> +	put_pid(tty->session);
> +	put_pid(tty->pgrp);
> +
> 	tty->flags = 0;
> 	tty->session = NULL;
> 	tty->pgrp = NULL;
>
> On 08/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>> > The /sbin/init application calls sys_clone() a few times but only one
>> > leak is reported (see below). Looking at the reported pid object (at
>> > 0xc7c14500), count is 2 and nr is 296 but no process with pid 296
>> > exists any more.
> [...]
>> > unreferenced object 0xc7c14500 (size 36):
>> >  comm "init", pid 245, jiffies 4294939289
>> >  backtrace:
>> >    [<c0070c18>] kmem_cache_alloc
>> >    [<c003a528>] alloc_pid
>> >    [<c0026468>] do_fork
>> >    [<c00153b0>] sys_clone
>> >    [<c0010f80>] ret_fast_syscall
>>
>> I think this is the path that all pid structures come from so
>> unfortunately that doesn't help tracing this problem down.
>
> No, indeed, but that's the only thing kmemleak can report. Anyway, I
> got some more information now, after adding several printk's:
>
> The difference from other pid objects is that this one (with nr 296)
> is passed as a parameter to proc_set_tty(). The __proc_set_tty()
> function increments the pid->count twice via get_pid(), and, with two
> other get_pid calls, the pid->count for this object gets to 5 (1 being
> the initial value). The prints below are function name, struct pid
> address (different from the runs yesterday though), pid->nr and
> pid->count (after get_pid incrementing). It also show the return
> address and symbol (the calling function):
>
>  alloc_pid: c7c149d8, 296, 1
>  get_pid: c7c149d8, 296, 2
>    return: c0122d64 (proc_set_tty+0x34/0x54)
>  get_pid: c7c149d8, 296, 3
>    return: c0122d64 (proc_set_tty+0x34/0x54)
>  get_pid: c7c149d8, 296, 4
>    return: c002b328 (do_exit+0x2e4/0x7f8) - this is actually the get_pid
>      in disassociate_ctty but it is reported like this because of get_pid
>      inlining
>  get_pid: c7c149d8, 296, 5
>    return: c0124a0c (tty_vhangup+0x14/0x18)
>
> On the exit path (see below), however, put_pid is called twice before
> free_pid and once via release_task -> detach_pid -> free_pid -> ... ->
> __rcu_process_callbacks -> delayed_put_pid -> put_pid. Note that
> free_pid is called with pid->nr == 3 and the last put_pid gets called
> with nr == 3 as well (but it decrements it to 2 and that's what I find
> at that memory location). In the trace below, the pid->count is
> printed before put_pid modifies it:
>
>  put_pid: c7c149d8, 296, 5
>    return: c0124b5c (disassociate_ctty+0x14c/0x230)
>  put_pid: c7c149d8, 296, 4
>    return: c0124ba8 (disassociate_ctty+0x198/0x230)
>  detach_pid: c7c149d8, 296, 3
>    return: c002a230 (release_task+0x1c0/0x358)
>  detach_pid: c7c149d8, 296, 3
>    return: c002a248 (release_task+0x1d8/0x358)
>  detach_pid: c7c149d8, 296, 3
>    return: c002a254 (release_task+0x1e4/0x358)
>  free_pid: c7c149d8, 296, 3
>    return: c003a990 (detach_pid+0xac/0xc8)
>  ...
>  delayed_put_pid: c7c149d8, 296, 3
>    return: c003af68 (__rcu_process_callbacks+0x19c/0x25c)
>  put_pid: c7c149d8, 296, 3
>    return: c003a8cc (delayed_put_pid+0x54/0x6c)
>
> In the above disassociate_ctty() function the code below (line 1542)
> doesn't seem to get called:
>
> 	tty = get_current_tty();
> 	if (tty) {
> 		put_pid(tty->session);
> 		put_pid(tty->pgrp);
> 		tty->session = NULL;
> 		tty->pgrp = NULL;
> 	} else {
>
> and I get the following error if TTY_DEBUG_HANGUP is defined - "error
> attempted to write to tty [0x00000000] = NULL".
>
> It looks like the tty_vhangup() call in in disassociate_ctty() sets
> current->signal->tty to NULL in the do_each_pid_task loop in
> do_tty_hangup (p->signal->tty = NULL). The second call to
> get_current_tty() in disassociate_ctty() return NULL and therefore no
> put_pid on tty->session and tty->pgrp (which are also set to NULL in
> the previous function).

Thanks.

If I can manage to focus on this, it looks like the information I need to 
start fixing this.

Adding the reference counting when we didn't have any before is always
interesting.

Eric

Re: Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

On 09/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> If I can manage to focus on this, it looks like the information I need to
> start fixing this.

I had a look at the second leak reported it seems to be caused by the
same proc_set_tty() call but, in this case, there is no
disassociate_tty() call for the task (and the patch I posted is not
enough). Maybe something like below (no thourough testing):

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index db91398..ea6ca7d 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -3854,7 +3854,14 @@ EXPORT_SYMBOL(tty_devnum);

 void proc_clear_tty(struct task_struct *p)
 {
+	struct tty_struct *tty;
+
 	spin_lock_irq(&p->sighand->siglock);
+	tty = p->signal->tty;
+	if (tty) {
+		put_pid(tty->session);
+		put_pid(tty->pgrp);
+	}
 	p->signal->tty = NULL;
 	spin_unlock_irq(&p->sighand->siglock);
 }

-- 
Catalin

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 09/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> If I can manage to focus on this, it looks like the information I need to
>> start fixing this.
>
> I had a look at the second leak reported it seems to be caused by the
> same proc_set_tty() call but, in this case, there is no
> disassociate_tty() call for the task (and the patch I posted is not
> enough). Maybe something like below (no thourough testing):

Thanks.  That doesn't look to bad. I'm just about to look into this.
If I'm lucky all I will need to do is a close code review of your
patch.

Hopefully I can do that this afternoon.

> diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
> index db91398..ea6ca7d 100644
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -3854,7 +3854,14 @@ EXPORT_SYMBOL(tty_devnum);
>
> void proc_clear_tty(struct task_struct *p)
> {
> +	struct tty_struct *tty;
> +
> 	spin_lock_irq(&p->sighand->siglock);
> +	tty = p->signal->tty;
> +	if (tty) {
> +		put_pid(tty->session);
> +		put_pid(tty->pgrp);
> +	}
> 	p->signal->tty = NULL;
> 	spin_unlock_irq(&p->sighand->siglock);
> }

Eric

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 09/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> If I can manage to focus on this, it looks like the information I need to
>> start fixing this.
>
> I had a look at the second leak reported it seems to be caused by the
> same proc_set_tty() call but, in this case, there is no
> disassociate_tty() call for the task (and the patch I posted is not
> enough). Maybe something like below (no thourough testing):
>
> diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
> index db91398..ea6ca7d 100644
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -3854,7 +3854,14 @@ EXPORT_SYMBOL(tty_devnum);
>
> void proc_clear_tty(struct task_struct *p)
> {
> +	struct tty_struct *tty;
> +
> 	spin_lock_irq(&p->sighand->siglock);
> +	tty = p->signal->tty;
> +	if (tty) {
> +		put_pid(tty->session);
> +		put_pid(tty->pgrp);
> +	}
> 	p->signal->tty = NULL;
> 	spin_unlock_irq(&p->sighand->siglock);
> }

This patch can't be right.  Not the way proc_clear_tty is called
once for each process in the session, plus we aren't clearing
tty->session and tty->pgrp here. 

If the above patch works it's a fluke.

Still it is the right general area of the code.  I've just started
looking at this it is going to take me a bit to come up to speed on
this code again and see what silly thing is missing.

Eric

Re: Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

On 13/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> > void proc_clear_tty(struct task_struct *p)
> > {
> > +     struct tty_struct *tty;
> > +
> >       spin_lock_irq(&p->sighand->siglock);
> > +     tty = p->signal->tty;
> > +     if (tty) {
> > +             put_pid(tty->session);
> > +             put_pid(tty->pgrp);
> > +     }
> >       p->signal->tty = NULL;
> >       spin_unlock_irq(&p->sighand->siglock);
> > }
>
> This patch can't be right.  Not the way proc_clear_tty is called
> once for each process in the session, plus we aren't clearing
> tty->session and tty->pgrp here.
>
> If the above patch works it's a fluke.

I looked at the logs and the pointer isn't freed indeed. It is just a
false negative in kmemleak and it would appear as a leak at some
point. But the previous patch (do_tty_hangup) seems to fix one of the
leaks.

For the 2nd leak, proc_set_tty is called and, for symmetry, I added
put_pid in proc_clear_tty (but without any deep thought). I also
haven't checked any lockdep issues with adding put_pid when
p->sighand->siglock is held.

-- 
Catalin

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

How does this look?

I don't have the setup to test this easily, but this bit makes seems to make
sense.  I will keep code reviewing and see if I can convince myself that this
is correct or incorrect in the mean time...

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index e453268..fc125ac 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1376,6 +1376,8 @@ static void do_tty_hangup(struct work_struct *work)
 	read_unlock(&tasklist_lock);
 
 	tty->flags = 0;
+	put_pid(tty->session);
+	put_pid(tty->pgrp);
 	tty->session = NULL;
 	tty->pgrp = NULL;
 	tty->ctrl_status = 0;
@@ -3841,6 +3843,8 @@ static struct pid *__proc_set_tty(struct task_struct *tsk, struct tty_struct *tt
 {
 	struct pid *old_pgrp;
 	if (tty) {
+		put_pid(tty->session);
+		put_pid(tty->pgrp);
 		tty->session = get_pid(task_session(tsk));
 		tty->pgrp = get_pid(task_pgrp(tsk));
 	}

Re: Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

On 14/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> How does this look?

It seems to fix the leak. I looked at the logs and proc_set_tty calls
put_pid twice for pid 245 (the unresolved leak) and get_pid for pid
296, which is later passed to put_pid via do_tty_hangup.

I still get the "error attempted to write to tty [0x00000000] = NULL"
when debugging is enabled in tty_io.c but it seems harmless.

> I don't have the setup to test this easily, but this bit makes seems to
> make sense.

And I don't have a fully tested kmemleak patch to post either :-).

Thanks.

-- 
Catalin

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 14/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> How does this look?
>
> It seems to fix the leak. I looked at the logs and proc_set_tty calls
> put_pid twice for pid 245 (the unresolved leak) and get_pid for pid
> 296, which is later passed to put_pid via do_tty_hangup.

I can see where this would.  Now I do have a concern that proc_set_tty.
With my current foggy recollections of the semantics of how SIGHUP is
sent I think both callers of proc_set_tty are buggy.  We steal away
the tty and don't send SIGHUP to the old users of the tty.

For flush_unauthorized files making that case looks fairly easy.
For tiocsctty this it looks more difficult.

I need to carefully read through what the rules are again to be certain.
There are legitimate cases for not sending SIGHUP.

> I still get the "error attempted to write to tty [0x00000000] = NULL"
> when debugging is enabled in tty_io.c but it seems harmless.

Yes.  I think that is the last vestiges of a recent tty layer debugging
session.  The code is 8 dec 2006, and came in when we started testing
for NULL and making a NULL tty there harmless.

I remember walking through disassociate_ctty several months ago and
not finding any bugs, but I might look again.

So anyway I almost have this.

Eric

Re: Possible "struct pid" leak from tty_io.c

[Eric W. Biederman]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 14/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> How does this look?
>
> It seems to fix the leak. I looked at the logs and proc_set_tty calls
> put_pid twice for pid 245 (the unresolved leak) and get_pid for pid
> 296, which is later passed to put_pid via do_tty_hangup.

Ok.  Any chance you could help me track down which application is
ultimately calling proc_set_tty (I think it has pid 296 in your case).

>From what I can tell reading the source it is calling
TIOCSCTTY with the arg field set to 1.  Which is a linux extension
to force other people off of the tty.

My skimming of the implementation says to me that we are forcing
other processes of the tty in the wrong way.  I think we should call
tty_vhangup (so those processes kicked off get normal terminal hangup
behavior) and not simply session_clear_tty.  However if I am correct the
implementation has been broken for over a decade so I am reluctant to
just change it without tracking down the users.  

The patch below is what I am looking at for a comprehensive fix.  I
think it fixes the second set of leaks in a better manner by simply
fixing callers to do the sane thing.

Eric


diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index e453268..5140f15 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1376,6 +1376,8 @@ static void do_tty_hangup(struct work_struct *work)
 	read_unlock(&tasklist_lock);
 
 	tty->flags = 0;
+	put_pid(tty->session);
+	put_pid(tty->pgrp);
 	tty->session = NULL;
 	tty->pgrp = NULL;
 	tty->ctrl_status = 0;
@@ -2972,9 +2974,7 @@ static int tiocsctty(struct tty_struct *tty, int arg)
 			/*
 			 * Steal it away
 			 */
-			read_lock(&tasklist_lock);
-			session_clear_tty(tty->session);
-			read_unlock(&tasklist_lock);
+			tty_vhangup(tty);
 		} else {
 			ret = -EPERM;
 			goto unlock;
@@ -3850,7 +3850,7 @@ static struct pid *__proc_set_tty(struct task_struct *tsk, struct tty_struct *tt
 	return old_pgrp;
 }
 
-void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
+static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
 {
 	struct pid *old_pgrp;
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index dee72b9..5f7a5fb 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -333,7 +333,6 @@ extern int tty_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 
 extern dev_t tty_devnum(struct tty_struct *tty);
 extern void proc_clear_tty(struct task_struct *p);
-extern void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
 extern struct tty_struct *get_current_tty(void);
 
 extern struct mutex tty_mutex;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 19a385e..84b489a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1759,12 +1759,16 @@ static inline void flush_unauthorized_files(struct files_struct * files)
 		}
 		file_list_unlock();
 
-		/* Reset controlling tty. */
-		if (drop_tty)
-			proc_set_tty(current, NULL);
 	}
 	mutex_unlock(&tty_mutex);
 
+	/* Reset controlling tty. */
+	if (drop_tty) {
+		if (current->signal->leader)
+			disassociate_ctty(0);
+		proc_clear_tty(current);
+	}
+
 	/* Revalidate access to inherited open files. */
 
 	AVC_AUDIT_DATA_INIT(&ad,FS);

Re: Possible "struct pid" leak from tty_io.c

[Catalin Marinas]

On 16/03/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> > It seems to fix the leak. I looked at the logs and proc_set_tty calls
> > put_pid twice for pid 245 (the unresolved leak) and get_pid for pid
> > 296, which is later passed to put_pid via do_tty_hangup.
>
> Ok.  Any chance you could help me track down which application is
> ultimately calling proc_set_tty (I think it has pid 296 in your case).

I'll look at this on Monday since I was trying it on an ARM embedded
platform in the office (with a Debian filesystem).

-- 
Catalin

[PATCH] tty: Fix two reported pid leaks

[Eric W. Biederman]

These leaks were reported by: Catalin Marinas <catalin.marians@gmail.com>
and I have been able to very by inspection they are possible.

When converting tty_io.c to store pids as struct pid pointers instead
of pid_t values it appears I overlooked two places where we stop using
the pid value.  The very obvious one is in do_tty_hangup, and the one
the less obvious one in __proc_set_tty.

When looking into the code __proc_set_tty only has pids that need to
be put because of failures of other parts of the code to properly
perform hangup processing.   Fixing the leak here in __proc_set_tty
is easy and obviously correct so I am doing that first.

Fixing the places that should be performing hangup processing is much
less obviously correct.  So those I'm aiming those patches at -mm.
for now, so the can age a while before they are merged.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/char/tty_io.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index e453268..7a32df5 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1376,6 +1376,8 @@ static void do_tty_hangup(struct work_struct *work)
 	read_unlock(&tasklist_lock);
 
 	tty->flags = 0;
+	put_pid(tty->session);
+	put_pid(tty->pgrp);
 	tty->session = NULL;
 	tty->pgrp = NULL;
 	tty->ctrl_status = 0;
@@ -3841,6 +3843,9 @@ static struct pid *__proc_set_tty(struct task_struct *tsk, struct tty_struct *tt
 {
 	struct pid *old_pgrp;
 	if (tty) {
+		/* We should not have a session or pgrp to here but.... */
+		put_pid(tty->session);
+		put_pid(tty->pgrp);
 		tty->session = get_pid(task_session(tsk));
 		tty->pgrp = get_pid(task_pgrp(tsk));
 	}
-- 
1.5.0.g53756

[PATCH 0/4] tty: small fixes and cleanups.

[Eric W. Biederman]

Andrew the following tty fixes turned up while I was reviewing the
code to see if I was fixing the pid leaks properly.  I don't think
they are critical so at this point so they are probably not 2.6.21
material.

The do appear to be changes worth making though.

Eric

[PATCH 1/4] tty: Remove unnecessary export of proc_clear_tty

[Eric W. Biederman]

All of the users of proc_clear_tty are compiled into
the kernel so exporting this symbol appears gratuitous.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/char/tty_io.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 7a32df5..583730f 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -3837,7 +3837,6 @@ void proc_clear_tty(struct task_struct *p)
 	p->signal->tty = NULL;
 	spin_unlock_irq(&p->sighand->siglock);
 }
-EXPORT_SYMBOL(proc_clear_tty);
 
 static struct pid *__proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
 {
-- 
1.5.0.g53756

[PATCH 2/4] tty: Simplify calling of put_pid.

[Eric W. Biederman]

This patch should contain no functional changes.

At somepoint I got confused and thought put_pid could not
be called while a spin lock was held.  While it may be nice
to avoid that to reduce lock hold times put_pid can be safely
called while we hold a spin lock.

This patch removes all of the complications from the code
introduced by my misunderstanding, making the code a little
more readable.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/char/tty_io.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 583730f..01fcfaf 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -155,8 +155,7 @@ int tty_ioctl(struct inode * inode, struct file * file,
 	      unsigned int cmd, unsigned long arg);
 static int tty_fasync(int fd, struct file * filp, int on);
 static void release_tty(struct tty_struct *tty, int idx);
-static struct pid *__proc_set_tty(struct task_struct *tsk,
-				struct tty_struct *tty);
+static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
 
 /**
  *	alloc_tty_struct	-	allocate a tty object
@@ -1534,10 +1533,9 @@ void disassociate_ctty(int on_exit)
 	}
 
 	spin_lock_irq(&current->sighand->siglock);
-	tty_pgrp = current->signal->tty_old_pgrp;
+	put_pid(current->signal->tty_old_pgrp);
 	current->signal->tty_old_pgrp = NULL;
 	spin_unlock_irq(&current->sighand->siglock);
-	put_pid(tty_pgrp);
 
 	mutex_lock(&tty_mutex);
 	/* It is possible that do_tty_hangup has free'd this tty */
@@ -2508,7 +2506,6 @@ static int tty_open(struct inode * inode, struct file * filp)
 	int index;
 	dev_t device = inode->i_rdev;
 	unsigned short saved_flags = filp->f_flags;
-	struct pid *old_pgrp;
 
 	nonseekable_open(inode, filp);
 	
@@ -2602,17 +2599,15 @@ got_driver:
 		goto retry_open;
 	}
 
-	old_pgrp = NULL;
 	mutex_lock(&tty_mutex);
 	spin_lock_irq(&current->sighand->siglock);
 	if (!noctty &&
 	    current->signal->leader &&
 	    !current->signal->tty &&
 	    tty->session == NULL)
-		old_pgrp = __proc_set_tty(current, tty);
+		__proc_set_tty(current, tty);
 	spin_unlock_irq(&current->sighand->siglock);
 	mutex_unlock(&tty_mutex);
-	put_pid(old_pgrp);
 	return 0;
 }
 
@@ -3838,9 +3833,8 @@ void proc_clear_tty(struct task_struct *p)
 	spin_unlock_irq(&p->sighand->siglock);
 }
 
-static struct pid *__proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
+static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
 {
-	struct pid *old_pgrp;
 	if (tty) {
 		/* We should not have a session or pgrp to here but.... */
 		put_pid(tty->session);
@@ -3848,21 +3842,16 @@ static struct pid *__proc_set_tty(struct task_struct *tsk, struct tty_struct *tt
 		tty->session = get_pid(task_session(tsk));
 		tty->pgrp = get_pid(task_pgrp(tsk));
 	}
-	old_pgrp = tsk->signal->tty_old_pgrp;
+	put_pid(tsk->signal->tty_old_pgrp);
 	tsk->signal->tty = tty;
 	tsk->signal->tty_old_pgrp = NULL;
-	return old_pgrp;
 }
 
 void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
 {
-	struct pid *old_pgrp;
-
 	spin_lock_irq(&tsk->sighand->siglock);
-	old_pgrp = __proc_set_tty(tsk, tty);
+	__proc_set_tty(tsk, tty);
 	spin_unlock_irq(&tsk->sighand->siglock);
-
-	put_pid(old_pgrp);
 }
 
 struct tty_struct *get_current_tty(void)
-- 
1.5.0.g53756

[PATCH 3/4] tty: Introduce no_tty and use it in selinux

[Eric W. Biederman]

While researching the tty layer pid leaks I found a weird case in
selinux when we drop a controlling tty because of inadequate
permissions we don't do the normal hangup processing.  Which is a
problem if it happens the session  leader has exec'd something that
can no longer access the tty.

We already have code in the kernel to handle this case in the form of
the TIOCNOTTY ioctl.  So this patch factors out a helper function that
is the essence of that ioctl and calls it from the selinux code.

This removes the inconsistency in handling dropping of a controlling
tty and who knows it might even make some part of user space happy
because it received a SIGHUP it was expecting.

In addition since this removes the last user of proc_set_tty outside
of tty_io.c proc_set_tty is made static and removed from tty.h

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/char/tty_io.c    |   19 +++++++++++++++----
 include/linux/tty.h      |    2 +-
 security/selinux/hooks.c |    7 +++----
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 01fcfaf..0843fcb 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -156,6 +156,7 @@ int tty_ioctl(struct inode * inode, struct file * file,
 static int tty_fasync(int fd, struct file * filp, int on);
 static void release_tty(struct tty_struct *tty, int idx);
 static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
+static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
 
 /**
  *	alloc_tty_struct	-	allocate a tty object
@@ -1560,6 +1561,18 @@ void disassociate_ctty(int on_exit)
 	unlock_kernel();
 }
 
+/**
+ *
+ *	no_tty	- Ensure the current process does not have a controlling tty
+ */
+void no_tty(void)
+{
+	struct task_struct *tsk = current;
+	if (tsk->signal->leader)
+		disassociate_ctty(0);
+	proc_clear_tty(tsk);
+}
+
 
 /**
  *	stop_tty	-	propogate flow control
@@ -3282,9 +3295,7 @@ int tty_ioctl(struct inode * inode, struct file * file,
 		case TIOCNOTTY:
 			if (current->signal->tty != tty)
 				return -ENOTTY;
-			if (current->signal->leader)
-				disassociate_ctty(0);
-			proc_clear_tty(current);
+			no_tty();
 			return 0;
 		case TIOCSCTTY:
 			return tiocsctty(tty, arg);
@@ -3847,7 +3858,7 @@ static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
 	tsk->signal->tty_old_pgrp = NULL;
 }
 
-void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
+static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty)
 {
 	spin_lock_irq(&tsk->sighand->siglock);
 	__proc_set_tty(tsk, tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index dee72b9..bb45760 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -313,6 +313,7 @@ extern int tty_hung_up_p(struct file * filp);
 extern void do_SAK(struct tty_struct *tty);
 extern void __do_SAK(struct tty_struct *tty);
 extern void disassociate_ctty(int priv);
+extern void no_tty(void);
 extern void tty_flip_buffer_push(struct tty_struct *tty);
 extern speed_t tty_get_baud_rate(struct tty_struct *tty);
 extern speed_t tty_termios_baud_rate(struct ktermios *termios);
@@ -333,7 +334,6 @@ extern int tty_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 
 extern dev_t tty_devnum(struct tty_struct *tty);
 extern void proc_clear_tty(struct task_struct *p);
-extern void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
 extern struct tty_struct *get_current_tty(void);
 
 extern struct mutex tty_mutex;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 19a385e..90da0e2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1758,12 +1758,11 @@ static inline void flush_unauthorized_files(struct files_struct * files)
 			}
 		}
 		file_list_unlock();
-
-		/* Reset controlling tty. */
-		if (drop_tty)
-			proc_set_tty(current, NULL);
 	}
 	mutex_unlock(&tty_mutex);
+	/* Reset controlling tty. */
+	if (drop_tty)
+		no_tty();
 
 	/* Revalidate access to inherited open files. */
 
-- 
1.5.0.g53756

[PATCH 4/4] tty: In tiocsctty when we steal a tty hang it up.

[Eric W. Biederman]

The linux implementation of TIOCSCTTY includes an extention
that provides for making a tty that is already the controlling
tty of another session our controlling tty.  To do this it must
steal the tty away from the previous group that used it as a
controlling tty.

The way we are currently stealing the controlling tty away is
weird.   In general there is a well defined process for loosing
the controlling tty.  TTY hangup processing.  However the linux
extension that steals the tty from another process group does
not invoke that.  And instead just forgets the controlling
tty of the processes that were part of the session.

I can not imagine how our current behaviour of stealling a tty
is correct or how a set of processes could deal with it reasonably.
So this patch modifies the tty stealing to call tty_vhangup
so we get full hanup processing when we steal a tty.

I did a quick survey of user space the only application I found using
this extended behaviour is sysvinit.  I perform a limited  amount of
testing and nothing appears to have broken with this change.  And I
did see sysvinit pass through this piece of code.

In terms of the tty leaks that started this patch series this fix
should remove the last path into proc_clear_tty that has a session
or a process group.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/char/tty_io.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 0843fcb..0f5a781 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -2982,9 +2982,7 @@ static int tiocsctty(struct tty_struct *tty, int arg)
 			/*
 			 * Steal it away
 			 */
-			read_lock(&tasklist_lock);
-			session_clear_tty(tty->session);
-			read_unlock(&tasklist_lock);
+			tty_vhangup(tty);
 		} else {
 			ret = -EPERM;
 			goto unlock;
-- 
1.5.0.g53756

Re: [PATCH 3/4] tty: Introduce no_tty and use it in selinux

[Alan Cox]

On Sun, 18 Mar 2007 13:08:34 -0600
ebiederm@xmission.com (Eric W. Biederman) wrote:

> 
> While researching the tty layer pid leaks I found a weird case in
> selinux when we drop a controlling tty because of inadequate
> permissions we don't do the normal hangup processing.  Which is a
> problem if it happens the session  leader has exec'd something that
> can no longer access the tty.

Acked-by: Alan Cox <alan@redhat.com>