signal: protect SIGNAL_UNKILLABLE from unintentional clearing.

This can result in init becoming stoppable/killable after tracing. For example, running:

while true; do kill -STOP 1; done &
strace -p 1

and then stopping strace and the kill loop will result in init being left in state TASK_STOPPED. Sending SIGCONT to init will resume it, but init will now respond to future SIGSTOP signals rather than ignoring them.

Quoted 6 lines · Jamie Iles

This can result in init becoming stoppable/killable after tracing. For example, running:

while true; do kill -STOP 1; done &
strace -p 1

Quoted 4 lines

and then stopping strace and the kill loop will result in init being left in state TASK_STOPPED. Sending SIGCONT to init will resume it, but init will now respond to future SIGSTOP signals rather than ignoring them.

Yes, and a lot more... I forgot about these problems again.

Jamie, sorry for delay, I'll try to read the patch and reply tomorrow.

Since 00cd5c37af (ptrace: permit ptracing of /sbin/init) we can now trace init processes. init is initially protected with SIGNAL_UNKILLABLE which will prevent fatal signals such as SIGSTOP, but there are a number of paths during tracing where SIGNAL_UNKILLABLE can be implicitly cleared.

Jamie,

I am really sorry for the huge delay.

Quoted 6 lines

Since 00cd5c37af (ptrace: permit ptracing of /sbin/init) we can now trace init processes. init is initially protected with SIGNAL_UNKILLABLE which will prevent fatal signals such as SIGSTOP, but there are a number of paths during tracing where SIGNAL_UNKILLABLE can be implicitly cleared.

Yes, old problem which nobody bothered to fix ;) Thanks for doing this. To remind, there are more problems with SIGNAL_UNKILLABLE, but this patch looks like a good start to me.

However, I would like to ask you to make V2, see below.

include/linux/sched.h struct signal_struct { · line 845

Quoted 5 lines

include/linux/sched.h +5 -0

+static inline void signal_set_flags(struct signal_struct *sig,
+				    unsigned int flags)
+{
+	sig->flags = (sig->flags & SIGNAL_PROTECTED_MASK) | flags;
+}

OK, agreed, probably the helper makes sense, but I think it is overused in this patch, please see below. In short, I'd suggest to use it only in the jctl code, at least for now.

include/linux/sched.h struct signal_struct { · line 851

Quoted 5 lines

include/linux/sched.h +5 -0

+static inline void signal_clear_flags(struct signal_struct *sig,
+				      unsigned int flags)
+{
+	sig->flags &= (~flags | SIGNAL_PROTECTED_MASK);
+}

But this one looks pointless.

kernel/exit.c do_group_exit(int exit_code) · line 925

Quoted 8 lines

kernel/exit.c +1 -1

@@ -922,7 +922,7 @@ do_group_exit(int exit_code)
 			exit_code = sig->group_exit_code;
 		else {
 			sig->group_exit_code = exit_code;
-			sig->flags = SIGNAL_GROUP_EXIT;
+			signal_set_flags(sig, SIGNAL_GROUP_EXIT);

Well. I am not sure about this change. SIGNAL_GROUP_EXIT is the terminal state, it is fine to clear UNKILLABLE.

Yes, perhaps this actually makes sense, and we want to make UNKILLABLE immutable, but then we need to change force_sig_info() too, at least. And btw it should be changed anyway, in particular UNKILLABLE can be wrongly cleared if the task is traced. But I'd prefer to do this later.

The same for the similar changes in zap_process(), coredump_finish(), and complete_signal().

kernel/exit.c static void complete_signal(int sig, struct · line 926

Quoted 7 lines

kernel/exit.c +1 -1

@@ -922,7 +922,7 @@ static void complete_signal(int sig, struct 
 task_struct *p, int group)
 			 * running and doing things after a slower
 			 * thread has the fatal signal pending.
 			 */
-			signal->flags = SIGNAL_GROUP_EXIT;
+			signal_set_flags(signal, SIGNAL_GROUP_EXIT);

Again, I'd prefer to just set SIGNAL_GROUP_EXIT, like in do_group_exit(). Note also that SIGNAL_UNKILLABLE can't be set in this case, see the check above, so this change has no effect. And at the same time this code needs the changes too, this SIGNAL_UNKILLABLE check is not 100% correct, but this is off-topic.

So I think it would be better to start with the minimal change which fixes task_participate_group_stop() and prepare_signal() only. And while I won't insist, perhaps we should add

	#define SIGNAL_STOP_MASK (SIGNAL_CLD_MASK | SIGNAL_STOP_STOPPED | SIGNAL_STOP_CONTINUED)

	signal_set_stop_flags(signal, flags)
	{
		signal->flags = (signal->flags & ~SIGNAL_STOP_MASK) | flags;
	}

instead of signal_set_flags(). This way we can, say, add WARN_ON(signal->flags & (SIGNAL_GROUP_EXIT|SIGNAL_GROUP_COREDUMP)) into this helper. Then later we can add signal_set_exit_flags() which manipulates GROUP_EXIT/COREDUMP/UNKILLABLE.

Not sure, up to you.

Quoted 8 lines · Oleg Nesterov

include/linux/sched.h struct signal_struct { · line 851

Quoted 6 lines

include/linux/sched.h +5 -0

+static inline void signal_clear_flags(struct signal_struct *sig,
+				      unsigned int flags)
+{
+	sig->flags &= (~flags | SIGNAL_PROTECTED_MASK);
+}

But this one looks pointless.

Well the intent was to have set/clear helpers and *always* use those so that it's clear when SIGNAL_UNKILLABLE is being intentionally cleared.

Quoted 16 lines

So I think it would be better to start with the minimal change which fixes task_participate_group_stop() and prepare_signal() only. And while I won't insist, perhaps we should add

	#define SIGNAL_STOP_MASK (SIGNAL_CLD_MASK | SIGNAL_STOP_STOPPED | SIGNAL_STOP_CONTINUED)

	signal_set_stop_flags(signal, flags)
	{
		signal->flags = (signal->flags & ~SIGNAL_STOP_MASK) | flags;
	}

instead of signal_set_flags(). This way we can, say, add WARN_ON(signal->flags & (SIGNAL_GROUP_EXIT|SIGNAL_GROUP_COREDUMP)) into this helper. Then later we can add signal_set_exit_flags() which manipulates GROUP_EXIT/COREDUMP/UNKILLABLE.

Quoted 1 line

Not sure, up to you.

Right, so part of the challenge was figuring out where SIGNAL_UNKILLABLE should be cleared, and where it shouldn't. So is making it an explicit boolean in a bitfield a better approach?