[PATCH 1/2] protect /sbin/init from unwanted signals more

[PATCH 1/2] protect /sbin/init from unwanted signals more

[Oleg Nesterov]

init ignores the SIG_DFL signals but we queue them anyway, including
SIGKILL. This is mostly OK, the signal will be dropped silently when
dequeued, but the pending SIGKILL has 2 bad implications:

	- it implies fatal_signal_pending(), so we confuse things
	  like wait_for_completion_killable/lock_page_killable.

	- for the sub-namespace inits, the pending SIGKILL can
	  mask (legacy_queue) the subsequent SIGKILL from the
	  parent namespace which must kill cinit reliably.
	  (preparation, cinits don't have SIGNAL_UNKILLABLE yet)

The patch can't help when init is ptraced, but ptracing of init is
not "safe" anyway.

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

--- K-IS/kernel/signal.c~1_INIT_IGN_KILL	2008-11-10 19:21:17.000000000 +0100
+++ K-IS/kernel/signal.c	2008-11-17 19:54:09.000000000 +0100
@@ -43,7 +43,13 @@ static struct kmem_cache *sigqueue_cache
 
 static void __user *sig_handler(struct task_struct *t, int sig)
 {
-	return t->sighand->action[sig - 1].sa.sa_handler;
+	void __user *h = t->sighand->action[sig - 1].sa.sa_handler;
+
+	/* drop SIGKILL early to not confuse wait_xxx_killable/etc */
+	if (unlikely(t->signal->flags & SIGNAL_UNKILLABLE) && h == SIG_DFL)
+		h = SIG_IGN;
+
+	return h;
 }
 
 static int sig_handler_ignored(void __user *handler, int sig)

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Roland McGrath]

The effect is fine, but that seems like a kludgey way to do it.
I really don't think the sigaction case matters--certainly it will never
come up with SIGKILL.  What about just this instead?

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -66,6 +66,15 @@ static int sig_ignored(struct task_struct *t, int sig)
 		return 0;
 
 	handler = sig_handler(t, sig);
+
+	/*
+	 * For init, short-circuit any signal without a handler.
+	 * We won't allow them to be delivered, so don't even queue them.
+	 */
+	if (unlikely(signal->flags & SIGNAL_UNKILLABLE) &&
+	    (handler == SIG_IGN || handler == SIG_DFL))
+		return 1;
+
 	if (!sig_handler_ignored(handler, sig))
 		return 0;
 
With that, I wonder if the SIGNAL_UNKILLABLE checks in get_signal_to_deliver
and complete_signal are needed at all.  Hmm, I guess we do because this
doesn't affect blocked signals, so they might be unblocked and delivered.
(Note that since it doesn't affect blocked signals, this doesn't break init
using sigwait if it wanted to.)


Thanks,
Roland

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Eric W. Biederman]

Roland McGrath <roland@redhat.com> writes:

> The effect is fine, but that seems like a kludgey way to do it.
> I really don't think the sigaction case matters--certainly it will never
> come up with SIGKILL.  What about just this instead?
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -66,6 +66,15 @@ static int sig_ignored(struct task_struct *t, int sig)
>  		return 0;
>  
>  	handler = sig_handler(t, sig);
> +
> +	/*
> +	 * For init, short-circuit any signal without a handler.
> +	 * We won't allow them to be delivered, so don't even queue them.
> +	 */
> +	if (unlikely(signal->flags & SIGNAL_UNKILLABLE) &&
> +	    (handler == SIG_IGN || handler == SIG_DFL))
> +		return 1;
> +
>  	if (!sig_handler_ignored(handler, sig))
>  		return 0;
>  
> With that, I wonder if the SIGNAL_UNKILLABLE checks in get_signal_to_deliver
> and complete_signal are needed at all.  Hmm, I guess we do because this
> doesn't affect blocked signals, so they might be unblocked and delivered.
> (Note that since it doesn't affect blocked signals, this doesn't break init
> using sigwait if it wanted to.)

