[PATCH v2 00/10] powerpc: Modernize unhandled signals message

[PATCH v2 00/10] 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.

We can add more informative messages, like informing what each code of a
SIGSEGV signal means.  We are open to suggestions.

Before this series:

    pandafault[5815]: unhandled signal 11 at 00000000100007d0 nip 000000001000061c lr 00003fff87ff5100 code 2

After this series:

    pandafault[10850]: segfault (11) at 00000000100007d0 nip 000000001000061c lr 00007fff9f3e5100 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

Link to v1:

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

v1..v2:

    - Broke patch 7 down into patches 7-9
    - Added proper copyright in arch/powerpc/include/asm/stacktrace.h
    - show_instructions(): prefixed lines with current->comm and current->pid

Cheers!

Murilo Opsfelder Araujo (10):
  powerpc/traps: Print unhandled signals in a separate function
  powerpc/traps: Return early in show_signal_msg()
  powerpc/reg: Add REG_FMT definition
  powerpc/traps: Use REG_FMT 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/reg.h        |  6 +++
 arch/powerpc/include/asm/stacktrace.h | 13 +++++
 arch/powerpc/kernel/process.c         | 35 ++++++-------
 arch/powerpc/kernel/traps.c           | 73 +++++++++++++++++++++++----
 4 files changed, 100 insertions(+), 27 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

--
2.17.1

[PATCH v2 01/10] 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 v2 02/10] 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 v2 03/10] powerpc/reg: Add REG_FMT definition

[Murilo Opsfelder Araujo]

Make REG definition, in arch/powerpc/kernel/process.c, generic enough by
renaming it to REG_FMT and placing it in arch/powerpc/include/asm/reg.h to
be used elsewhere.

Replace occurrences of REG by REG_FMT in arch/powerpc/kernel/process.c.

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/include/asm/reg.h |  6 ++++++
 arch/powerpc/kernel/process.c  | 22 ++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 858aa7984ab0..d6c5c77383de 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1319,6 +1319,12 @@
 #define PVR_ARCH_207	0x0f000004
 #define PVR_ARCH_300	0x0f000005
 
+#ifdef CONFIG_PPC64
+#define REG_FMT		"%016lx"
+#else
+#define REG_FMT		"%08lx"
+#endif /* CONFIG_PPC64 */
+
 /* Macros for setting and retrieving special purpose registers */
 #ifndef __ASSEMBLY__
 #define mfmsr()		({unsigned long rval; \
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e9533b4d2f08..25b562c21b7b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1381,11 +1381,9 @@ static void print_msr_bits(unsigned long val)
 }
 
 #ifdef CONFIG_PPC64
-#define REG		"%016lx"
 #define REGS_PER_LINE	4
 #define LAST_VOLATILE	13
 #else
-#define REG		"%08lx"
 #define REGS_PER_LINE	8
 #define LAST_VOLATILE	12
 #endif
@@ -1396,21 +1394,21 @@ void show_regs(struct pt_regs * regs)
 
 	show_regs_print_info(KERN_DEFAULT);
 
-	printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",
+	printk("NIP:  "REG_FMT" LR: "REG_FMT" CTR: "REG_FMT"\n",
 	       regs->nip, regs->link, regs->ctr);
 	printk("REGS: %px TRAP: %04lx   %s  (%s)\n",
 	       regs, regs->trap, print_tainted(), init_utsname()->release);
-	printk("MSR:  "REG" ", regs->msr);
+	printk("MSR:  "REG_FMT" ", regs->msr);
 	print_msr_bits(regs->msr);
