[PATCH v3 0/9] powerpc: Modernize unhandled signals message

[PATCH v3 0/9] powerpc: Modernize unhandled signals message

[Murilo Opsfelder Araujo]

Hi, everyone.

This series was inspired by the need to modernize and display more
informative messages about unhandled signals.

The "unhandled signal NN" is not very informative.  We thought it would be
helpful adding a human-readable message describing what the signal number
means, printing the VMA address, and dumping the instructions.

Before this series:

  pandafault32[4724]: unhandled signal 11 at 100005e4 nip 10000444 lr 0fe31ef4 code 2

  pandafault64[4725]: unhandled signal 11 at 0000000010000718 nip 0000000010000574 lr 00007fff7faa7a6c code 2

After this series:

  pandafault32[4753]: segfault (11) at 100005e4 nip 10000444 lr fe31ef4 code 2 in pandafault32[10000000+10000]
  pandafault32[4753]: code: 4bffff3c 60000000 60420000 4bffff30 9421ffd0 93e1002c 7c3f0b78 3d201000
  pandafault32[4753]: code: 392905e4 913f0008 813f0008 39400048 <99490000> 39200000 7d234b78 397f0030

  pandafault64[4754]: segfault (11) at 10000718 nip 10000574 lr 7fffb0007a6c code 2 in pandafault64[10000000+10000]
  pandafault64[4754]: code: e8010010 7c0803a6 4bfffef4 4bfffef0 fbe1fff8 f821ffb1 7c3f0b78 3d22fffe
  pandafault64[4754]: code: 39298818 f93f0030 e93f0030 39400048 <99490000> 39200000 7d234b78 383f0050

Link to v2:

  https://lore.kernel.org/lkml/20180727145811.12334-1-muriloo@linux.ibm.com/

v2..v3:

  - Dropped patch 3
  - Updated patch 4 to use %lx

Cheers!

Murilo Opsfelder Araujo (9):
  powerpc/traps: Print unhandled signals in a separate function
  powerpc/traps: Return early in show_signal_msg()
  powerpc/traps: Use %lx format in show_signal_msg()
  powerpc/traps: Print VMA for unhandled signals
  powerpc/traps: Print signal name for unhandled signals
  powerpc: Do not call __kernel_text_address() in show_instructions()
  powerpc: Add stacktrace.h header
  powerpc/traps: Show instructions on exceptions
  powerpc/traps: Add line prefix in show_instructions()

 arch/powerpc/include/asm/stacktrace.h | 13 +++++
 arch/powerpc/kernel/process.c         | 13 +++--
 arch/powerpc/kernel/traps.c           | 72 +++++++++++++++++++++++----
 3 files changed, 83 insertions(+), 15 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

-- 
2.17.1

[PATCH v3 1/9] powerpc/traps: Print unhandled signals in a separate function

[Murilo Opsfelder Araujo]

Isolate the logic of printing unhandled signals out of _exception_pkey().
No functional change, only code rearrangement.

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0e17dcb48720..cbd3dc365193 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,26 +301,32 @@ void user_single_step_siginfo(struct task_struct *tsk,
 	info->si_addr = (void __user *)regs->nip;
 }
 
+static void show_signal_msg(int signr, struct pt_regs *regs, int code,
+			    unsigned long addr)
+{
+	const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+		"at %08lx nip %08lx lr %08lx code %x\n";
+	const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+		"at %016lx nip %016lx lr %016lx code %x\n";
+
+	if (show_unhandled_signals && unhandled_signal(current, signr)) {
+		printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+				   current->comm, current->pid, signr,
+				   addr, regs->nip, regs->link, code);
+	}
+}
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-		unsigned long addr, int key)
+		     unsigned long addr, int key)
 {
 	siginfo_t info;
-	const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-			"at %08lx nip %08lx lr %08lx code %x\n";
-	const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-			"at %016lx nip %016lx lr %016lx code %x\n";
 
 	if (!user_mode(regs)) {
 		die("Exception in kernel mode", regs, signr);
 		return;
 	}
 
-	if (show_unhandled_signals && unhandled_signal(current, signr)) {
-		printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-				   current->comm, current->pid, signr,
-				   addr, regs->nip, regs->link, code);
-	}
+	show_signal_msg(signr, regs, code, addr);
 
 	if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
 		local_irq_enable();
