* [PATCH] move tty_kref_put() outside of __cleanup_signal()
@ 2010-03-18 22:37 Oleg Nesterov
2010-03-18 22:51 ` Alan Cox
2010-04-08 2:15 ` Roland McGrath
0 siblings, 2 replies; 6+ messages in thread
From: Oleg Nesterov @ 2010-03-18 22:37 UTC (permalink / raw)
To: Andrew Morton; +Cc: Alan Cox, Roland McGrath, linux-kernel
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);
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()
2010-03-18 22:37 [PATCH] move tty_kref_put() outside of __cleanup_signal() Oleg Nesterov
@ 2010-03-18 22:51 ` Alan Cox
2010-03-18 23:16 ` Oleg Nesterov
2010-04-08 2:15 ` Roland McGrath
1 sibling, 1 reply; 6+ messages in thread
From: Alan Cox @ 2010-03-18 22:51 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Alan Cox, Roland McGrath, linux-kernel
> - 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 ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()
2010-03-18 22:51 ` Alan Cox
@ 2010-03-18 23:16 ` Oleg Nesterov
2010-03-19 11:08 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2010-03-18 23:16 UTC (permalink / raw)
To: Alan Cox; +Cc: Andrew Morton, Alan Cox, Roland McGrath, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()
2010-03-18 23:16 ` Oleg Nesterov
@ 2010-03-19 11:08 ` Alan Cox
2010-03-19 13:09 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Alan Cox @ 2010-03-19 11:08 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Alan Cox, Roland McGrath, linux-kernel
> 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 ?)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()
2010-03-19 11:08 ` Alan Cox
@ 2010-03-19 13:09 ` Oleg Nesterov
0 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2010-03-19 13:09 UTC (permalink / raw)
To: Alan Cox; +Cc: Andrew Morton, Alan Cox, Roland McGrath, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] move tty_kref_put() outside of __cleanup_signal()
2010-03-18 22:37 [PATCH] move tty_kref_put() outside of __cleanup_signal() Oleg Nesterov
2010-03-18 22:51 ` Alan Cox
@ 2010-04-08 2:15 ` Roland McGrath
1 sibling, 0 replies; 6+ messages in thread
From: Roland McGrath @ 2010-04-08 2:15 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Alan Cox, linux-kernel
Acked-by: Roland McGrath <roland@redhat.com>
Thanks,
Roland
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-04-08 2:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-18 22:37 [PATCH] move tty_kref_put() outside of __cleanup_signal() Oleg Nesterov
2010-03-18 22:51 ` Alan Cox
2010-03-18 23:16 ` Oleg Nesterov
2010-03-19 11:08 ` Alan Cox
2010-03-19 13:09 ` Oleg Nesterov
2010-04-08 2:15 ` Roland McGrath
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.