[PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Ed Swierk]

With print-fatal-signals=1 on a kernel with CONFIG_PREEMPT=y, sending an
unexpected signal to a process causes a BUG: using smp_processor_id() in
preemptible code.

get_signal_to_deliver() releases the siglock before calling
print_fatal_signal(), which calls show_regs(), which calls
smp_processor_id(), which is not supposed to be called from a
preemptible thread.

Calling print_fatal_signal() before releasing the siglock avoids this
problem.

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

---
Index: linux-2.6.27.4/kernel/signal.c
===================================================================
--- linux-2.6.27.4.orig/kernel/signal.c
+++ linux-2.6.27.4/kernel/signal.c
@@ -1848,6 +1848,11 @@ relock:
 			continue;
 		}
 
+		if (sig_kernel_coredump(signr) && print_fatal_signals) {
+			/* hold lock for smp_processor_id() */
+			print_fatal_signal(regs, info->si_signo);
+		}
+
 		spin_unlock_irq(&sighand->siglock);
 
 		/*
@@ -1856,8 +1861,6 @@ relock:
 		current->flags |= PF_SIGNALED;
 
 		if (sig_kernel_coredump(signr)) {
-			if (print_fatal_signals)
-				print_fatal_signal(regs, info->si_signo);
 			/*
 			 * If it was able to dump core, this kills all
 			 * other threads in the group and synchronizes with

Re: [PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Ingo Molnar]

* Ed Swierk <eswierk@aristanetworks.com> wrote:

> With print-fatal-signals=1 on a kernel with CONFIG_PREEMPT=y, sending an
> unexpected signal to a process causes a BUG: using smp_processor_id() in
> preemptible code.
> 
> get_signal_to_deliver() releases the siglock before calling
> print_fatal_signal(), which calls show_regs(), which calls
> smp_processor_id(), which is not supposed to be called from a
> preemptible thread.
> 
> Calling print_fatal_signal() before releasing the siglock avoids this
> problem.
> 
> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
> 
> ---
> Index: linux-2.6.27.4/kernel/signal.c
> ===================================================================
> --- linux-2.6.27.4.orig/kernel/signal.c
> +++ linux-2.6.27.4/kernel/signal.c
> @@ -1848,6 +1848,11 @@ relock:
>  			continue;
>  		}
>  
> +		if (sig_kernel_coredump(signr) && print_fatal_signals) {
> +			/* hold lock for smp_processor_id() */
> +			print_fatal_signal(regs, info->si_signo);
> +		}
> +
>  		spin_unlock_irq(&sighand->siglock);

This trades a (harmless) debug warning against a potential deadlock or 
even a crash, because print_fatal_signal() can do this:

                        __get_user(insn, (unsigned char *)(regs->ip + i));

which will work without a fault most of the time but might also generate a 
pagefault and schedule away from atomic context.

So please add preempt_disable()+preempt_enable() calls around the 
show_regs() call instead.

	Ingo

Re: [PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Ed Swierk]

On Tue, 2009-01-27 at 00:15 +0100, Ingo Molnar wrote:
> This trades a (harmless) debug warning against a potential deadlock or 
> even a crash, because print_fatal_signal() can do this:
> 
>                         __get_user(insn, (unsigned char *)(regs->ip + i));
> 
> which will work without a fault most of the time but might also generate a 
> pagefault and schedule away from atomic context.

Ouch!

> So please add preempt_disable()+preempt_enable() calls around the 
> show_regs() call instead.

Take 2:

With print-fatal-signals=1 on a kernel with CONFIG_PREEMPT=y, sending an
unexpected signal to a process causes a BUG: using smp_processor_id() in
preemptible code.

get_signal_to_deliver() releases the siglock before calling
print_fatal_signal(), which calls show_regs(), which calls
smp_processor_id(), which is not supposed to be called from a
preemptible thread.

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

---
Index: linux-2.6.27.4/kernel/signal.c
===================================================================
--- linux-2.6.27.4.orig/kernel/signal.c
+++ linux-2.6.27.4/kernel/signal.c
@@ -890,7 +890,9 @@ static void print_fatal_signal(struct pt
 	}
 #endif
 	printk("\n");
+	preempt_disable();
 	show_regs(regs);
+	preempt_enable();
 }
 
 static int __init setup_print_fatal_signals(char *str)

Re: [PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Ingo Molnar]

* Ed Swierk <eswierk@aristanetworks.com> wrote:

> On Tue, 2009-01-27 at 00:15 +0100, Ingo Molnar wrote:
> > This trades a (harmless) debug warning against a potential deadlock or 
> > even a crash, because print_fatal_signal() can do this:
> > 
> >                         __get_user(insn, (unsigned char *)(regs->ip + i));
> > 
> > which will work without a fault most of the time but might also generate a 
> > pagefault and schedule away from atomic context.
> 
> Ouch!
> 
> > So please add preempt_disable()+preempt_enable() calls around the 
> > show_regs() call instead.
> 
> Take 2:
> 
> With print-fatal-signals=1 on a kernel with CONFIG_PREEMPT=y, sending an
> unexpected signal to a process causes a BUG: using smp_processor_id() in
> preemptible code.
> 
> get_signal_to_deliver() releases the siglock before calling
> print_fatal_signal(), which calls show_regs(), which calls
> smp_processor_id(), which is not supposed to be called from a
> preemptible thread.
> 
> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

