Re: Running OOM and worse with broken signal handler

[Kirill Korotaev <dev@sw.ru>]

Andrew, here is the patch with the comment for the problem described below.

Patch comment:
If SIGKILL does not have priority, we cannot instantly kill task
before it makes some unexpected job. It can be critical, but we
were unable to reproduce this easily until Heiko Carstens 
<Heiko.Carstens@de.ibm.com> reported this problem on LKML.

Signed-Off-By: Kirill Korotaev <dev@sw.ru>
Signed-Off-By: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>

Kirill


> Hi Kirill,
>
> your patch fixes this issue. Thanks!
> Andrew, any chances to get this merged?
>
> Heiko
>
>
> > Can you test this patch, please?
> >
> > Alexey Kuznetsov discovered long ago that SIGKILL is low priority than 
> > signalls 1-8, so it can be delivered very long... But we didn't 
> > succedded to reproduce this in real life, looks like you did it :)
> >
> > Kirill
> >
> >
> > > Hi all,
> > >
> > > we experienced some interesting behaviour with an out of
> > > memory condition caused by signal handling (on s390x).
> > > The following program ran our system in an OOM situation
> > > and couldn't be killed because the SIGKILL signal couldn't
> > > be delivered.
> > > Necessary for this to happen is that the stack size limit
> > > is set to unlimited.
> > >
> > > sig_handler(int sig)
> > > {
> > >  asm volatile(".long 0\n");
> > > }
> > >
> > > int main (int argc, char **argv)
> > > {
> > >  struct sigaction act;
> > >
> > >  act.sa_handler = &sig_handler;
> > >  act.sa_restorer = 0;
> > >  act.sa_flags = SA_NOMASK | SA_RESTART;
> > >
> > >  sigaction(SIGILL, &act, 0);
> > >  sigaction(SIGSEGV, &act, 0);
> > >
> > >  asm volatile(".long 0\n");
> > > }
> > >
> > > The instruction in the asm block is suppossed to be an
> > > illegal opcode which enforces a SIGILL.
> > > When executed the following happens:
> > > The illegal instruction causes a SIGILL to be delivered to
> > > the process. Since the signal handler itself contains an
> > > illegal instruction this causes another SIGILL to
> > > be delivered, thus causing the stack to grow unlimited.
> > > When we are finally out of memory the OOM killer selects
> > > our process and sends it a SIGKILL.
> > > Only problem in this scenario is that the SIGKILL never
> > > will be sent to our process simply because there is
> > > always a SIGILL pending too, which will be handled before
> > > the SIGKILL because of its lower number (see next_signal()
> > > in kernel/signal.c).
> > > The only possibly way this signal would be handled would
> > > be that the process is running in userspace while trying
> > > to handle the delivered SIGILL, where it would be interrupted
> > > by an interrupt and upon return to userspace do_signal()
> > > would be called again. This is unfortunately very unlikely
> > > if you are running a nearly timer interrupt free kernel
> > > like we do on s390/s390x.
> > > Since the OOM killer set the TIF_MEMDIE flag for our
> > > process it now is allowed to eat up all the memory left
> > > and our system is more or less dead until you're lucky
> > > and an interrupt hits at the right time and finally
> > > causing the process to be terminated...
> > >
> > > Maybe the OOM killer or signal handling would need
> > > a change to fix this?
> > >
> > > Thanks,
> > > Heiko
> > > -
> > > To unsubscribe from this list: send the line "unsubscribe 
>
> linux-kernel" in
>
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> >
> > diff -ur orig/linux-2.6.8.1/kernel/signal.c 
>
> linux-2.6.8.1/kernel/signal.c
>
> > --- orig/linux-2.6.8.1/kernel/signal.c   2005-05-12 02:44:12.000000000 
>
> +0400
>
> > +++ linux-2.6.8.1/kernel/signal.c   2005-05-13 12:07:04.000000000 +0400
> > @@ -519,7 +520,16 @@
> > {
> >    int sig = 0;
> >
> > -   sig = next_signal(pending, mask);
> > +   /* 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))
> > +      sig = next_signal(pending, mask);
> >    if (sig) {
> >       if (current->notifier) {
> >          if (sigismember(current->notifier_mask, sig)) {

diff -ur orig/linux-2.6.8.1/kernel/signal.c linux-2.6.8.1/kernel/signal.c
--- orig/linux-2.6.8.1/kernel/signal.c	2005-05-12 02:44:12.000000000 +0400
+++ linux-2.6.8.1/kernel/signal.c	2005-05-13 12:07:04.000000000 +0400
@@ -519,7 +520,16 @@
 {
 	int sig = 0;
 
-	sig = next_signal(pending, mask);
+	/* 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))
+		sig = next_signal(pending, mask);
 	if (sig) {
 		if (current->notifier) {
 			if (sigismember(current->notifier_mask, sig)) {