[PATCH 20/40] arm,kgdb: Add hook to catch an oops with debugger

[PATCH 20/40] arm,kgdb: Add hook to catch an oops with debugger

[Russell King - ARM Linux]

On Thu, Jan 14, 2010 at 08:59:16AM -0600, Jason Wessel wrote:
> Add in a low level hook to catch calls to die() in the debugger.
> 
> After the debugger is done, the standard system rules will be in play
> for the original exception.
> 
> The kdb debugger wants a chance to catch these sorts of exceptions for
> analysis.

NAK.  I have a similar patch which implements the hook properly - but
with one caveat.  It needs a review to ensure that its safe to return
from die().  Until that's established, this patch can not be merged.

Please also ensure that the linux-arm-kernel list is copied with ARM
patches.

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index d65b2f5..5b66e51 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -73,8 +73,7 @@ extern unsigned int mem_fclk_21285;
 
 struct pt_regs;
 
-void die(const char *msg, struct pt_regs *regs, int err)
-		__attribute__((noreturn));
+void die(const char *msg, struct pt_regs *regs, int err);
 
 struct siginfo;
 void arm_notify_die(const char *str, struct pt_regs *regs, struct siginfo *info,
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index f838f36..29a0f4a 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -12,15 +12,17 @@
  *  'linux/arch/arm/lib/traps.S'.  Mostly a debugging aid, but will probably
  *  kill the offending process.
  */
-#include <linux/module.h>
 #include <linux/signal.h>
-#include <linux/spinlock.h>
 #include <linux/personality.h>
 #include <linux/kallsyms.h>
-#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
 #include <linux/hardirq.h>
+#include <linux/kdebug.h>
+#include <linux/module.h>
+#include <linux/kexec.h>
+#include <linux/delay.h>
 #include <linux/init.h>
-#include <linux/uaccess.h>
 
 #include <asm/atomic.h>
 #include <asm/cacheflush.h>
@@ -224,14 +226,21 @@ void show_stack(struct task_struct *tsk, unsigned long *sp)
 #define S_SMP ""
 #endif
 
-static void __die(const char *str, int err, struct thread_info *thread, struct pt_regs *regs)
+static int __die(const char *str, int err, struct thread_info *thread, struct pt_regs *regs)
 {
 	struct task_struct *tsk = thread->task;
 	static int die_counter;
+	int ret;
 
 	printk(KERN_EMERG "Internal error: %s: %x [#%d]" S_PREEMPT S_SMP "\n",
 	       str, err, ++die_counter);
 	sysfs_printk_last_file();
+
+	/* trap and error numbers are mostly meaningless on ARM */
+	ret = notify_die(DIE_OOPS, str, regs, err, tsk->thread.trap_no, SIGSEGV);
+	if (ret == NOTIFY_STOP)
+		return ret;
+
 	print_modules();
 	__show_regs(regs);
 	printk(KERN_EMERG "Process %.*s (pid: %d, stack limit = 0x%p)\n",
@@ -243,6 +252,8 @@ static void __die(const char *str, int err, struct thread_info *thread, struct p
 		dump_backtrace(regs, tsk);
 		dump_instr(KERN_EMERG, regs);
 	}
+
+	return ret;
 }
 
 DEFINE_SPINLOCK(die_lock);
@@ -250,16 +261,21 @@ DEFINE_SPINLOCK(die_lock);
 /*
  * This function is protected against re-entrancy.
  */
-NORET_TYPE void die(const char *str, struct pt_regs *regs, int err)
+void die(const char *str, struct pt_regs *regs, int err)
 {
 	struct thread_info *thread = current_thread_info();
+	int ret;
 
 	oops_enter();
 
 	spin_lock_irq(&die_lock);
 	console_verbose();
 	bust_spinlocks(1);
-	__die(str, err, thread, regs);
+	ret = __die(str, err, thread, regs);
+
+	if (regs && kexec_should_crash(thread->task))
+		crash_kexec(regs);
+
 	bust_spinlocks(0);
 	add_taint(TAINT_DIE);
 	spin_unlock_irq(&die_lock);
@@ -267,11 +283,10 @@ NORET_TYPE void die(const char *str, struct pt_regs *regs, int err)
 
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
-
 	if (panic_on_oops)
 		panic("Fatal exception");
-
-	do_exit(SIGSEGV);
+	if (ret != NOTIFY_STOP)
+		do_exit(SIGSEGV);
 }
 
 void arm_notify_die(const char *str, struct pt_regs *regs,

[PATCH 20/40] arm, kgdb: Add hook to catch an oops with debugger

[Jason Wessel]

Russell King - ARM Linux wrote:
> On Thu, Jan 14, 2010 at 08:59:16AM -0600, Jason Wessel wrote:
>   
>> Add in a low level hook to catch calls to die() in the debugger.
>>
>> After the debugger is done, the standard system rules will be in play
>> for the original exception.
>>
>> The kdb debugger wants a chance to catch these sorts of exceptions for
>> analysis.
>>     
>
> NAK.  I have a similar patch which implements the hook properly - but
> with one caveat.  It needs a review to ensure that its safe to return
> from die().  Until that's established, this patch can not be merged.
>   

I dropped the patch which you NAK'ed from the series.

As for your included patch, I will try it out  against the various
regression tests I have, which exercise the oops and exception paths.

> Please also ensure that the linux-arm-kernel list is copied with ARM
> patches.
>
>   

Check.


Thanks,
Jason.

[PATCH 20/40] arm, kgdb: Add hook to catch an oops with debugger

[Jason Wessel]

Russell King - ARM Linux wrote:
> 
> I have a similar patch which implements the hook properly - but
> with one caveat.  It needs a review to ensure that its safe to return
> from die().  Until that's established, this patch can not be merged.
> 

I completed the analysis on your patch and yes, it is safe to return
from __die() and die() the way you currently structured it, but it
doesn't work quite the same as on some other architectures.

After changing kgdb.c to register with the die notifier, I stepped
through your code with an ICE, as well as running my regression tests
which panic, oops, bad access etc...

While kernel execution does happen to continue to work, I don't know
that you really want to continue execution.

1) The kernel is marked tainted
2) bust_spinlocks() was toggled for a while

On x86 for example, the notifier is invoked prior to the
bust_spinlocks() etc... and then it can pass the exception along to
the rest of the system (which can result in something bad, but
remember the human behind the kernel debugger controls did it for some
reason or another).

I made the following addition to your patch, and then it behaved as
the other archs do with respect to passing along the result of the
exception.  Given this information, would you be willing to merge your
patch and possibly fold in the change below, or further comment?

Thanks,
Jason.

--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -273,6 +273,9 @@ void die(const char *str, struct pt_regs
 	bust_spinlocks(1);
 	ret = __die(str, err, thread, regs);
 
+	if (ret == NOTIFY_STOP)
+		return;
+
 	if (regs && kexec_should_crash(thread->task))
 		crash_kexec(regs);
 
@@ -285,8 +288,7 @@ void die(const char *str, struct pt_regs
 		panic("Fatal exception in interrupt");
 	if (panic_on_oops)
 		panic("Fatal exception");
-	if (ret != NOTIFY_STOP)
-		do_exit(SIGSEGV);
+	do_exit(SIGSEGV);
 }
 
 void arm_notify_die(const char *str, struct pt_regs *regs,

[PATCH 20/40] arm,kgdb: Add hook to catch an oops with debugger

[Russell King - ARM Linux]

On Thu, Jan 14, 2010 at 02:29:54PM -0600, Jason Wessel wrote:
> Russell King - ARM Linux wrote:
> > 
> > I have a similar patch which implements the hook properly - but
> > with one caveat.  It needs a review to ensure that its safe to return
> > from die().  Until that's established, this patch can not be merged.
> 
> I completed the analysis on your patch and yes, it is safe to return
> from __die() and die() the way you currently structured it, but it
> doesn't work quite the same as on some other architectures.
> 
> After changing kgdb.c to register with the die notifier, I stepped
> through your code with an ICE, as well as running my regression tests
> which panic, oops, bad access etc...
> 
> While kernel execution does happen to continue to work, I don't know
> that you really want to continue execution.
> 
> 1) The kernel is marked tainted
> 2) bust_spinlocks() was toggled for a while
> 
> On x86 for example, the notifier is invoked prior to the
> bust_spinlocks() etc... and then it can pass the exception along to
> the rest of the system (which can result in something bad, but
> remember the human behind the kernel debugger controls did it for some
> reason or another).

On x86, it's called in multiple places - both before die(), and also
inside __die().

In __die(), notify_die() gets called with DIE_OOPS.  There's also a
pile of notify_die() calls in arch/x86/kernel/traps.c, which we don't
implement on ARM yet - it's unclear what's required here, and until
we have a user of notify_die()...

> I made the following addition to your patch, and then it behaved as
> the other archs do with respect to passing along the result of the
> exception.  Given this information, would you be willing to merge your
> patch and possibly fold in the change below, or further comment?

This changes the behaviour away from x86, so I'm not sure it's the
right thing to do.  For instance, it means that kexec won't get to
know about the oops on ARM if NOTIFY_STOP is returned, whereas on
x86 it will.

Maybe this hook wasn't meant for kgdb - what does kgdb use on x86?

[PATCH 20/40] arm,kgdb: Add hook to catch an oops with debugger

[Jason Wessel]

Russell King - ARM Linux wrote:
> On Thu, Jan 14, 2010 at 02:29:54PM -0600, Jason Wessel wrote:
>   
>> Russell King - ARM Linux wrote:
>>     
>>> I have a similar patch which implements the hook properly - but
>>> with one caveat.  It needs a review to ensure that its safe to return
>>> from die().  Until that's established, this patch can not be merged.
>>>       
>> I completed the analysis on your patch and yes, it is safe to return
>> from __die() and die() the way you currently structured it, but it
>> doesn't work quite the same as on some other architectures.
>>
>> After changing kgdb.c to register with the die notifier, I stepped
>> through your code with an ICE, as well as running my regression tests
>> which panic, oops, bad access etc...
>>
>> While kernel execution does happen to continue to work, I don't know
>> that you really want to continue execution.
>>
>> 1) The kernel is marked tainted
>> 2) bust_spinlocks() was toggled for a while
>>
>> On x86 for example, the notifier is invoked prior to the
>> bust_spinlocks() etc... and then it can pass the exception along to
>> the rest of the system (which can result in something bad, but
>> remember the human behind the kernel debugger controls did it for some
>> reason or another).
>>     
>
> On x86, it's called in multiple places - both before die(), and also
> inside __die().
>
> In __die(), notify_die() gets called with DIE_OOPS.  There's also a
> pile of notify_die() calls in arch/x86/kernel/traps.c, which we don't
> implement on ARM yet - it's unclear what's required here, and until
> we have a user of notify_die()...
>
>   

