port-powerpc/51367: kernel panic for powerpc/ibm4xx with "option DDB"

[rokuyama%rk.phys.keio.ac.jp@localhost]

>Number:         51367
>Category:       port-powerpc
>Synopsis:       kernel panic for powerpc/ibm4xx with "option DDB"
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    port-powerpc-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jul 27 15:05:00 +0000 2016
>Originator:     Rin Okuyama
>Release:        HEAD
>Organization:
Faculty of Science and Technology, Keio University
>Environment:
NetBSD obs266 7.99.34 NetBSD 7.99.34 (OPENBLOCKS266) #1: Wed Jul 27 22:09:26 JST 2016  rin@XXX:XXX evbppc
>Description:
Kernel panic occurs for powerpc/ibm4xx (e.g., evbppc/OPENBLOCKS266) when
an userland process issues an illegal instruction. For example, reboot
system with sshd=YES in rc.conf, then, you will observe

====================
  ...
  Updating motd.
  Stopped in pid 358.1 (ssh-keygen) at    0:      illegal instruction 7074000
  db>
====================

Here, ssh-keygen issues an illegal instruction because of assembler codes
in OpenSSL. This is due to the trap handler for ddb: ssh-keygen, for
example, works fine with kernel compiled without DDB option.
>How-To-Repeat:
Described above.
>Fix:
In the trap handler for ddb, check whether it is called in the privileged
mode or user mode. If it is called in the user mode, switch to the default
trap handler, as in a similar manner to powerpc/powerpc/trap_subr.S. Then,
an illegal instruction in the user mode does not trigger kernel panic.
I've checked that ddb can be activated from console, and also "continue"
command works fine. The patch attached below contains fix to ipkdb, but it
is not tested.

====================
--- src/sys/arch/powerpc/ibm4xx/trap_subr.S.orig	2016-07-26 17:22:14.111178887 +0900
+++ src/sys/arch/powerpc/ibm4xx/trap_subr.S	2016-07-26 17:50:49.971311997 +0900
@@ -205,7 +205,27 @@
 _C_LABEL(ddblow):
 	mtsprg1	%r1			/* save SP */
 	GET_CPUINFO(%r1)
-	stmw	%r28,CI_DDBSAVE(%r1)	/* free r28-r31 */
+	stmw	%r30,(CI_DDBSAVE+CPUSAVE_R30)(%r1); /* free r30-r31 */
+	mfcr	%r30;			/* save CR */
+	mfsrr1	%r31;
+	mtcr	%r31;
+	bf	MSR_PR,1f		/* branch if privileged */
+	mtcr	%r30;			/* restore CR */
+	lmw	%r30,(CI_DDBSAVE+CPUSAVE_R30)(%r1); /* restore r30-r31 */
+	stmw	%r28,(CI_TEMPSAVE+CPUSAVE_R28)(%r1); /* free r28-r31 */
+	mflr	%r28;			/* save LR */
+	mfcr	%r29;			/* save CR */
+	mfsrr0	%r30;
+	mfsrr1	%r31;
+	stmw	%r30,(CI_TEMPSAVE+CPUSAVE_SRR0)(%r1); /* save srr0/srr1 */
+	mfsprg1	%r1;			/* restore SP */
+	GET_PCB(%r1);
+	addi	%r1,%r1,USPACE-CALLFRAMELEN; /* stack is top of user struct */
+	bla	s_trap;
+1:
+	mtcr	%r30;			/* restore CR */
+	lmw	%r30,(CI_DDBSAVE+CPUSAVE_R30)(%r1);	/* restore r30-r31 */
+	stmw	%r28,(CI_DDBSAVE+CPUSAVE_R28)(%r1)	/* free r28-r31 */
 	mflr	%r28			/* save LR */
 	mfcr	%r29			/* save CR */
 	mfsrr0	%r30
@@ -228,7 +248,28 @@
 _C_LABEL(ipkdblow):
 	mtsprg1	%r1			/* save SP */
 	GET_CPUINFO(%r1)
-	stmw	%r28,CI_IPKDBSAVE(%r1)	/* free r28-r31 */
+	stmw	%r30,(CI_IPKDBSAVE+CPUSAVE_R30)(%r1); /* free r30-r31 */
+	mfcr	%r30;			/* save CR */
+	mfsrr1	%r31;
+	mtcr	%r31;
+	bf	MSR_PR,1f		/* branch if privileged */
+	mtcr	%r30;			/* restore CR */
+	lmw	%r30,(CI_IPKDBSAVE+CPUSAVE_R30)(%r1); /* restore r30-r31 */
+	stmw	%r28,(CI_TEMPSAVE+CPUSAVE_R28)(%r1); /* free r28-r31 */
+	mflr	%r28;			/* save LR */
+	mfcr	%r29;			/* save CR */
+	mfsrr0	%r30;
+	mfsrr1	%r31;
+	stmw	%r30,(CI_TEMPSAVE+CPUSAVE_SRR0)(%r1); /* save srr0/srr1 */
+	mfsprg1	%r1;			/* restore SP */
+	GET_PCB(%r1);
+	addi	%r1,%r1,USPACE-CALLFRAMELEN; /* stack is top of user struct */
+	bla	s_trap;
+1:
+	mtcr	%r30;			/* restore CR */
+	lmw	%r30,(CI_IPKDBSAVE+CPUSAVE_R30)(%r1);	/* restore r30-r31 */
+	lwz	%r31,(CI_IPKDBSAVE+CPUSAVE_R31)(%r1);	/* restore r31 */
+	stmw	%r28,(CI_IPKDBSAVE+CPUSAVE_R28)(%r1)	/* free r28-r31 */
 	mflr	%r28			/* save LR */
 	mfcr	%r29			/* save CR */
 	mfsrr0	%r30