-- 
2.17.1

[PATCH v3 2/9] powerpc/traps: Return early in show_signal_msg()

[Murilo Opsfelder Araujo]

Modify the logic of show_signal_msg() to return early, if possible.
Replace printk_ratelimited() by printk() and a default rate limit burst to
limit displaying unhandled signals messages.

Mainly reason of this change is to improve readability of the function.
The conditions to display the message were coupled together in one single
`if` statement.

Splitting out the rate limit check outside show_signal_msg() makes it
easier to the caller decide if it wants to respect a printk rate limit or
not.

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cbd3dc365193..4faab4705774 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,6 +301,13 @@ void user_single_step_siginfo(struct task_struct *tsk,
 	info->si_addr = (void __user *)regs->nip;
 }
 
+static bool show_unhandled_signals_ratelimited(void)
+{
+	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
+				      DEFAULT_RATELIMIT_BURST);
+	return show_unhandled_signals && __ratelimit(&rs);
+}
+
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 			    unsigned long addr)
 {
@@ -309,11 +316,12 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 	const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
 		"at %016lx nip %016lx lr %016lx code %x\n";
 
-	if (show_unhandled_signals && unhandled_signal(current, signr)) {
-		printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-				   current->comm, current->pid, signr,
-				   addr, regs->nip, regs->link, code);
-	}
+	if (!unhandled_signal(current, signr))
+		return;
+
+	printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+	       current->comm, current->pid, signr,
+	       addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
@@ -326,7 +334,8 @@ void _exception_pkey(int signr, struct pt_regs *regs, int code,
 		return;
 	}
 
-	show_signal_msg(signr, regs, code, addr);
+	if (show_unhandled_signals_ratelimited())
+		show_signal_msg(signr, regs, code, addr);
 
 	if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
 		local_irq_enable();
-- 
2.17.1

[PATCH v3 3/9] powerpc/traps: Use %lx format in show_signal_msg()

[Murilo Opsfelder Araujo]

Use %lx format to print registers.  This avoids having two different
formats and avoids checking for MSR_64BIT, improving readability of the
function.

Even though we could have used %px, which is functionally equivalent to %lx
as per Documentation/core-api/printk-formats.rst, it is not semantically
correct because the data printed are not pointers.  And using %px requires
casting data to (void *).

Besides that, %lx matches the format used in show_regs().

Before this patch:

  pandafault[4808]: unhandled signal 11 at 0000000010000718 nip 0000000010000574 lr 00007fff935e7a6c code 2

After this patch:

  pandafault[4732]: unhandled signal 11 at 10000718 nip 10000574 lr 7fff86697a6c code 2

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4faab4705774..fd4e0648a2d2 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,17 +311,12 @@ static bool show_unhandled_signals_ratelimited(void)
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 			    unsigned long addr)
 {
-	const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-		"at %08lx nip %08lx lr %08lx code %x\n";
-	const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-		"at %016lx nip %016lx lr %016lx code %x\n";
-
 	if (!unhandled_signal(current, signr))
 		return;
 
-	printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-	       current->comm, current->pid, signr,
-	       addr, regs->nip, regs->link, code);
+	pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",
+		current->comm, current->pid, signr,
+		addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v3 4/9] powerpc/traps: Print VMA for unhandled signals

[Murilo Opsfelder Araujo]

This adds VMA address in the message printed for unhandled signals,
similarly to what other architectures, like x86, print.

Before this patch, a page fault looked like:

  pandafault[61470]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff8d185100 code 2

