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 <[email protected]>
---
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
* Ed Swierk <[email protected]> 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 <[email protected]>
>
> ---
> 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
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 <[email protected]>
---
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)
* Ed Swierk <[email protected]> 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 <[email protected]>
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
On 01/27, Ingo Molnar wrote:
>
> * Ed Swierk <[email protected]> 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 <[email protected]>
>
> 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.
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 <[email protected]>
---
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)
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.
* Oleg Nesterov <[email protected]> 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