[PATCH 0/2] powerpc: oops fixes

Hi,

Couple of small things I've come across. After this, injecting nmi to
enter xmon (via qemu) and then recovering works nicely.

Thanks,
Nick

Nicholas Piggin (2):
  powerpc: fix graceful debugger recovery
  powerpc: fix second nested oops hang

 arch/powerpc/kernel/traps.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.10.2

When exiting xmon with 'x' (exit and recover), oops_begin bails
out immediately, but die then calls __die() and oops_end(), which
cause a lot of bad things to happen.

If the debugger was attached then went to graceful recovery, exit
from die() immediately.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/traps.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 7480902..26c3ba4 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c

@@ -122,9 +122,6 @@ static unsigned long oops_begin(struct pt_regs *regs)
 	int cpu;
 	unsigned long flags;
 
-	if (debugger(regs))
-		return 1;
-
 	oops_enter();
 
 	/* racy, but better than risking deadlock. */

@@ -227,8 +224,12 @@ NOKPROBE_SYMBOL(__die);
 
 void die(const char *str, struct pt_regs *regs, long err)
 {
-	unsigned long flags = oops_begin(regs);
+	unsigned long flags;
+
+	if (debugger(regs))
+		return;
 
+	flags = oops_begin(regs);
 	if (__die(str, regs, err))
 		err = 0;
 	oops_end(flags, regs, err);

-- 
2.10.2

When ending an oops, don't clear die_owner unless the nest count
went to zero. This prevents a second nested oops from hanging forever
on the die_lock.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/traps.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 26c3ba4..ba78e38 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c

@@ -147,14 +147,15 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
 			       int signr)
 {
 	bust_spinlocks(0);
-	die_owner = -1;
 	add_taint(TAINT_DIE, LOCKDEP_NOW_UNRELIABLE);
 	die_nest_count--;
 	oops_exit();
 	printk("\n");
-	if (!die_nest_count)
+	if (!die_nest_count) {
 		/* Nest count reaches zero, release the lock. */
+		die_owner = -1;
 		arch_spin_unlock(&die_lock);
+	}
 	raw_local_irq_restore(flags);
 
 	crash_fadump(regs, "die oops");

-- 
2.10.2

Nicholas Piggin [off-list ref] writes:

When exiting xmon with 'x' (exit and recover), oops_begin bails
out immediately, but die then calls __die() and oops_end(), which
cause a lot of bad things to happen.

In fact oops_begin() returns 1, which oops_end() then passes directly to
raw_local_irq_restore() as flags. On 64-bit that actually works because
arch_local_irq_restore() takes just "en" (enable), not real flags. But
on 32-bit it's supposed to be the MSR value. So that's impressively
broken.

If the debugger was attached then went to graceful recovery, exit
from die() immediately.

Right. Crucially it doesn't change anything in terms of the actual logic
of oops_begin(), ie. previously oops_begin() did nothing prior to
calling debugger(), and after this patch that remains the same (which
you did mention above but just spelling it out for myself).

cheers

On Thu, 10 Nov 2016 12:35:59 +1100
Michael Ellerman [off-list ref] wrote:

Nicholas Piggin [off-list ref] writes:

quoted

When exiting xmon with 'x' (exit and recover), oops_begin bails
out immediately, but die then calls __die() and oops_end(), which
cause a lot of bad things to happen.  

In fact oops_begin() returns 1, which oops_end() then passes directly to
raw_local_irq_restore() as flags. On 64-bit that actually works because
arch_local_irq_restore() takes just "en" (enable), not real flags. But
on 32-bit it's supposed to be the MSR value. So that's impressively
broken.

Yeah, I guess most of the time you either go to debugger with
sysrq, or in case of a crash don't try to graceful recover.

When sending debug NMIs down via system reset it becomes a problem!

quoted

If the debugger was attached then went to graceful recovery, exit
from die() immediately.  

Right. Crucially it doesn't change anything in terms of the actual logic
of oops_begin(), ie. previously oops_begin() did nothing prior to
calling debugger(), and after this patch that remains the same (which
you did mention above but just spelling it out for myself).

Right.

Thanks,
Nick

Nicholas Piggin [off-list ref] writes:

On Thu, 10 Nov 2016 12:35:59 +1100
Michael Ellerman [off-list ref] wrote:

quoted

Nicholas Piggin [off-list ref] writes:

quoted

When exiting xmon with 'x' (exit and recover), oops_begin bails
out immediately, but die then calls __die() and oops_end(), which
cause a lot of bad things to happen.  

In fact oops_begin() returns 1, which oops_end() then passes directly to
raw_local_irq_restore() as flags. On 64-bit that actually works because
arch_local_irq_restore() takes just "en" (enable), not real flags. But
on 32-bit it's supposed to be the MSR value. So that's impressively
broken.

Yeah, I guess most of the time you either go to debugger with
sysrq, or in case of a crash don't try to graceful recover.

Yeah. It's debatable whether we should even allow graceful recovery, but
it's useful sometimes and regular users probably shouldn't have a
debugger enabled anyway.

cheers

On Tue, 2016-11-08 at 12:14:44 UTC, Nicholas Piggin wrote:

When exiting xmon with 'x' (exit and recover), oops_begin bails
out immediately, but die then calls __die() and oops_end(), which
cause a lot of bad things to happen.

If the debugger was attached then went to graceful recovery, exit
from die() immediately.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/6f44b20ee9b4130345c189c0c90ef6

cheers

On Tue, 2016-11-08 at 12:14:45 UTC, Nicholas Piggin wrote:

When ending an oops, don't clear die_owner unless the nest count
went to zero. This prevents a second nested oops from hanging forever
on the die_lock.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/7458e8b2ce62e65051fec0bcb14cac

cheers