Ah.  That answers the question I had bouncing in the back of my head.

My original analysis of the situation was that we should not send blocked signals.
Treating handler != SIG_DFL as a permission check.  Not as an optimization.

Mostly because it is more consistent and uniform.

inits today don't do anything with blocked signals.  They explicitly ignore all signals,
they don't want to deal with an enable those they do.

Which I guess means in practice only SIGKILL and SIGSTOP are especially interesting,
and we can't block those so worrying about blocked signals is no big deal.

Which reminds me.  I need to retest, but I had a case where I had a trivial init
that set all signal handlers to SIG_IGN so it could ignore SIGCHLD.  And not
all of it's children were getting reaped automagically.  Do we have a bug in
the reparenting/reaping logic?

Eric

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Roland McGrath]

> inits today don't do anything with blocked signals.  They explicitly ignore all signals,
> they don't want to deal with an enable those they do.

I don't think we should constrain them this way.  
Future init versions might want to use blocked signals and sigwait.

> Which reminds me.  I need to retest, but I had a case where I had a
> trivial init that set all signal handlers to SIG_IGN so it could ignore
> SIGCHLD.  And not all of it's children were getting reaped automagically.
> Do we have a bug in the reparenting/reaping logic?

We have had bugs along those lines in the past.
AFAIK we've licked them all now.  Re-testing is in order.


Thanks,
Roland

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Oleg Nesterov]

On 11/19, Eric W. Biederman wrote:
>
> Roland McGrath <roland@redhat.com> writes:
>
> > With that, I wonder if the SIGNAL_UNKILLABLE checks in get_signal_to_deliver
> > and complete_signal are needed at all.  Hmm, I guess we do because this
> > doesn't affect blocked signals, so they might be unblocked and delivered.
> > (Note that since it doesn't affect blocked signals, this doesn't break init
> > using sigwait if it wanted to.)
>
> Ah.  That answers the question I had bouncing in the back of my head.

Even worse. The signal can be dequeued even before unblocked by the target.
complete_signal() can "redirect" this signal to another thread wich doesn't
block it.

> My original analysis of the situation was that we should not send blocked signals.
> Treating handler != SIG_DFL as a permission check.  Not as an optimization.
>
> Mostly because it is more consistent and uniform.
>
> inits today don't do anything with blocked signals.

(I guess you mean "with blocked SIG_DFL signals", otherwise this is
 too strong ;)

If init does exec and do not want to miss (say) SIGCHLD, the only option
is to block it before exec. And right after exec the handler is SIG_DFL.

> They explicitly ignore all signals,
> they don't want to deal with an enable those they do.

I do remember I had the (unrelated) bugreport which in particular showed
that user-space sends SIGUSR1 to init. Usually init has a handler and does
something in responce, but sometimes the handler is SIG_DFL. I don't
remember the distribution, ubuntu iirc.

Yes, this perhaps means init is not perfect, but still.

> Which reminds me.  I need to retest, but I had a case where I had a trivial init
> that set all signal handlers to SIG_IGN so it could ignore SIGCHLD.  And not
> all of it's children were getting reaped automagically.  Do we have a bug in
> the reparenting/reaping logic?

Ah... I thought this was already fixed... shouldn't reparent_thread()
check task_detached() after do_notify() ? like ptrace_exit() does.

Oleg.

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 11/19, Eric W. Biederman wrote:
>>
>> Roland McGrath <roland@redhat.com> writes:
>>
>> > With that, I wonder if the SIGNAL_UNKILLABLE checks in get_signal_to_deliver
>> > and complete_signal are needed at all.  Hmm, I guess we do because this
>> > doesn't affect blocked signals, so they might be unblocked and delivered.
>> > (Note that since it doesn't affect blocked signals, this doesn't break init
>> > using sigwait if it wanted to.)
>>
>> Ah.  That answers the question I had bouncing in the back of my head.
>
> Even worse. The signal can be dequeued even before unblocked by the target.
> complete_signal() can "redirect" this signal to another thread wich doesn't
> block it.