After this patch, a page fault looks like:

  pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index fd4e0648a2d2..1c4f06fca370 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -314,9 +314,13 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 	if (!unhandled_signal(current, signr))
 		return;
 
-	pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",
+	pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x",
 		current->comm, current->pid, signr,
 		addr, regs->nip, regs->link, code);
+
+	print_vma_addr(KERN_CONT " in ", regs->nip);
+
+	pr_cont("\n");
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Murilo Opsfelder Araujo]

This adds a human-readable name in the unhandled signal message.

Before this patch, a page fault looked like:

  pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]

After this patch, a page fault looks like:

  pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1c4f06fca370..e71f12bca146 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #define TM_DEBUG(x...) do { } while(0)
 #endif
 
+static const char *signames[SIGRTMIN + 1] = {
+	"UNKNOWN",
+	"SIGHUP",			// 1
+	"SIGINT",			// 2
+	"SIGQUIT",			// 3
+	"SIGILL",			// 4
+	"unhandled trap",		// 5 = SIGTRAP
+	"SIGABRT",			// 6 = SIGIOT
+	"bus error",			// 7 = SIGBUS
+	"floating point exception",	// 8 = SIGFPE
+	"illegal instruction",		// 9 = SIGILL
+	"SIGUSR1",			// 10
+	"segfault",			// 11 = SIGSEGV
+	"SIGUSR2",			// 12
+	"SIGPIPE",			// 13
+	"SIGALRM",			// 14
+	"SIGTERM",			// 15
+	"SIGSTKFLT",			// 16
+	"SIGCHLD",			// 17
+	"SIGCONT",			// 18
+	"SIGSTOP",			// 19
+	"SIGTSTP",			// 20
+	"SIGTTIN",			// 21
+	"SIGTTOU",			// 22
+	"SIGURG",			// 23
+	"SIGXCPU",			// 24
+	"SIGXFSZ",			// 25
+	"SIGVTALRM",			// 26
+	"SIGPROF",			// 27
+	"SIGWINCH",			// 28
+	"SIGIO",			// 29 = SIGPOLL = SIGLOST
+	"SIGPWR",			// 30
+	"SIGSYS",			// 31 = SIGUNUSED
+};
+
 /*
  * Trap & Exception support
  */
@@ -314,8 +349,8 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 	if (!unhandled_signal(current, signr))
 		return;
 
-	pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x",
-		current->comm, current->pid, signr,
+	pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
+		current->comm, current->pid, signames[signr], signr,
 		addr, regs->nip, regs->link, code);
 
 	print_vma_addr(KERN_CONT " in ", regs->nip);
-- 
2.17.1

[PATCH v3 6/9] powerpc: Do not call __kernel_text_address() in show_instructions()

[Murilo Opsfelder Araujo]

Modify show_instructions() not to call __kernel_text_address(), allowing
userspace instruction dump.  probe_kernel_address(), which returns -EFAULT
if something goes wrong, is still being called.

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e9533b4d2f08..50094c44bf79 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1283,8 +1283,7 @@ static void show_instructions(struct pt_regs *regs)
 			pc = (unsigned long)phys_to_virt(pc);
 #endif
 