-	pr_cont("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
+	pr_cont("  CR: "REG_FMT"  XER: "REG_FMT"\n", regs->ccr, regs->xer);
 	trap = TRAP(regs);
 	if ((TRAP(regs) != 0xc00) && cpu_has_feature(CPU_FTR_CFAR))
-		pr_cont("CFAR: "REG" ", regs->orig_gpr3);
+		pr_cont("CFAR: "REG_FMT" ", regs->orig_gpr3);
 	if (trap == 0x200 || trap == 0x300 || trap == 0x600)
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
-		pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
+		pr_cont("DEAR: "REG_FMT" ESR: "REG_FMT" ", regs->dar, regs->dsisr);
 #else
-		pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
+		pr_cont("DAR: "REG_FMT" DSISR: "REG_FMT" ", regs->dar, regs->dsisr);
 #endif
 #ifdef CONFIG_PPC64
 	pr_cont("IRQMASK: %lx ", regs->softe);
@@ -1423,7 +1421,7 @@ void show_regs(struct pt_regs * regs)
 	for (i = 0;  i < 32;  i++) {
 		if ((i % REGS_PER_LINE) == 0)
 			pr_cont("\nGPR%02d: ", i);
-		pr_cont(REG " ", regs->gpr[i]);
+		pr_cont(REG_FMT " ", regs->gpr[i]);
 		if (i == LAST_VOLATILE && !FULL_REGS(regs))
 			break;
 	}
@@ -1433,8 +1431,8 @@ void show_regs(struct pt_regs * regs)
 	 * Lookup NIP late so we have the best change of getting the
 	 * above info out without failing
 	 */
-	printk("NIP ["REG"] %pS\n", regs->nip, (void *)regs->nip);
-	printk("LR ["REG"] %pS\n", regs->link, (void *)regs->link);
+	printk("NIP ["REG_FMT"] %pS\n", regs->nip, (void *)regs->nip);
+	printk("LR ["REG_FMT"] %pS\n", regs->link, (void *)regs->link);
 #endif
 	show_stack(current, (unsigned long *) regs->gpr[1]);
 	if (!user_mode(regs))
@@ -2038,7 +2036,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 		newsp = stack[0];
 		ip = stack[STACK_FRAME_LR_SAVE];
 		if (!firstframe || ip != lr) {
-			printk("["REG"] ["REG"] %pS", sp, ip, (void *)ip);
+			printk("["REG_FMT"] ["REG_FMT"] %pS", sp, ip, (void *)ip);
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 			if ((ip == rth) && curr_frame >= 0) {
 				pr_cont(" (%pS)",
-- 
2.17.1

[PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Murilo Opsfelder Araujo]

Simplify the message format by using REG_FMT as the register format.  This
avoids having two different formats and avoids checking for MSR_64BIT.

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

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4faab4705774..047d980ac776 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,17 +311,13 @@ 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 "REG_FMT \
+		" nip "REG_FMT" lr "REG_FMT" 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

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[LEROY Christophe]

Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :

> Simplify the message format by using REG_FMT as the register format.  This
> avoids having two different formats and avoids checking for MSR_64BIT.

Are you sure it is what we want ?

Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?

Christophe


>
> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> ---
>  arch/powerpc/kernel/traps.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 4faab4705774..047d980ac776 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -311,17 +311,13 @@ 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 "REG_FMT \
> +		" nip "REG_FMT" lr "REG_FMT" 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

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Joe Perches]

On Fri, 2018-07-27 at 18:40 +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
> 
> > Simplify the message format by using REG_FMT as the register format.  This
> > avoids having two different formats and avoids checking for MSR_64BIT.
> 
> Are you sure it is what we want ?
> 
> Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?

[]

> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
[]
> > @@ -311,17 +311,13 @@ 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 "REG_FMT \

I think it better to use a space after the close "
and also the line continuation is unnecessary.

> > +		" nip "REG_FMT" lr "REG_FMT" code %x\n",

And spaces before the open quotes too.

I'd also prefer the format on a single line:

	pr_info("%s[%d]: unhandled signal %d at " REG_FMT " nip " REG_FMT " lr " REG_FMT " code %x\n",

> > +		current->comm, current->pid, signr, addr,
> > +		regs->nip, regs->link, code);

Seeing as these are all unsigned long, a better way to do
this is to use %p and cast to pointer.

This might be better anyway as this output exposes pointer
addresses and instead would now use pointer hashed output.

	pr_info("%s[%d]: unhandled signal %d at %p nip %p lr %p code %x\n",
		current->comm, current->pid, signr,
		(void *)addr, (void *)regs->nip, (void *)regs->link, code);

Use %px if you _really_ need to emit unhashed addresses.

see: Documentation/core-api/printk-formats.rst

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
>
> > Simplify the message format by using REG_FMT as the register format.  This
> > avoids having two different formats and avoids checking for MSR_64BIT.
>
> Are you sure it is what we want ?

Yes.

> Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?

In fact, this changes how many zeroes are prefixed when displaying the registers
(%016lx vs. %08lx format).  For example, 32-bits userspace, 64-bits kernel:

before this series:
  [66475.002900] segv[4599]: unhandled signal 11 at 00000000 nip 10000420 lr 0fe61854 code 1

after this series:
  [   73.414535] segv[3759]: segfault (11) at 0000000000000000 nip 0000000010000420 lr 000000000fe61854 code 1 in segv[10000000+10000]
  [   73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6 4bffff30 9421ffd0 93e1002c 7c3f0b78
  [   73.414665] segv[3759]: code: 39200000 913f001c 813f001c 39400001 <91490000> 39200000 7d234b78 397f0030

Have you spotted any other behaviour change?

Cheers
Murilo

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[LEROY Christophe]

Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :

> Hi, Christophe.
>
> On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote:
>> Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
>>
>> > Simplify the message format by using REG_FMT as the register format.  This
>> > avoids having two different formats and avoids checking for MSR_64BIT.
>>
>> Are you sure it is what we want ?
>
> Yes.
>
>> Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?
>
> In fact, this changes how many zeroes are prefixed when displaying  
> the registers
> (%016lx vs. %08lx format).  For example, 32-bits userspace, 64-bits kernel:

Indeed that's what I suspected. What is the real benefit of this  
change ? Why not keep the current format for 32bits userspace ? All  
those leading zeroes are pointless to me.

>
> before this series:
>   [66475.002900] segv[4599]: unhandled signal 11 at 00000000 nip  
> 10000420 lr 0fe61854 code 1
>
> after this series:
>   [   73.414535] segv[3759]: segfault (11) at 0000000000000000 nip  
> 0000000010000420 lr 000000000fe61854 code 1 in segv[10000000+10000]
>   [   73.414641] segv[3759]: code: 4e800421 80010014 38210010  
> 7c0803a6 4bffff30 9421ffd0 93e1002c 7c3f0b78
>   [   73.414665] segv[3759]: code: 39200000 913f001c 813f001c  
> 39400001 <91490000> 39200000 7d234b78 397f0030
>
> Have you spotted any other behaviour change?

Not as of today

Christophe

>
> Cheers
> Murilo

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Mon, Jul 30, 2018 at 06:30:47PM +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
>
> > Hi, Christophe.
> >
> > On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote:
> > > Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
> > >
> > > > Simplify the message format by using REG_FMT as the register format.  This
> > > > avoids having two different formats and avoids checking for MSR_64BIT.
> > >
> > > Are you sure it is what we want ?
> >
> > Yes.
> >
> > > Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?
> >
> > In fact, this changes how many zeroes are prefixed when displaying the
> > registers
> > (%016lx vs. %08lx format).  For example, 32-bits userspace, 64-bits kernel:
>
> Indeed that's what I suspected. What is the real benefit of this change ?
> Why not keep the current format for 32bits userspace ? All those leading
> zeroes are pointless to me.

One of the benefits is simplifying the code by removing some checks.  Another is
deduplicating almost identical format strings in favor of a unified one.

After reading Joe's comment [1], %px seems to be the format we're looking for.
An extract from Documentation/core-api/printk-formats.rst:

  "%px is functionally equivalent to %lx (or %lu). %px is preferred because it
  is more uniquely grep'able."

So I guess we don't need to worry about the format (%016lx vs. %08lx), let's
just use %px, as per the guideline.

[1] https://lore.kernel.org/lkml/26f07092cdde378ebb42c1034badde1b56521c36.camel@perches.com/

Cheers
Murilo

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Michael Ellerman]

Murilo Opsfelder Araujo <muriloo@linux.ibm.com> writes:
> On Mon, Jul 30, 2018 at 06:30:47PM +0200, LEROY Christophe wrote:
>> Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
>> > On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote:
>> > > Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
>> > >
>> > > > Simplify the message format by using REG_FMT as the register format.  This
>> > > > avoids having two different formats and avoids checking for MSR_64BIT.
>> > >
>> > > Are you sure it is what we want ?
>> >
>> > Yes.
>> >
>> > > Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?
>> >
>> > In fact, this changes how many zeroes are prefixed when displaying the
>> > registers
>> > (%016lx vs. %08lx format).  For example, 32-bits userspace, 64-bits kernel:
>>
>> Indeed that's what I suspected. What is the real benefit of this change ?
>> Why not keep the current format for 32bits userspace ? All those leading
>> zeroes are pointless to me.
>
> One of the benefits is simplifying the code by removing some checks.  Another is
> deduplicating almost identical format strings in favor of a unified one.
>
> After reading Joe's comment [1], %px seems to be the format we're looking for.
> An extract from Documentation/core-api/printk-formats.rst:
>
>   "%px is functionally equivalent to %lx (or %lu). %px is preferred because it
>   is more uniquely grep'able."
>
> So I guess we don't need to worry about the format (%016lx vs. %08lx), let's
> just use %px, as per the guideline.

I don't think I like %px.

It makes the format string cleaner, but it means we have to cast
everything to void * which is ugly as heck.

I actually don't think the leading zeroes are helpful at all in the
signal message, ie. we should just use %lx there.

They are useful in show_regs() because we want everything to line up.

So I think I'll drop patch 3 and use 0x%lx in show_signal_msg(), meaning
we end up with, eg:

  [   73.414535] segv[3759]: segfault (11) at 0x0 nip 0x10000420 lr 0xfe61854 code 0x1 in segv[10000000+10000]
  [   73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6 4bffff30 9421ffd0 93e1002c 7c3f0b78
  [   73.414665] segv[3759]: code: 39200000 913f001c 813f001c 39400001 <91490000> 39200000 7d234b78 397f0030


I'll do that unless anyone screams loudly, because it would be nice to
get this into 4.19.

cheers

RE: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Alastair D'Silva]

> -----Original Message-----
> From: Michael Ellerman <mpe@ellerman.id.au>
> Sent: Tuesday, 31 July 2018 7:32 PM
> To: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>; LEROY Christophe
> <christophe.leroy@c-s.fr>
> Cc: linux-kernel@vger.kernel.org; Alastair D'Silva <alastair@d-silva.org>;
> Andrew Donnellan <andrew.donnellan@au1.ibm.com>; Balbir Singh
> <bsingharora@gmail.com>; Benjamin Herrenschmidt
> <benh@kernel.crashing.org>; Cyril Bur <cyrilbur@gmail.com>; Eric W .
> Biederman <ebiederm@xmission.com>; Joe Perches <joe@perches.com>;
> Michael Neuling <mikey@neuling.org>; Nicholas Piggin
> <npiggin@gmail.com>; Paul Mackerras <paulus@samba.org>; Simon Guo
> <wei.guo.simon@gmail.com>; Sukadev Bhattiprolu
> <sukadev@linux.vnet.ibm.com>; Tobin C . Harding <me@tobin.cc>; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in
> show_signal_msg()
> 
> Murilo Opsfelder Araujo <muriloo@linux.ibm.com> writes:
> > On Mon, Jul 30, 2018 at 06:30:47PM +0200, LEROY Christophe wrote:
> >> Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
> >> > On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote:
> >> > > Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit :
> >> > >
> >> > > > Simplify the message format by using REG_FMT as the register
> >> > > > format.  This avoids having two different formats and avoids
> checking for MSR_64BIT.
> >> > >
> >> > > Are you sure it is what we want ?
> >> >
> >> > Yes.
> >> >
> >> > > Won't it change the behaviour for a 32 bits app running on a 64bits
> kernel ?
> >> >
> >> > In fact, this changes how many zeroes are prefixed when displaying
> >> > the registers (%016lx vs. %08lx format).  For example, 32-bits
> >> > userspace, 64-bits kernel:
> >>
> >> Indeed that's what I suspected. What is the real benefit of this change ?
> >> Why not keep the current format for 32bits userspace ? All those
> >> leading zeroes are pointless to me.
> >
> > One of the benefits is simplifying the code by removing some checks.
> > Another is deduplicating almost identical format strings in favor of a unified
> one.
> >
> > After reading Joe's comment [1], %px seems to be the format we're
> looking for.
> > An extract from Documentation/core-api/printk-formats.rst:
> >
> >   "%px is functionally equivalent to %lx (or %lu). %px is preferred because it
> >   is more uniquely grep'able."
> >
> > So I guess we don't need to worry about the format (%016lx vs. %08lx),
> > let's just use %px, as per the guideline.
> 
> I don't think I like %px.

Me neither, semantically, it's for pointers, and the data being displayed is not a pointer.

> It makes the format string cleaner, but it means we have to cast everything
> to void * which is ugly as heck.
> 
> I actually don't think the leading zeroes are helpful at all in the signal
> message, ie. we should just use %lx there.
> 
> They are useful in show_regs() because we want everything to line up.
> 
> So I think I'll drop patch 3 and use 0x%lx in show_signal_msg(), meaning we
> end up with, eg:
> 
>   [   73.414535] segv[3759]: segfault (11) at 0x0 nip 0x10000420 lr 0xfe61854
> code 0x1 in segv[10000000+10000]
>   [   73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6
> 4bffff30 9421ffd0 93e1002c 7c3f0b78
>   [   73.414665] segv[3759]: code: 39200000 913f001c 813f001c 39400001
> <91490000> 39200000 7d234b78 397f0030

Or better yet, "%#lx" - the hash adds the appropriate prefix in the right case for the format.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

[PATCH v2 05/10] 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 00000000100007d0 nip 000000001000061c lr 00007fff8d185100 code 2

After this patch, a page fault looks like:

    pandafault[6303]: unhandled signal 11 at 00000000100007d0 nip 000000001000061c lr 00007fff93c55100 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 047d980ac776..e6c43ef9fb50 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -315,9 +315,13 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 		return;
 
 	pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
-		" nip "REG_FMT" lr "REG_FMT" code %x\n",
+		" nip "REG_FMT" lr "REG_FMT" 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 v2 06/10] 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 00000000100007d0 nip 000000001000061c lr 00007fff93c55100 code 2 in pandafault[10000000+10000]

After this patch, a page fault looks like:

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

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

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e6c43ef9fb50..e55ee639d010 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,10 +349,10 @@ 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 "REG_FMT \
-		" nip "REG_FMT" lr "REG_FMT" code %x",
-		current->comm, current->pid, signr, addr,
-		regs->nip, regs->link, code);
+	pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \
+		" lr "REG_FMT" 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 v2 07/10] 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 25b562c21b7b..04960796fcce 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 v2 08/10] 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 04960796fcce..709bfb524b84 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 v2 09/10] 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 00000000100007d0 nip 000000001000061c lr 00007fffbd295100 code 2 in pandafault[10000000+10000]

After this patch, it looks like:

    pandafault[10524]: segfault (11) at 00000000100007d0 nip 000000001000061c lr 00007fffbd295100 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 e55ee639d010..3beca17ac1b1 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;
@@ -357,6 +358,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 v2 10/10] 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 00000000100007d0 nip 000000001000061c lr 00007fffbd295100 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 00000000100007d0 nip 000000001000061c lr 00007fff9f3e5100 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 709bfb524b84..25b6dfc8dd81 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