The signal handlers should still be the same.

>> My original analysis of the situation was that we should not send blocked
> signals.
>> Treating handler != SIG_DFL as a permission check.  Not as an optimization.
>>
>> Mostly because it is more consistent and uniform.
>>
>> inits today don't do anything with blocked signals.
>
> (I guess you mean "with blocked SIG_DFL signals", otherwise this is
>  too strong ;)

Could be.

> If init does exec and do not want to miss (say) SIGCHLD, the only option
> is to block it before exec. And right after exec the handler is SIG_DFL.

Interesting point.

>> They explicitly ignore all signals,
>> they don't want to deal with an enable those they do.
>
> I do remember I had the (unrelated) bugreport which in particular showed
> that user-space sends SIGUSR1 to init. Usually init has a handler and does
> something in responce, but sometimes the handler is SIG_DFL. I don't
> remember the distribution, ubuntu iirc.

Could be.  I have to follow up on what craziness upstart is doing.
So my information is a bit dated.

> Yes, this perhaps means init is not perfect, but still.
>
>> Which reminds me.  I need to retest, but I had a case where I had a trivial
> init
>> that set all signal handlers to SIG_IGN so it could ignore SIGCHLD.  And not
>> all of it's children were getting reaped automagically.  Do we have a bug in
>> the reparenting/reaping logic?
>
> Ah... I thought this was already fixed... shouldn't reparent_thread()
> check task_detached() after do_notify() ? like ptrace_exit() does.

Like I said I need to retest.  I was on a 2.6.26 fedora kernel base.
So if there have been recent bug fixes things may have changed.

Eric

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Oleg Nesterov]

On 11/20, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 11/19, Eric W. Biederman wrote:
> >>
> >> Roland McGrath <roland@redhat.com> writes:
> >>
> >> > With that, I wonder if the SIGNAL_UNKILLABLE checks in get_signal_to_deliver
> >> > and complete_signal are needed at all.  Hmm, I guess we do because this
> >> > doesn't affect blocked signals, so they might be unblocked and delivered.
> >> > (Note that since it doesn't affect blocked signals, this doesn't break init
> >> > using sigwait if it wanted to.)
> >>
> >> Ah.  That answers the question I had bouncing in the back of my head.
> >
> > Even worse. The signal can be dequeued even before unblocked by the target.
> > complete_signal() can "redirect" this signal to another thread wich doesn't
> > block it.
>
> The signal handlers should still be the same.

Yes sure. and the handler can be SIG_DFL.

In short, if we have any reason (we do have) to check sig_kernel_ignore()
in get_signal_to_deliver(), then we have the same reason to check
SIGNAL_UNKILLABLE too.

> >> Which reminds me.  I need to retest, but I had a case where I had a trivial
> > init
> >> that set all signal handlers to SIG_IGN so it could ignore SIGCHLD.  And not
> >> all of it's children were getting reaped automagically.  Do we have a bug in
> >> the reparenting/reaping logic?
> >
> > Ah... I thought this was already fixed... shouldn't reparent_thread()
> > check task_detached() after do_notify() ? like ptrace_exit() does.
>
> Like I said I need to retest.  I was on a 2.6.26 fedora kernel base.
> So if there have been recent bug fixes things may have changed.

I bet the bug is still here.

The fix should be simple, except we can't trust ptrace_reparented().

I'll send the patch, but first I'd like to do another one. Since
I stopped my attempts to understand the orphaned pgrps a long
ago, I hope you can review ;)

Oleg.

[PATCH] processes: reparent_thread: don't call kill_orphaned_pgrp() if task_detached()

[Oleg Nesterov]

Needs an ack from someone who understands orphaned groups.