Initially I was just looking to get the memory violation tests to pass
on ARM, where the kernel debugger can catch an invalid memory write for
instance.  That means anything that generates any kind of system fault
should jump into the debugger via the die notifier.  There might be
other places for this on ARM, but I figured we could start with the
passing the memory fault tests first.

>> I made the following addition to your patch, and then it behaved as
>> the other archs do with respect to passing along the result of the
>> exception.  Given this information, would you be willing to merge your
>> patch and possibly fold in the change below, or further comment?
>>     
>
> This changes the behaviour away from x86, so I'm not sure it's the
> right thing to do.  For instance, it means that kexec won't get to
> know about the oops on ARM if NOTIFY_STOP is returned, whereas on
> x86 it will.
>
> Maybe this hook wasn't meant for kgdb - what does kgdb use on x86?
>   

On x86, kgdb uses the notify die hook.  It is possible that there are
some inconsistent uses of the notifiy_die(), but the general idea is
that any user in the hook path can elect to consume the exception and
allow the system to restore.

In terms of kgdb's use of this, I have only found it useful for
programmatic testing of exception cases.  Specifically when using kdb,
the default are always to propagate exceptions unless it was a
breakpoint or single step exception which was set by the kernel debugger.

That being said, you patch works for the purpose of catching the
exception and returning with or without the addition of an earlier
return before bust_spinlocks() which I had proposed.

Jason.

[PATCH 20/40] arm,kgdb: Add hook to catch an oops with debugger

[Russell King - ARM Linux]

On Mon, Jan 18, 2010 at 08:30:04AM -0600, Jason Wessel wrote:
> That being said, you patch works for the purpose of catching the
> exception and returning with or without the addition of an earlier
> return before bust_spinlocks() which I had proposed.

So does that mean I can have an acked-by for the patch?

[PATCH 20/40] arm,kgdb: Add hook to catch an oops with debugger

[Jason Wessel]

Russell King - ARM Linux wrote:
> On Mon, Jan 18, 2010 at 08:30:04AM -0600, Jason Wessel wrote:
>   
>> That being said, you patch works for the purpose of catching the
>> exception and returning with or without the addition of an earlier
>> return before bust_spinlocks() which I had proposed.
>>     
>
> So does that mean I can have an acked-by for the patch?
>   
Absolutely.

Jason.

Acked-by: Jason Wessel <jason.wessel@windriver.com>