protect /sbin/init from unwanted signals more

PATCH In review Oleg Nesterov 3 participants 13 messages Nov 18, 2008 – Dec 4, 2008

Share & tools

Oleg Nesterov

author

2008-11-18 16:58 UTC #0 lore raw

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

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 <[email protected]>

kernel/signal.c renamed from kernel/signal.c~1_INIT_IGN_KILL +7 -1

@@ -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)

Roland McGrath

2008-11-19 18:52 UTC #1 lore raw

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?

kernel/signal.c +9 -0

@@ -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

Eric W. Biederman

2008-11-20 02:05 UTC #2 lore raw

Quoted 22 lines · 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?

kernel/signal.c +9 -0

@@ -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;

Quoted 5 lines

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

Roland McGrath

2008-11-20 03:05 UTC #3 lore raw

Quoted 2 lines

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.

Quoted 4 lines

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

Oleg Nesterov

author

2008-11-20 13:52 UTC #4 lore raw

In reply to Eric W. Biederman

Quoted 10 lines · Eric W. Biederman

Quoted 5 lines · Roland McGrath
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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Oleg Nesterov

author

2008-11-20 14:20 UTC #11 lore raw

In reply to Roland McGrath

Quoted 2 lines · Roland McGrath

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.

Quoted 2 lines

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.

kernel/signal.c static int sig_ignored(struct task_struct *t, int sig) · line 79

Quoted 8 lines

What about just this instead?

kernel/signal.c +4 -0

+	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.

Quoted 1 line

For consistency, change tracehook_consider_fatal_signal to match.

Yes, will do.

Oleg.

Eric W. Biederman

2008-11-20 18:16 UTC #5 lore raw

In reply to Oleg Nesterov

Quoted 10 lines · Oleg Nesterov

Quoted 9 lines · Eric W. Biederman
Quoted 5 lines · Roland McGrath
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.)

Quoted 5 lines

Quoted 1 line
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.

Quoted 10 lines

Quoted 1 line
My original analysis of the situation was that we should not send blocked
signals.
Quoted 5 lines
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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 10 lines

Yes, this perhaps means init is not perfect, but still.
Quoted 1 line
Which reminds me. I need to retest, but I had a case where I had a trivial
init
Quoted 3 lines
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

Oleg Nesterov

author

2008-11-20 19:00 UTC #6 lore raw

Quoted 13 lines · Eric W. Biederman

Quoted 10 lines · Oleg Nesterov
Quoted 9 lines · Eric W. Biederman
Quoted 5 lines · Roland McGrath
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.)

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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.

Quoted 11 lines

Quoted 8 lines
Quoted 1 line
Which reminds me. I need to retest, but I had a case where I had a trivial
init
Quoted 3 lines
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.

Oleg Nesterov

author

2008-11-20 19:28 UTC #7 lore raw

PATCH 1/1 processes: reparent_thread: don't call kill_orphaned_pgrp() if task_detached()

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 <[email protected]>

kernel/exit.c renamed from kernel/exit.c~1_IGNORE_DETACHED +4 -4

@@ -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);

Oleg Nesterov

author

2008-11-20 20:23 UTC #12 lore raw

In reply to Oleg Nesterov

Quoted 11 lines · Oleg Nesterov

Quoted 2 lines · Roland McGrath
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.
Quoted 2 lines
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)

Quoted 3 lines

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.

Roland McGrath

2008-12-04 00:54 UTC #8 lore raw

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

In reply to Oleg Nesterov

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

kernel/exit.c static void reparent_thread(struct task_ · line 820

Quoted 6 lines

kernel/exit.c renamed from kernel/exit.c~1_IGNORE_DETACHED +2 -0

@@ -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

Roland McGrath

2008-12-04 01:08 UTC #10 lore raw

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

In reply to Oleg Nesterov

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

kernel/exit.c static void reparent_thread(struct task_ · line 820

Quoted 6 lines

kernel/exit.c renamed from kernel/exit.c~1_IGNORE_DETACHED +2 -0

@@ -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

Oleg Nesterov

author

2008-12-04 17:16 UTC #9 lore raw

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

In reply to Roland McGrath

Quoted 17 lines · Roland McGrath

kernel/exit.c static void reparent_thread(struct task_ · line 820
Quoted 7 lines
kernel/exit.c renamed from kernel/exit.c~1_IGNORE_DETACHED +2 -0
@@ -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);

Quoted 5 lines

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.