If task_detached(p) == T, then either

	a) p is not the main thread, we will find the group leader
	   on the ->children list.
or
	b) p is the group leader but its ->exit_state = EXIT_DEAD.
	   This can only happen when the last sub-thread has died,
	   but in that case that thread has already called
	   kill_orphaned_pgrp() from exit_notify().

In both cases kill_orphaned_pgrp() looks bogus.

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

--- K-28/kernel/exit.c~1_IGNORE_DETACHED	2008-11-17 02:02:12.000000000 +0100
+++ K-28/kernel/exit.c	2008-11-20 20:21:02.000000000 +0100
@@ -816,6 +816,8 @@ static void reparent_thread(struct task_
 
 	list_move_tail(&p->sibling, &p->real_parent->children);
 
+	if (task_detached(p))
+		return;
 	/* If this is a threaded reparent there is no need to
 	 * notify anyone anything has happened.
 	 */
@@ -823,15 +825,13 @@ static void reparent_thread(struct task_
 		return;
 
 	/* We don't want people slaying init.  */
-	if (!task_detached(p))
-		p->exit_signal = SIGCHLD;
+	p->exit_signal = SIGCHLD;
 
 	/* If we'd notified the old parent about this child's death,
 	 * also notify the new parent.
 	 */
 	if (!ptrace_reparented(p) &&
-	    p->exit_state == EXIT_ZOMBIE &&
-	    !task_detached(p) && thread_group_empty(p))
+	    p->exit_state == EXIT_ZOMBIE && thread_group_empty(p))
 		do_notify_parent(p, p->exit_signal);
 
 	kill_orphaned_pgrp(p, father);

Re: [PATCH] processes: reparent_thread: don't call kill_orphaned_pgrp() if task_detached()

[Roland McGrath]

> Needs an ack from someone who understands orphaned groups.

I understand what the semantics are supposed to be.
But I'm getting a headache trying to see if I really understand the code.

> If task_detached(p) == T, then either
> 
> 	a) p is not the main thread, we will find the group leader
> 	   on the ->children list.

Correct.

> or
> 	b) p is the group leader but its ->exit_state = EXIT_DEAD.
> 	   This can only happen when the last sub-thread has died,
> 	   but in that case that thread has already called
> 	   kill_orphaned_pgrp() from exit_notify().

I think that's right too.

> @@ -816,6 +816,8 @@ static void reparent_thread(struct task_
>  
>  	list_move_tail(&p->sibling, &p->real_parent->children);
>  
> +	if (task_detached(p))
> +		return;

Seems like it would be cleaner to reorganize the code a little.
reparent_thread has only one caller.  How about we move:

	if (p->pdeath_signal)
		/* We already hold the tasklist_lock here.  */
		group_send_sig_info(p->pdeath_signal, SEND_SIG_NOINFO, p);

	list_move_tail(&p->sibling, &p->real_parent->children);

into forget_original_parent and rename reparent_thread to something else,
called only:

	if (!task_detached(p) && !same_thread_group(p->real_parent, father))
		orphaned_process(p);


Thanks,
Roland

Re: [PATCH] processes: reparent_thread: don't call kill_orphaned_pgrp() if task_detached()

[Oleg Nesterov]

On 11/26, Roland McGrath wrote:
>
> > @@ -816,6 +816,8 @@ static void reparent_thread(struct task_
> >
> >  	list_move_tail(&p->sibling, &p->real_parent->children);
> >
> > +	if (task_detached(p))
> > +		return;
>
> Seems like it would be cleaner to reorganize the code a little.
> reparent_thread has only one caller.  How about we move:
>
> 	if (p->pdeath_signal)
> 		/* We already hold the tasklist_lock here.  */
> 		group_send_sig_info(p->pdeath_signal, SEND_SIG_NOINFO, p);
>
> 	list_move_tail(&p->sibling, &p->real_parent->children);
>
> into forget_original_parent and rename reparent_thread to something else,
> called only:
>
> 	if (!task_detached(p) && !same_thread_group(p->real_parent, father))
> 		orphaned_process(p);

Not that I think this really matters, but imho this code needs more
little trivial reorganizations.

reparent_thread() (or whatever) needs the "&ptrace_dead" parameter
too, if the new parent (init) ignores SIGCHLD we should release a
zombie. So we should rename "ptrace_dead" and ptrace_exit_finish().
And imho it makes sense to create the new helper which does
"list_for_each_entry_safe(father->children) {}", to make this code
more symmetrical() with ptrace_exit().

Oleg.

Re: [PATCH] processes: reparent_thread: don't call kill_orphaned_pgrp() if task_detached()

[Roland McGrath]

> Needs an ack from someone who understands orphaned groups.

I understand what the semantics are supposed to be.
But I'm getting a headache trying to see if I really understand the code.

> If task_detached(p) == T, then either
> 
> 	a) p is not the main thread, we will find the group leader
> 	   on the ->children list.

Correct.

> or
> 	b) p is the group leader but its ->exit_state = EXIT_DEAD.
> 	   This can only happen when the last sub-thread has died,
> 	   but in that case that thread has already called
> 	   kill_orphaned_pgrp() from exit_notify().