@@ -536,7 +577,7 @@
 	stw	%r3,(CI_DDBSAVE+CPUSAVE_SRR1)(%r4)
 	wrteei	0			/* disable interrupts */
 	isync
-	stmw	%r28,CI_DDBSAVE(%r4)
+	stmw	%r28,(CI_DDBSAVE+CPUSAVE_R28)(%r4)
 	mflr	%r28
 	stw	%r28,(CI_DDBSAVE+CPUSAVE_SRR0)(%r4)
 	li	%r29,EXC_BPT
@@ -568,7 +609,7 @@
 	stw	%r3,(CI_IPKDBSAVE+CPUSAVE_SRR1)(%r4)
 	wrteei	0			/* disable interrupts */
 	isync
-	stmw	%r28,CI_IPKDBSAVE(%r4)
+	stmw	%r28,(CI_IPKDBSAVE+CPUSAVE_R28)(%r4)
 	mflr	%r28
 	stw	%r28,(CI_IPKDBSAVE+CPUSAVE_SRR0)(%r4)
 	li	%r29,EXC_BPT
====================

Here I attached another tiny patch which enables us to compile kernel
without DDB option. It also corrects a typo and a harmless logic error.

====================
--- src/sys/arch/powerpc/fpu/fpu_emu.c.orig	2016-07-26 06:43:25.645354519 +0900
+++ src/sys/arch/powerpc/fpu/fpu_emu.c	2016-07-26 06:45:23.596533829 +0900
@@ -248,14 +248,14 @@
 	case NOTFPU:
 	default:
 		DPRINTF(FPE_EX, ("fpu_emulate: SIGILL\n"));
-#ifdef DEBUG
+#if defined(DDB) && defined(DEBUG)
 		if (fpe_debug & FPE_EX) {
 			printf("fpu_emulate:  illegal insn %x at %p:",
 			insn.i_int, (void *) (tf->tf_srr0));
 			opc_disasm((vaddr_t)(tf->tf_srr0), insn.i_int);
 		}
 #endif
-#if defined(PPC_IBM4XX) && defined(DEBUG)
+#if defined(PPC_IBM4XX) && defined(DDB) && defined(DEBUG)
 		/*
 		* XXXX retry an illegal insn once due to cache issues.
 		*/
@@ -265,7 +265,7 @@
 				Debugger();
 		}
 		lastill = tf->tf_srr0;
-#endif /* PPC_IBM4XX && DEBUG */
+#endif /* PPC_IBM4XX && DDB && DEBUG */
 		return false;
 	}
 }
--- src/sys/arch/powerpc/ibm4xx/ibm4xx_machdep.c.orig	2016-07-26 06:36:01.337826518 +0900
+++ src/sys/arch/powerpc/ibm4xx/ibm4xx_machdep.c	2016-07-26 14:58:11.074394305 +0900
@@ -75,6 +75,7 @@
 #include "opt_kgdb.h"
 #include "opt_ipkdb.h"
 #include "opt_modular.h"
+#include "ksyms.h" /* for NKSYMS */
 
 #include <sys/param.h>
 #include <sys/msgbuf.h>
@@ -133,6 +134,8 @@
 extern const uint32_t errata51handler[], errata51size;
 #ifdef DDB
 extern const uint32_t ddblow[], ddbsize;
+#endif
+#ifdef IPKDB
 extern const uint32_t ipkdblow[], ipkdbsize;
 #endif
 static const struct exc_info trap_table[] = {
@@ -147,12 +150,12 @@
 	{ EXC_DEBUG,	debugtrap,	(uintptr_t)&debugsize },
 	{ (EXC_DTMISS|EXC_ALI),
 			errata51handler, (uintptr_t)&errata51size },
-#if defined(DDB)
+#ifdef DDB
 	{ EXC_PGM,	ddblow,		(uintptr_t)&ddbsize },
-#endif /* DDB */
-#if defined(IPKBD)
+#endif
+#ifdef IPKDB
 	{ EXC_PGM,	ipkdblow,	(uintptr_t)&ipkdbsize },
-#endif /* DDB */
+#endif
 };
 
 /*
====================