applied to tip/core/urgent, thanks Ed!

You can track/test your fix via the -tip tree's tip/master branch:

  http://people.redhat.com/mingo/tip.git/README

	Ingo

Re: [PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Oleg Nesterov]

On 01/27, Ingo Molnar wrote:
>
> * Ed Swierk <eswierk@aristanetworks.com> wrote:
>
> > Take 2:
> >
> > With print-fatal-signals=1 on a kernel with CONFIG_PREEMPT=y, sending an
> > unexpected signal to a process causes a BUG: using smp_processor_id() in
> > preemptible code.
> >
> > get_signal_to_deliver() releases the siglock before calling
> > print_fatal_signal(), which calls show_regs(), which calls
> > smp_processor_id(), which is not supposed to be called from a
> > preemptible thread.
> >
> > Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
>
> applied to tip/core/urgent, thanks Ed!

Ed, Ingo, but isn't it better to just use raw_smp_processor_id() in
__show_regs() ? This is only debug info, the printed CPU doesn't
have the "exact" meaning.

And, without the comment, it is not easy to see why print_fatal_signal()
disables preeemption before show_regs().

Oleg.

Re: [PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Ed Swierk]

On Tue, 2009-01-27 at 01:41 +0100, Oleg Nesterov wrote:
> Ed, Ingo, but isn't it better to just use raw_smp_processor_id() in
> __show_regs() ? This is only debug info, the printed CPU doesn't
> have the "exact" meaning.

I guess it doesn't really matter which CPU the signal handling thread
happened to be running on, but are there other situations where
show_regs() is always expected to print the correct CPU (and if not, why
bother printing the CPU at all)?  Disabling preemption here seems the
safest approach and doesn't add much overhead. 

> And, without the comment, it is not easy to see why print_fatal_signal()
> disables preeemption before show_regs().

Agreed; here's an updated patch.

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

---
Index: linux-2.6.27.4/kernel/signal.c
===================================================================
--- linux-2.6.27.4.orig/kernel/signal.c
+++ linux-2.6.27.4/kernel/signal.c
@@ -890,7 +890,9 @@ static void print_fatal_signal(struct pt
 	}
 #endif
 	printk("\n");
-	show_regs(regs);
+	preempt_disable();
+	show_regs(regs); /* calls smp_processor_id(), preemption not allowed */
+	preempt_enable();
 }
 
 static int __init setup_print_fatal_signals(char *str)

Re: [PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Oleg Nesterov]

On 01/26, Ed Swierk wrote:
>
> On Tue, 2009-01-27 at 01:41 +0100, Oleg Nesterov wrote:
> > Ed, Ingo, but isn't it better to just use raw_smp_processor_id() in
> > __show_regs() ? This is only debug info, the printed CPU doesn't
> > have the "exact" meaning.
>
> I guess it doesn't really matter which CPU the signal handling thread
> happened to be running on, but are there other situations where
> show_regs() is always expected to print the correct CPU (and if not, why
> bother printing the CPU at all)?  Disabling preemption here seems the
> safest approach and doesn't add much overhead.

OK.

> > And, without the comment, it is not easy to see why print_fatal_signal()
> > disables preeemption before show_regs().
>
> Agreed; here's an updated patch.

Actually, now I think show_regs() has other reasons to run with the
preemption disabled, __show_regs() does read_crX()/etc, I guess it is
better to stay on the same CPU throughout.

So, Ed, I am sorry for noise.

Oleg.

Re: [PATCH] Fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 01/26, Ed Swierk wrote:
> >
> > On Tue, 2009-01-27 at 01:41 +0100, Oleg Nesterov wrote:
> > > Ed, Ingo, but isn't it better to just use raw_smp_processor_id() in
> > > __show_regs() ? This is only debug info, the printed CPU doesn't
> > > have the "exact" meaning.
> >
> > I guess it doesn't really matter which CPU the signal handling thread 
> > happened to be running on, but are there other situations where 
> > show_regs() is always expected to print the correct CPU (and if not, 
> > why bother printing the CPU at all)?  Disabling preemption here seems 
> > the safest approach and doesn't add much overhead.
> 
> OK.
> 
> > > And, without the comment, it is not easy to see why print_fatal_signal()
> > > disables preeemption before show_regs().
> >
> > Agreed; here's an updated patch.
> 
> Actually, now I think show_regs() has other reasons to run with the 
> preemption disabled, __show_regs() does read_crX()/etc, I guess it is 
> better to stay on the same CPU throughout.
> 
> So, Ed, I am sorry for noise.

another reason why it's good to run it with preemption disabled is that 
whatever context does show_regs() ought to be non-preemptible as it deals 
with CPU local details.

In the fatal-signals case we indeed have a "it does not really matter" 
boundary case, but in most of the other uses we want to be non-preemptible 
in debug contexts, and want a constant reminder in terms of 
smp_processor_id() warnings if that expectation is not met.

	Ingo