I think that's right too.

> @@ -816,6 +816,8 @@ static void reparent_thread(struct task_
>  
>  	list_move_tail(&p->sibling, &p->real_parent->children);
>  
> +	if (task_detached(p))
> +		return;

Seems like it would be cleaner to reorganize the code a little.
reparent_thread has only one caller.  How about we move:

	if (p->pdeath_signal)
		/* We already hold the tasklist_lock here.  */
		group_send_sig_info(p->pdeath_signal, SEND_SIG_NOINFO, p);

	list_move_tail(&p->sibling, &p->real_parent->children);

into forget_original_parent and rename reparent_thread to something else,
called only:

	if (!task_detached(p) && !same_thread_group(p->real_parent, father))
		orphaned_process(p);


Thanks,
Roland

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Oleg Nesterov]

On 11/19, Roland McGrath wrote:
>
> The effect is fine, but that seems like a kludgey way to do it.

Agreed, that is why I did the next patch to kill the ugliness.

> I really don't think the sigaction case matters--certainly it will never
> come up with SIGKILL.

Yes. This patch doesn't affect sigaction, the next one adds a very
minor side effect: init drops pending !sig_kernel_ignore() signals
if it does sigaction(SIG_IGN). But this has nothing to do with SIGKILL
of course.

> What about just this instead?
>
> +	if (unlikely(signal->flags & SIGNAL_UNKILLABLE) &&
> +	    (handler == SIG_IGN || handler == SIG_DFL))
> +		return 1;
> +
>  	if (!sig_handler_ignored(handler, sig))
>  		return 0;

Yes, this is the same, but anyway this is killed by the next patch.



> For consistency, change tracehook_consider_fatal_signal to match.

Yes, will do.

Oleg.

Re: [PATCH 1/2] protect /sbin/init from unwanted signals more

[Oleg Nesterov]

On 11/20, Oleg Nesterov wrote:
>
> On 11/19, Roland McGrath wrote:
> >
> > The effect is fine, but that seems like a kludgey way to do it.
>
> Agreed, that is why I did the next patch to kill the ugliness.
>
> > I really don't think the sigaction case matters--certainly it will never
> > come up with SIGKILL.
>
> Yes. This patch doesn't affect sigaction, the next one adds a very

(this one, not the next one)

> minor side effect: init drops pending !sig_kernel_ignore() signals
> if it does sigaction(SIG_IGN). But this has nothing to do with SIGKILL
> of course.

Ah sorry, now I see I misunderstood you...

You mean, we shouldn't touch the sigaction() path. Now I am wondering
if it is really OK to drop signals if init does sigaction(SIG_DFL),
perhaps you are right.

Oleg.