-		if (!__kernel_text_address(pc) ||
-		     probe_kernel_address((unsigned int __user *)pc, instr)) {
+		if (probe_kernel_address((unsigned int __user *)pc, instr)) {
 			pr_cont("XXXXXXXX ");
 		} else {
 			if (regs->nip == pc)
-- 
2.17.1

[PATCH v3 7/9] powerpc: Add stacktrace.h header

[Murilo Opsfelder Araujo]

Move show_instructions() declaration to
arch/powerpc/include/asm/stacktrace.h and include asm/stracktrace.h in
arch/powerpc/kernel/process.c, which contains the implementation.

This allows show_instructions() to be called on, for example,
show_signal_msg().

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/include/asm/stacktrace.h | 13 +++++++++++++
 arch/powerpc/kernel/process.c         |  3 ++-
 2 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

diff --git a/arch/powerpc/include/asm/stacktrace.h b/arch/powerpc/include/asm/stacktrace.h
new file mode 100644
index 000000000000..217ebc52ff97
--- /dev/null
+++ b/arch/powerpc/include/asm/stacktrace.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Stack trace functions.
+ *
+ * Copyright 2018, Murilo Opsfelder Araujo, IBM Corporation.
+ */
+
+#ifndef _ASM_POWERPC_STACKTRACE_H
+#define _ASM_POWERPC_STACKTRACE_H
+
+void show_instructions(struct pt_regs *regs);
+
+#endif /* _ASM_POWERPC_STACKTRACE_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 50094c44bf79..e78799a8855a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -52,6 +52,7 @@
 #include <asm/machdep.h>
 #include <asm/time.h>
 #include <asm/runlatch.h>
+#include <asm/stacktrace.h>
 #include <asm/syscalls.h>
 #include <asm/switch_to.h>
 #include <asm/tm.h>
@@ -1261,7 +1262,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
 
 static int instructions_to_print = 16;
 
-static void show_instructions(struct pt_regs *regs)
+void show_instructions(struct pt_regs *regs)
 {
 	int i;
 	unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
-- 
2.17.1

[PATCH v3 8/9] powerpc/traps: Show instructions on exceptions

[Murilo Opsfelder Araujo]

Call show_instructions() in arch/powerpc/kernel/traps.c to dump
instructions at faulty location, useful to debugging.

Before this patch, an unhandled signal message looked like:

  pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]

After this patch, it looks like:

  pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
  Instruction dump:
  4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
  392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/traps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e71f12bca146..b27f3f71d745 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -70,6 +70,7 @@
 #include <asm/hmi.h>
 #include <sysdev/fsl_pci.h>
 #include <asm/kprobes.h>
+#include <asm/stacktrace.h>
 
 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC_CORE)
 int (*__debugger)(struct pt_regs *regs) __read_mostly;
@@ -356,6 +357,8 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 	print_vma_addr(KERN_CONT " in ", regs->nip);
 
 	pr_cont("\n");
+
+	show_instructions(regs);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v3 9/9] powerpc/traps: Add line prefix in show_instructions()

[Murilo Opsfelder Araujo]

Remove "Instruction dump:" line by adding a prefix to display current->comm
and current->pid, along with the instructions dump.

The prefix can serve as a glue that links the instructions dump to its
originator, allowing messages to be interleaved in the logs.

Before this patch, a page fault looked like:

  pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
  Instruction dump:
  4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
  392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040

After this patch, it looks like:

  pandafault[10850]: segfault (11) at 100007d0 nip 1000061c lr 7fff9f3e5100 code 2 in pandafault[10000000+10000]
  pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
  pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/kernel/process.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e78799a8855a..d12143e7d8f9 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1265,16 +1265,19 @@ static int instructions_to_print = 16;
 void show_instructions(struct pt_regs *regs)
 {
 	int i;
+	const char *prefix = KERN_INFO "%s[%d]: code: ";
 	unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
 			sizeof(int));
 
