[PATCH] more sigkill priority fix

[PATCH] more sigkill priority fix

[Atsushi Nemoto]

On Linux/MIPS, a simple test program can create unkillable process.
The "sigkill priority fix" was introduced in 2.6.12, but it does not
effective for signals sent by force_sig() in kernel.  For detailed
behavior and testcase, please look at this thread in linux-mips ML:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=20050907.014234.108739386.anemo%40mba.ocn.ne.jp

Here is a proposal fix for generic signal handling code.

Patch comment:
The "sigkill priority fix" does not work as it desired if any signal
(< SIGKILL) was queued by force_sig() in kernel.  Search SIGKILL in
tsk->pending and tsk->signal->shared_pending first, then search
another signals.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

--- linux-2.6.13/kernel/signal.c	2005-08-29 08:41:01.000000000 +0900
+++ linux/kernel/signal.c	2005-09-07 01:33:52.338420760 +0900
@@ -520,19 +520,14 @@
 }
 
 static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
-			siginfo_t *info)
+			siginfo_t *info, int sig)
 {
-	int sig = 0;
-
-	/* SIGKILL must have priority, otherwise it is quite easy
-	 * to create an unkillable process, sending sig < SIGKILL
-	 * to self */
-	if (unlikely(sigismember(&pending->signal, SIGKILL))) {
-		if (!sigismember(mask, SIGKILL))
-			sig = SIGKILL;
-	}
-
-	if (likely(!sig))
+	if (sig) {
+		/* check signal with priority first */
+		if (likely(!sigismember(&pending->signal, sig)) ||
+		    sigismember(mask, sig))
+			sig = 0;
+	} else
 		sig = next_signal(pending, mask);
 	if (sig) {
 		if (current->notifier) {
@@ -561,10 +556,18 @@
  */
 int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
 {
-	int signr = __dequeue_signal(&tsk->pending, mask, info);
+	/* SIGKILL must have priority, otherwise it is quite easy
+	 * to create an unkillable process, sending sig < SIGKILL
+	 * to self */
+	int signr = __dequeue_signal(&tsk->pending, mask, info, SIGKILL);
+	if (likely(!signr))
+		signr = __dequeue_signal(&tsk->signal->shared_pending,
+					 mask, info, SIGKILL);
+	if (likely(!signr))
+		signr = __dequeue_signal(&tsk->pending, mask, info, 0);
 	if (!signr)
 		signr = __dequeue_signal(&tsk->signal->shared_pending,
-					 mask, info);
+					 mask, info, 0);
  	if (signr && unlikely(sig_kernel_stop(signr))) {
  		/*
  		 * Set a marker that we have dequeued a stop signal.  Our

---
Atsushi Nemoto

Re: [PATCH] more sigkill priority fix

[Atsushi Nemoto]

Adding Kirill Korotaev and Heiko Carstens to CC.

>>>>> On Thu, 08 Sep 2005 01:24:50 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:

anemo> On Linux/MIPS, a simple test program can create unkillable
anemo> process.  The "sigkill priority fix" was introduced in 2.6.12,
anemo> but it does not effective for signals sent by force_sig() in
anemo> kernel.  For detailed behavior and testcase, please look at
anemo> this thread in linux-mips ML:

This is fixed by another way in 2.6.14-rc1 for i386 (Thanks, Roland).
The changelog line is:

>    [PATCH] i386: Don't miss pending signals returning to user mode after signal processing
>    Signed-off-by: Roland McGrath <roland@redhat.com>

And now similar fix for mips is already in Linux/MIPS CVS tree too.

--- linux-mips/arch/mips/kernel/entry.S	2005-03-04 22:17:29.000000000 +0900
+++ linux/arch/mips/kernel/entry.S	2005-09-16 01:04:52.365022536 +0900
@@ -105,7 +105,7 @@
 	move	a0, sp
 	li	a1, 0
 	jal	do_notify_resume	# a2 already loaded
-	j	restore_all
+	j	resume_userspace
 
 FEXPORT(syscall_exit_work_partial)
 	SAVE_STATIC


I suppose the original problem on s390 (reported by Heiko Carstens)
could be fixed same way.  Then 'sigkill priority fix' would be
reverted safely.

---
Atsushi Nemoto

Re: [PATCH] more sigkill priority fix

[Heiko Carstens]

> anemo> On Linux/MIPS, a simple test program can create unkillable
> anemo> process.  The "sigkill priority fix" was introduced in 2.6.12,
> anemo> but it does not effective for signals sent by force_sig() in
> anemo> kernel.  For detailed behavior and testcase, please look at
> anemo> this thread in linux-mips ML:
> 
> This is fixed by another way in 2.6.14-rc1 for i386 (Thanks, Roland).
> The changelog line is:
> 
> >    [PATCH] i386: Don't miss pending signals returning to user mode after signal processing
> >    Signed-off-by: Roland McGrath <roland@redhat.com>
> 
> And now similar fix for mips is already in Linux/MIPS CVS tree too.
> 
> --- linux-mips/arch/mips/kernel/entry.S	2005-03-04 22:17:29.000000000 +0900
> +++ linux/arch/mips/kernel/entry.S	2005-09-16 01:04:52.365022536 +0900
> @@ -105,7 +105,7 @@
>  	move	a0, sp
>  	li	a1, 0
>  	jal	do_notify_resume	# a2 already loaded
> -	j	restore_all
> +	j	resume_userspace
>  
>  FEXPORT(syscall_exit_work_partial)
>  	SAVE_STATIC
> 
> 
> I suppose the original problem on s390 (reported by Heiko Carstens)
> could be fixed same way.  Then 'sigkill priority fix' would be
> reverted safely.

If I understand the two arch changes correctly then this means that before
going back to userspace always _all_ pending signals will be delivered.
Of course this would fix the original problem and the 'sigkill priority fix'
could be reverted, if all architectures would implement this behaviour.
Is this the way the kernel is supposed to handle signals now?
Just wondering, since this changes signal handling quite significantly from
what it was before.

Heiko

Re: [PATCH] more sigkill priority fix

[Roland McGrath]

> Is this the way the kernel is supposed to handle signals now?
> Just wondering, since this changes signal handling quite significantly from
> what it was before.

It has always been the correct behavior.


Thanks,
Roland

Re: [PATCH] more sigkill priority fix

[Martin Schwidefsky]

On Mon, 2005-09-19 at 01:46 -0700, Roland McGrath wrote:
> > Is this the way the kernel is supposed to handle signals now?
> > Just wondering, since this changes signal handling quite significantly from
> > what it was before.
> 
> It has always been the correct behavior.

Does that mean that it is incorrect to deliver one signal at a time?

-- 
blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Re: [PATCH] more sigkill priority fix

[Roland McGrath]

> Does that mean that it is incorrect to deliver one signal at a time?

Whenever there are pending unblocked signals, they need to be delivered
before running any more user code.  If after setting up a signal handler
(and blocking signals for it), there is another unblocked signal pending,
then that must be delivered immediately.  This can mean terminating the
process before the handler ever runs, or it can mean setting up another
signal handler frame starting from the context of the first handler frame. 

The only change with this bug fix is that this reliably happens immediately.
Before, it would always happen pretty soon (except in a pathological case
with a bad stack).  That is, the next preemption, system call, fault, 
other trap, or external interrupt--anything that entered the kernel--would
check the pending signals properly before returning to user mode.  


Thanks,
Roland