[PATCH] move tty_kref_put() outside of __cleanup_signal()

[PATCH] move tty_kref_put() outside of __cleanup_signal()

[Oleg Nesterov]

tty_kref_put() have two callsites in copy_process() pathes,

	1. if copy_process() suceeds it is called before we copy
	   signal->tty from parent

	2. otherwise it is called from __cleanup_signal() under
	   bad_fork_cleanup_signal: label

In both cases tty_kref_put() is not right and unneeded because we don't
have the balancing tty_kref_get(). Fortunately, this is harmless because
this can only happen without CLONE_THREAD, and in this case signal->tty
must be NULL.

Remove tty_kref_put() from copy_process() and __cleanup_signal(), and
change another caller of __cleanup_signal(), __exit_signal(), to call
tty_kref_put() by hand.

I hope this change makes sense by itself, but it is also needed to
make ->signal refcountable.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/fork.c |    2 --
 kernel/exit.c |    1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

--- 34-rc1/kernel/fork.c~7_TTY_PUT	2010-03-17 16:04:22.000000000 +0100
+++ 34-rc1/kernel/fork.c	2010-03-18 22:45:14.000000000 +0100
@@ -896,7 +896,6 @@ static int copy_signal(unsigned long clo
 void __cleanup_signal(struct signal_struct *sig)
 {
 	thread_group_cputime_free(sig);
-	tty_kref_put(sig->tty);
 	kmem_cache_free(signal_cachep, sig);
 }
 
@@ -1264,7 +1263,6 @@ static struct task_struct *copy_process(
 				p->nsproxy->pid_ns->child_reaper = p;
 
 			p->signal->leader_pid = pid;
-			tty_kref_put(p->signal->tty);
 			p->signal->tty = tty_kref_get(current->signal->tty);
 			attach_pid(p, PIDTYPE_PGID, task_pgrp(current));
 			attach_pid(p, PIDTYPE_SID, task_session(current));
--- 34-rc1/kernel/exit.c~7_TTY_PUT	2010-03-17 20:05:38.000000000 +0100
+++ 34-rc1/kernel/exit.c	2010-03-18 22:46:41.000000000 +0100
@@ -150,6 +150,7 @@ static void __exit_signal(struct task_st
 		 * see account_group_exec_runtime().
 		 */
 		task_rq_unlock_wait(tsk);
+		tty_kref_put(sig->tty);
 		__cleanup_signal(sig);
 	}
 }

Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()

[Alan Cox]

> -			tty_kref_put(p->signal->tty);
>  			p->signal->tty = tty_kref_get(current->signal->tty);

That bit needs commenting clearly or a WARN_ON() that p->signal->tty is
NULL before the get otherwise when the assumption is broken the flaw will
be subtle and hard to find.

>  			attach_pid(p, PIDTYPE_PGID, task_pgrp(current));
>  			attach_pid(p, PIDTYPE_SID, task_session(current));
> --- 34-rc1/kernel/exit.c~7_TTY_PUT	2010-03-17 20:05:38.000000000 +0100
> +++ 34-rc1/kernel/exit.c	2010-03-18 22:46:41.000000000 +0100
> @@ -150,6 +150,7 @@ static void __exit_signal(struct task_st
>  		 * see account_group_exec_runtime().
>  		 */
>  		task_rq_unlock_wait(tsk);
> +		tty_kref_put(sig->tty);

and a sig->tty = NULL assignment to trap races might not go amiss here
perhaps ?

Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()

[Oleg Nesterov]

On 03/18, Alan Cox wrote:
>
> > -			tty_kref_put(p->signal->tty);
> >  			p->signal->tty = tty_kref_get(current->signal->tty);
>
> That bit needs commenting clearly or a WARN_ON() that p->signal->tty is
> NULL before the get otherwise when the assumption is broken the flaw will
> be subtle and hard to find.

Well. It must be NULL. This tty_kref_get() was added by
9c9f4ded90a59eee84e15f5fd38c03d60184e112, and the same commit also added
sig->tty = NULL into copy_signal(). With the recent changes in copy_signal()
it must be NULL due to zalloc().

But this doesn't matter. We should not do tty_kref_put() even if it is not
NULL. If we do "put", we need the balancing "get" which we do not have.
That is why this "get" is not only unneeded, but wrong imho.

Probably the little comment makes sense, but I don't really understand what
it should explain. To me, this is like list_add_tail(&init_task.tasks) few
lines below. We do not have the comment to explain the "missing" list_del().
This task is new, nobody can see/use it before we drop the locks. NULL or not,
its signal->tty is just uninitialized yet.

> > --- 34-rc1/kernel/exit.c~7_TTY_PUT	2010-03-17 20:05:38.000000000 +0100
> > +++ 34-rc1/kernel/exit.c	2010-03-18 22:46:41.000000000 +0100
> > @@ -150,6 +150,7 @@ static void __exit_signal(struct task_st
> >  		 * see account_group_exec_runtime().
> >  		 */
> >  		task_rq_unlock_wait(tsk);
> > +		tty_kref_put(sig->tty);
>
> and a sig->tty = NULL assignment to trap races might not go amiss here
> perhaps ?

Indeed ;)

The subsequent patches will do this, we need more changes anyway. Currently
this doesn't matter because we are going to kfree() this memory unconditionally.
But when we pin ->signal to task_struct, we should clear ->signal->tty before
we drop ->siglock, then tty_kref_put().

Oleg.

Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()

[Alan Cox]

> This task is new, nobody can see/use it before we drop the locks. NULL or not,
> its signal->tty is just uninitialized yet.

Fair enough.

> 
> > > --- 34-rc1/kernel/exit.c~7_TTY_PUT	2010-03-17 20:05:38.000000000 +0100
> > > +++ 34-rc1/kernel/exit.c	2010-03-18 22:46:41.000000000 +0100
> > > @@ -150,6 +150,7 @@ static void __exit_signal(struct task_st
> > >  		 * see account_group_exec_runtime().
> > >  		 */
> > >  		task_rq_unlock_wait(tsk);
> > > +		tty_kref_put(sig->tty);
> >
> > and a sig->tty = NULL assignment to trap races might not go amiss here
> > perhaps ?
> 
> Indeed ;)
> 
> The subsequent patches will do this, we need more changes anyway. Currently
> this doesn't matter because we are going to kfree() this memory unconditionally.
> But when we pin ->signal to task_struct, we should clear ->signal->tty before
> we drop ->siglock, then tty_kref_put().

Ok - yes the moment you start refcounting ->signal that changes (or do you
expect to free ->tty when you destruct the signals ?)

Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()

[Oleg Nesterov]

On 03/19, Alan Cox wrote:
>
> > > > --- 34-rc1/kernel/exit.c~7_TTY_PUT	2010-03-17 20:05:38.000000000 +0100
> > > > +++ 34-rc1/kernel/exit.c	2010-03-18 22:46:41.000000000 +0100
> > > > @@ -150,6 +150,7 @@ static void __exit_signal(struct task_st
> > > >  		 * see account_group_exec_runtime().
> > > >  		 */
> > > >  		task_rq_unlock_wait(tsk);
> > > > +		tty_kref_put(sig->tty);
> > >
> > > and a sig->tty = NULL assignment to trap races might not go amiss here
> > > perhaps ?
> >
> > Indeed ;)
> >
> > The subsequent patches will do this, we need more changes anyway. Currently
> > this doesn't matter because we are going to kfree() this memory unconditionally.
> > But when we pin ->signal to task_struct, we should clear ->signal->tty before
> > we drop ->siglock, then tty_kref_put().
>
> Ok - yes the moment you start refcounting ->signal that changes (or do you
> expect to free ->tty when you destruct the signals ?)

I think signal->tty should be freed (and nullified under ->siglock) when the
last thread exits.

The goal is to make ->signal immutable, so that it would be always safe to
dereference task->signal if you have a reference to task_struct. But I don't
think get_task_struct() should defer tty_kref_put(), and besides put_task_struct()
must be safe in any context.

See also http://marc.info/?l=linux-kernel&m=126885423426183

Oleg.

Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>


Thanks,
Roland