-	printk("Instruction dump:");
+	printk(prefix, current->comm, current->pid);
 
 	for (i = 0; i < instructions_to_print; i++) {
 		int instr;
 
-		if (!(i % 8))
+		if (!(i % 8) && (i > 0)) {
 			pr_cont("\n");
+			printk(prefix, current->comm, current->pid);
+		}
 
 #if !defined(CONFIG_BOOKE)
 		/* If executing with the IMMU off, adjust pc rather
-- 
2.17.1

Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Christophe LEROY]

Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> This adds a human-readable name in the unhandled signal message.
> 
> Before this patch, a page fault looked like:
> 
>    pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
> 
> After this patch, a page fault looks like:
> 
>    pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]
> 
> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> ---
>   arch/powerpc/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++++--
>   1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 1c4f06fca370..e71f12bca146 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
>   #define TM_DEBUG(x...) do { } while(0)
>   #endif
>   
> +static const char *signames[SIGRTMIN + 1] = {
> +	"UNKNOWN",
> +	"SIGHUP",			// 1
> +	"SIGINT",			// 2
> +	"SIGQUIT",			// 3
> +	"SIGILL",			// 4
> +	"unhandled trap",		// 5 = SIGTRAP
> +	"SIGABRT",			// 6 = SIGIOT
> +	"bus error",			// 7 = SIGBUS
> +	"floating point exception",	// 8 = SIGFPE
> +	"illegal instruction",		// 9 = SIGILL
> +	"SIGUSR1",			// 10
> +	"segfault",			// 11 = SIGSEGV
> +	"SIGUSR2",			// 12
> +	"SIGPIPE",			// 13
> +	"SIGALRM",			// 14
> +	"SIGTERM",			// 15
> +	"SIGSTKFLT",			// 16
> +	"SIGCHLD",			// 17
> +	"SIGCONT",			// 18
> +	"SIGSTOP",			// 19
> +	"SIGTSTP",			// 20
> +	"SIGTTIN",			// 21
> +	"SIGTTOU",			// 22
> +	"SIGURG",			// 23
> +	"SIGXCPU",			// 24
> +	"SIGXFSZ",			// 25
> +	"SIGVTALRM",			// 26
> +	"SIGPROF",			// 27
> +	"SIGWINCH",			// 28
> +	"SIGIO",			// 29 = SIGPOLL = SIGLOST
> +	"SIGPWR",			// 30
> +	"SIGSYS",			// 31 = SIGUNUSED
> +};

I don't think is is worth having that full table when we only use a few 
of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/)

I would suggest to instead use a function like this:

static const char *signame(int signr)
{
	if (signr == SIGBUS)
		return "bus error";
	if (signr == SIGFPE)
		return "floating point exception";
	if (signr == SIGILL)
		return "illegal instruction";
	if (signr == SIGILL)
		return "segfault";
	if (signr == SIGTRAP)
		return "unhandled trap";
	return "unknown signal";
}

Christophe

> +
>   /*
>    * Trap & Exception support
>    */
> @@ -314,8 +349,8 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,
>   	if (!unhandled_signal(current, signr))
>   		return;
>   
> -	pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x",
> -		current->comm, current->pid, signr,
> +	pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
> +		current->comm, current->pid, signames[signr], signr,
>   		addr, regs->nip, regs->link, code);
>   
>   	print_vma_addr(KERN_CONT " in ", regs->nip);
> 

Re: [PATCH v3 9/9] powerpc/traps: Add line prefix in show_instructions()

[Christophe LEROY]

Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> Remove "Instruction dump:" line by adding a prefix to display current->comm
> and current->pid, along with the instructions dump.
> 
> The prefix can serve as a glue that links the instructions dump to its
> originator, allowing messages to be interleaved in the logs.
> 
> Before this patch, a page fault looked like:
> 
>    pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
>    Instruction dump:
>    4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>    392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
> 
> After this patch, it looks like:
> 
>    pandafault[10850]: segfault (11) at 100007d0 nip 1000061c lr 7fff9f3e5100 code 2 in pandafault[10000000+10000]
>    pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>    pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
> 
> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>

Does the script scripts/decode_stacktrace.sh also works with this format 
change ?

