Messages in this thread Patch in this message | | | Date | Thu, 26 Feb 2009 22:59:46 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction |
| |
On 02/25, Oleg Nesterov wrote: > > On 02/25, Linus Torvalds wrote: > > > > > As I think I said before, I don't really know what the actual use case is > > > for CLONE_PARENT without CLONE_THREAD. So it's easy to approve changing > > > its behavior, but I do vaguely worry about who expected what behavior before. > > > > I think changing it is wrong. > > Perhaps. As I said, I don't know what is the expected behaviour. And in fact > I can't think of the "obviously good" behaviour. > > > I can easily see somebody using CLONE_PARENT to get the correct getppid > > semantics in the thread, and then setting the signal to zero to not make > > the parent see the thread go away. > > ->exit_signal == 0 doesn't mean the thread silently goes away, it becomes > a zombie (even if ->parent ignores SIGCHLD). We don't send the signal, but > that is all. > > And if ->parent execs, we reset ->exit_signal to SIGCHLD anyway.
Really, how CLONE_PARENT + exit_signal==0 can be useful?
I still think this patch does the right change. Not because it fixes the security problem, as I said I do not think the ability to send SIGKILL to ->parent is wrong from the security pov, even if child does setuid/etc, the problem is that CLONE_PARENT can fool ->xxx_exec_id logic and we can send SIGKILL after parent/child exec.
But because the current behaviour is just silly. Imho.
But of course, if this change can break the user-space applications, then it should not be applied.
> > And quite > > frankly, it would be good to try to see if there are other alternatives. > > Agreed. I thought about checking ->xxx_exec_id's in copy_process(), > but doesn't look very nice...
I meant something like the patch below. But I don't like it.
Anybody has other ideas?
Oleg.
--- kernel/fork.c +++ kernel/fork.c @@ -1218,9 +1218,15 @@ static struct task_struct *copy_process( set_task_cpu(p, smp_processor_id()); /* CLONE_PARENT re-uses the old parent */ - if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) + if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) { p->real_parent = current->real_parent; - else + p->self_exec_id = p->parent_exec_id = + p->real_parent->self_exec_id; + + if (current->parent_exec_id != current->real_parent->self_exec_id || + current->self_exec_id != current->parent_exec_id) + p->exit_signal = SIGCHLD; + } else p->real_parent = current; spin_lock(¤t->sighand->siglock);
| |