> ---
>   arch/powerpc/kernel/process.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index e78799a8855a..d12143e7d8f9 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1265,16 +1265,19 @@ static int instructions_to_print = 16;
>   void show_instructions(struct pt_regs *regs)
>   {
>   	int i;
> +	const char *prefix = KERN_INFO "%s[%d]: code: ";
>   	unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
>   			sizeof(int));
>   
> -	printk("Instruction dump:");
> +	printk(prefix, current->comm, current->pid);
>   
>   	for (i = 0; i < instructions_to_print; i++) {
>   		int instr;
>   
> -		if (!(i % 8))
> +		if (!(i % 8) && (i > 0)) {
>   			pr_cont("\n");
> +			printk(prefix, current->comm, current->pid);
> +		}
>   
>   #if !defined(CONFIG_BOOKE)
>   		/* If executing with the IMMU off, adjust pc rather
> 

Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Joe Perches]

On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
> Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > This adds a human-readable name in the unhandled signal message.
> > Before this patch, a page fault looked like:
> >    pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
> > After this patch, a page fault looks like:
> >    pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]
]]
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
[]
> > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
> >   #define TM_DEBUG(x...) do { } while(0)
> >   #endif
> >   
> > +static const char *signames[SIGRTMIN + 1] = {
> > +	"UNKNOWN",
> > +	"SIGHUP",			// 1
> > +	"SIGINT",			// 2
[]
> I don't think is is worth having that full table when we only use a few 
> of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/)
> 
> I would suggest to instead use a function like this:
> 
> static const char *signame(int signr)
> {
> 	if (signr == SIGBUS)
> 		return "bus error";
> 	if (signr == SIGFPE)
> 		return "floating point exception";
> 	if (signr == SIGILL)
> 		return "illegal instruction";
> 	if (signr == SIGILL)
> 		return "segfault";
> 	if (signr == SIGTRAP)
> 		return "unhandled trap";
> 	return "unknown signal";
> }

trivia:

Unless the if tests are ordered most to least likely,
perhaps it would be better to use a switch/case and
let the compiler decide.

	switch (signr) {
	case SIGBUS:	return "bus error";
	case SIGFPE:	return "floating point exception";
	case SIGILL:	return "illegal instruction";
	case SIGSEGV:	return "segfault";
	case SIGTRAP:	return "unhandled trap";
	}
	return "unknown signal";
}

Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Segher Boessenkool]

On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
> On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
> > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > I would suggest to instead use a function like this:
> > 
> > static const char *signame(int signr)
> > {
> > 	if (signr == SIGBUS)
> > 		return "bus error";
> > 	if (signr == SIGFPE)
> > 		return "floating point exception";
> > 	if (signr == SIGILL)
> > 		return "illegal instruction";
> > 	if (signr == SIGILL)
> > 		return "segfault";
> > 	if (signr == SIGTRAP)
> > 		return "unhandled trap";
> > 	return "unknown signal";
> > }
> 
> trivia:
> 
> Unless the if tests are ordered most to least likely,
> perhaps it would be better to use a switch/case and
> let the compiler decide.

That would also show there are two entries for SIGILL (here and in the
original patch), one of them very wrong.

Check the table with psignal or something?


Segher

Re: [PATCH v3 9/9] powerpc/traps: Add line prefix in show_instructions()

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:

> Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
>> Remove "Instruction dump:" line by adding a prefix to display current->comm
>> and current->pid, along with the instructions dump.
>> 
>> The prefix can serve as a glue that links the instructions dump to its
>> originator, allowing messages to be interleaved in the logs.
>> 
>> Before this patch, a page fault looked like:
>> 
>>    pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
>>    Instruction dump:
>>    4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>>    392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
>> 
>> After this patch, it looks like:
>> 
>>    pandafault[10850]: segfault (11) at 100007d0 nip 1000061c lr 7fff9f3e5100 code 2 in pandafault[10000000+10000]
>>    pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>>    pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
>> 
>> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>
> Does the script scripts/decode_stacktrace.sh also works with this format 
> change ?

Did it work before? I've never used it.

Would be great if it did work though.

cheers

Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Murilo Opsfelder Araujo]

On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
> On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
> > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > > This adds a human-readable name in the unhandled signal message.
> > > Before this patch, a page fault looked like:
> > >    pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
> > > After this patch, a page fault looks like:
> > >    pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]
> ]]
> > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> []
> > > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
> > >   #define TM_DEBUG(x...) do { } while(0)
> > >   #endif
> > >
> > > +static const char *signames[SIGRTMIN + 1] = {
> > > +	"UNKNOWN",
> > > +	"SIGHUP",			// 1
> > > +	"SIGINT",			// 2
> []
> > I don't think is is worth having that full table when we only use a few
> > of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/)
> >
> > I would suggest to instead use a function like this:
> >
> > static const char *signame(int signr)
> > {
> > 	if (signr == SIGBUS)
> > 		return "bus error";
> > 	if (signr == SIGFPE)
> > 		return "floating point exception";
> > 	if (signr == SIGILL)
> > 		return "illegal instruction";
> > 	if (signr == SIGILL)
> > 		return "segfault";
> > 	if (signr == SIGTRAP)
> > 		return "unhandled trap";
> > 	return "unknown signal";
> > }
>
> trivia:
>
> Unless the if tests are ordered most to least likely,
> perhaps it would be better to use a switch/case and
> let the compiler decide.
>
> 	switch (signr) {
> 	case SIGBUS:	return "bus error";
> 	case SIGFPE:	return "floating point exception";
> 	case SIGILL:	return "illegal instruction";
> 	case SIGSEGV:	return "segfault";
> 	case SIGTRAP:	return "unhandled trap";
> 	}
> 	return "unknown signal";
> }
>

Hi, Joe, Christophe.

That's a nice enhancement.  I'll do that in my next respin.

Cheers
Murilo

Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Murilo Opsfelder Araujo]

Hi, Segher.

On Wed, Aug 01, 2018 at 02:49:03AM -0500, Segher Boessenkool wrote:
> On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
> > On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
> > > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > > I would suggest to instead use a function like this:
> > >
> > > static const char *signame(int signr)
> > > {
> > > 	if (signr == SIGBUS)
> > > 		return "bus error";
> > > 	if (signr == SIGFPE)
> > > 		return "floating point exception";
> > > 	if (signr == SIGILL)
> > > 		return "illegal instruction";
> > > 	if (signr == SIGILL)
> > > 		return "segfault";
> > > 	if (signr == SIGTRAP)
> > > 		return "unhandled trap";
> > > 	return "unknown signal";
> > > }
> >
> > trivia:
> >
> > Unless the if tests are ordered most to least likely,
> > perhaps it would be better to use a switch/case and
> > let the compiler decide.
>
> That would also show there are two entries for SIGILL (here and in the
> original patch), one of them very wrong.

Good catch.  I'll take care of that in my next respin.

> Check the table with psignal or something?

Nice suggestion.  Thanks!

Cheers
Murilo

Re: [PATCH v3 9/9] powerpc/traps: Add line prefix in show_instructions()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Wed, Aug 01, 2018 at 08:41:15AM +0200, Christophe LEROY wrote:
>
>
> Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > Remove "Instruction dump:" line by adding a prefix to display current->comm
> > and current->pid, along with the instructions dump.
> >
> > The prefix can serve as a glue that links the instructions dump to its
> > originator, allowing messages to be interleaved in the logs.
> >
> > Before this patch, a page fault looked like:
> >
> >    pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
> >    Instruction dump:
> >    4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
> >    392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
> >
> > After this patch, it looks like:
> >
> >    pandafault[10850]: segfault (11) at 100007d0 nip 1000061c lr 7fff9f3e5100 code 2 in pandafault[10000000+10000]
> >    pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
> >    pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
> >
> > Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>
> Does the script scripts/decode_stacktrace.sh also works with this format
> change ?

I've got more feedback from Michael about this.  A better approach would be
having a new show_user_instructions(), a slightly modified version of
show_instructions(), that can be called from within show_signal_msg().

Since _exception_pkey() dies if the exception is in kernel mode, we'll be
safe to call the new show_user_instructions(), without interfering in
scripts/decode_stacktrace.sh.

Cheers
Murilo