[PATCH v4 0/6] powerpc: Modernize unhandled signals message

[PATCH v4 0/6] 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 v3:

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

v3..v4:

  - Added new show_user_instructions() based on the existing
    show_instructions()
  - Updated commit messages
  - Replaced signals names table with a tiny function that returns a
    literal string for each signal number

Cheers!

Murilo Opsfelder Araujo (6):
  powerpc/traps: Print unhandled signals in a separate function
  powerpc/traps: Use an explicit ratelimit state for show_signal_msg()
  powerpc/traps: Use %lx format in show_signal_msg()
  powerpc/traps: Print VMA for unhandled signals
  powerpc: Add show_user_instructions()
  powerpc/traps: Show instructions on exceptions

 arch/powerpc/include/asm/stacktrace.h | 13 +++++++
 arch/powerpc/kernel/process.c         | 40 +++++++++++++++++++++
 arch/powerpc/kernel/traps.c           | 52 +++++++++++++++++++++------
 3 files changed, 95 insertions(+), 10 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

-- 
2.17.1

[PATCH v4 1/6] 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

Re: [v4, 1/6] powerpc/traps: Print unhandled signals in a separate function

[Michael Ellerman]

On Wed, 2018-08-01 at 21:33:15 UTC, Murilo Opsfelder Araujo wrote:
> 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>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/658b0f92bc7003bc734471f61bf7cd

cheers

[PATCH v4 2/6] powerpc/traps: Use an explicit ratelimit state for show_signal_msg()

[Murilo Opsfelder Araujo]

Replace printk_ratelimited() by printk() and a default rate limit
burst to limit displaying unhandled signals messages.

This will allow us to call print_vma_addr() in a future patch, which
does not work with printk_ratelimited().

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

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cbd3dc365193..367534b41617 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,15 @@ 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;
+
+	if (!show_unhandled_signals_ratelimited())
+		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,
-- 
2.17.1

[PATCH v4 3/6] 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 367534b41617..4503e22f6ba5 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,20 +311,15 @@ 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;
 
 	if (!show_unhandled_signals_ratelimited())
 		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 v4 4/6] 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]: segfault 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 | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4503e22f6ba5..bcefbb1ee771 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -96,6 +96,19 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #define TM_DEBUG(x...) do { } while(0)
 #endif
 
+static const char *signame(int signr)
+{
+	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";
+}
+
 /*
  * Trap & Exception support
  */
@@ -317,9 +330,13 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,
 	if (!show_unhandled_signals_ratelimited())
 		return;
 
-	pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",
-		current->comm, current->pid, signr,
+	pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
+		current->comm, current->pid, signame(signr), 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 v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

show_user_instructions() is a slightly modified version of
show_instructions() that allows userspace instruction dump.

This will be useful within show_signal_msg() to dump userspace
instructions of the faulty location.

Here is a sample of what show_user_instructions() outputs:

  pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
  pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040

The current->comm and current->pid printed can serve as a glue that
links the instructions dump to its originator, allowing messages to be
interleaved in the logs.

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
 arch/powerpc/include/asm/stacktrace.h | 13 +++++++++
 arch/powerpc/kernel/process.c         | 40 +++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)
 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..6149b53b3bc8
--- /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_user_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 e9533b4d2f08..364645ac732c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
 	pr_cont("\n");
 }
 
+void show_user_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(prefix, current->comm, current->pid);
+
+	for (i = 0; i < instructions_to_print; i++) {
+		int instr;
+
+		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
+		 * than print XXXXXXXX.
+		 */
+		if (!(regs->msr & MSR_IR))
+			pc = (unsigned long)phys_to_virt(pc);
+#endif
+
+		if (probe_kernel_address((unsigned int __user *)pc, instr)) {
+			pr_cont("XXXXXXXX ");
+		} else {
+			if (regs->nip == pc)
+				pr_cont("<%08x> ", instr);
+			else
+				pr_cont("%08x ", instr);
+		}
+
+		pc += sizeof(int);
+	}
+
+	pr_cont("\n");
+}
+
 struct regbit {
 	unsigned long bit;
 	const char *name;
-- 
2.17.1

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Christophe LEROY]

Le 01/08/2018 à 23:33, Murilo Opsfelder Araujo a écrit :
> show_user_instructions() is a slightly modified version of
> show_instructions() that allows userspace instruction dump.
> 
> This will be useful within show_signal_msg() to dump userspace
> instructions of the faulty location.
> 
> Here is a sample of what show_user_instructions() outputs:
> 
>    pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>    pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
> 
> The current->comm and current->pid printed can serve as a glue that
> links the instructions dump to its originator, allowing messages to be
> interleaved in the logs.
> 
> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/stacktrace.h | 13 +++++++++
>   arch/powerpc/kernel/process.c         | 40 +++++++++++++++++++++++++++
>   2 files changed, 53 insertions(+)
>   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..6149b53b3bc8
> --- /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_user_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 e9533b4d2f08..364645ac732c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
>   	pr_cont("\n");
>   }
>   
> +void show_user_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(prefix, current->comm, current->pid);

Why not use pr_info() and remove KERN_INFO from *prefix ?

> +
> +	for (i = 0; i < instructions_to_print; i++) {
> +		int instr;
> +
> +		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
> +		 * than print XXXXXXXX.
> +		 */
> +		if (!(regs->msr & MSR_IR))
> +			pc = (unsigned long)phys_to_virt(pc);

Shouldn't this be done outside of the loop, only once ?

Christophe

> +#endif
> +
> +		if (probe_kernel_address((unsigned int __user *)pc, instr)) {
> +			pr_cont("XXXXXXXX ");
> +		} else {
> +			if (regs->nip == pc)
> +				pr_cont("<%08x> ", instr);
> +			else
> +				pr_cont("%08x ", instr);
> +		}
> +
> +		pc += sizeof(int);
> +	}
> +
> +	pr_cont("\n");
> +}
> +
>   struct regbit {
>   	unsigned long bit;
>   	const char *name;
> 

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Thu, Aug 02, 2018 at 07:26:20AM +0200, Christophe LEROY wrote:
>
>
> Le 01/08/2018 à 23:33, Murilo Opsfelder Araujo a écrit :
> > show_user_instructions() is a slightly modified version of
> > show_instructions() that allows userspace instruction dump.
> >
> > This will be useful within show_signal_msg() to dump userspace
> > instructions of the faulty location.
> >
> > Here is a sample of what show_user_instructions() outputs:
> >
> >    pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
> >    pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
> >
> > The current->comm and current->pid printed can serve as a glue that
> > links the instructions dump to its originator, allowing messages to be
> > interleaved in the logs.
> >
> > Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> > ---
> >   arch/powerpc/include/asm/stacktrace.h | 13 +++++++++
> >   arch/powerpc/kernel/process.c         | 40 +++++++++++++++++++++++++++
> >   2 files changed, 53 insertions(+)
> >   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..6149b53b3bc8
> > --- /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_user_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 e9533b4d2f08..364645ac732c 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
> >   	pr_cont("\n");
> >   }
> > +void show_user_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(prefix, current->comm, current->pid);
>
> Why not use pr_info() and remove KERN_INFO from *prefix ?

Because it doesn't compile:

  arch/powerpc/kernel/process.c:1317:10: error: expected ‘)’ before ‘prefix’
    pr_info(prefix, current->comm, current->pid);
            ^
  ./include/linux/printk.h:288:21: note: in definition of macro ‘pr_fmt’
   #define pr_fmt(fmt) fmt
                     ^

`pr_info(prefix, ...)` expands to `printk("\001" "6" prefix, ...)`,
which is an invalid string concatenation.

`pr_info("%s", ...)` expands to `printk("\001" "6" "%s", ...)`, which is
valid.

> > +
> > +	for (i = 0; i < instructions_to_print; i++) {
> > +		int instr;
> > +
> > +		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
> > +		 * than print XXXXXXXX.
> > +		 */
> > +		if (!(regs->msr & MSR_IR))
> > +			pc = (unsigned long)phys_to_virt(pc);
>
> Shouldn't this be done outside of the loop, only once ?

I don't think so.

pc gets incremented at the bottom of the loop:

  pc += sizeof(int);

Adjusting pc is necessary at each iteration.  Leaving this block inside
the loop seems correct.

Cheers
Murilo

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Joe Perches]

On Thu, 2018-08-02 at 21:42 -0300, Murilo Opsfelder Araujo wrote:
> > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
[]
> > > @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
> > >   	pr_cont("\n");
> > >   }
> > > +void show_user_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(prefix, current->comm, current->pid);
> > 
> > Why not use pr_info() and remove KERN_INFO from *prefix ?
> 
> Because it doesn't compile:
> 
>   arch/powerpc/kernel/process.c:1317:10: error: expected ‘)’ before ‘prefix’
>     pr_info(prefix, current->comm, current->pid);
>             ^
>   ./include/linux/printk.h:288:21: note: in definition of macro ‘pr_fmt’
>    #define pr_fmt(fmt) fmt
>                      ^

What being suggested is using:

	pr_info("%s[%d]: code: ", current->comm, current->pid);

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Christophe LEROY]

Hi Murilo,

Le 03/08/2018 à 02:42, Murilo Opsfelder Araujo a écrit :
> Hi, Christophe.
> 
> On Thu, Aug 02, 2018 at 07:26:20AM +0200, Christophe LEROY wrote:
>>
>>
>> Le 01/08/2018 à 23:33, Murilo Opsfelder Araujo a écrit :
>>> show_user_instructions() is a slightly modified version of
>>> show_instructions() that allows userspace instruction dump.
>>>
>>> This will be useful within show_signal_msg() to dump userspace
>>> instructions of the faulty location.
>>>
>>> Here is a sample of what show_user_instructions() outputs:
>>>
>>>     pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>>>     pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
>>>
>>> The current->comm and current->pid printed can serve as a glue that
>>> links the instructions dump to its originator, allowing messages to be
>>> interleaved in the logs.
>>>
>>> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>>> ---
>>>    arch/powerpc/include/asm/stacktrace.h | 13 +++++++++
>>>    arch/powerpc/kernel/process.c         | 40 +++++++++++++++++++++++++++
>>>    2 files changed, 53 insertions(+)
>>>    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..6149b53b3bc8
>>> --- /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_user_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 e9533b4d2f08..364645ac732c 100644
>>> --- a/arch/powerpc/kernel/process.c
>>> +++ b/arch/powerpc/kernel/process.c
>>> @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
>>>    	pr_cont("\n");
>>>    }
>>> +void show_user_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(prefix, current->comm, current->pid);
>>
>> Why not use pr_info() and remove KERN_INFO from *prefix ?
> 
> Because it doesn't compile:
> 
>    arch/powerpc/kernel/process.c:1317:10: error: expected ‘)’ before ‘prefix’
>      pr_info(prefix, current->comm, current->pid);
>              ^
>    ./include/linux/printk.h:288:21: note: in definition of macro ‘pr_fmt’
>     #define pr_fmt(fmt) fmt
>                       ^
> 
> `pr_info(prefix, ...)` expands to `printk("\001" "6" prefix, ...)`,
> which is an invalid string concatenation.
> 
> `pr_info("%s", ...)` expands to `printk("\001" "6" "%s", ...)`, which is
> valid.

Then what about using directly:

pr_info("%s[%d]: code: ", ...);

> 
>>> +
>>> +	for (i = 0; i < instructions_to_print; i++) {
>>> +		int instr;
>>> +
>>> +		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
>>> +		 * than print XXXXXXXX.
>>> +		 */
>>> +		if (!(regs->msr & MSR_IR))
>>> +			pc = (unsigned long)phys_to_virt(pc);
>>
>> Shouldn't this be done outside of the loop, only once ?
> 
> I don't think so.
> 
> pc gets incremented at the bottom of the loop:
> 
>    pc += sizeof(int);
> 
> Adjusting pc is necessary at each iteration.  Leaving this block inside
> the loop seems correct.

This looks pretty strange.
The first time, pc is a physical address, that you change to a virtual 
address. Then when you increment it it is still a virtual address.
So when you call phys_to_virt(pc) for the second time, pc is already a 
virt address, so what happens indeed ?

Christophe

> 
> Cheers
> Murilo
> 

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 03/08/2018 à 02:42, Murilo Opsfelder Araujo a écrit :
>> Hi, Christophe.
>> On Thu, Aug 02, 2018 at 07:26:20AM +0200, Christophe LEROY wrote:
>>> Le 01/08/2018 à 23:33, Murilo Opsfelder Araujo a écrit :
>>>> show_user_instructions() is a slightly modified version of
>>>> show_instructions() that allows userspace instruction dump.
>>>>
>>>> This will be useful within show_signal_msg() to dump userspace
>>>> instructions of the faulty location.
>>>>
>>>> Here is a sample of what show_user_instructions() outputs:
>>>>
>>>>     pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>>>>     pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
>>>>
>>>> The current->comm and current->pid printed can serve as a glue that
>>>> links the instructions dump to its originator, allowing messages to be
>>>> interleaved in the logs.
>>>>
>>>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>>>> index e9533b4d2f08..364645ac732c 100644
>>>> --- a/arch/powerpc/kernel/process.c
>>>> +++ b/arch/powerpc/kernel/process.c
>>>> @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
>>>>    	pr_cont("\n");
>>>>    }
>>>> +void show_user_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(prefix, current->comm, current->pid);
>>>
>>> Why not use pr_info() and remove KERN_INFO from *prefix ?
>> 
>> Because it doesn't compile:
>> 
>>    arch/powerpc/kernel/process.c:1317:10: error: expected ‘)’ before ‘prefix’
>>      pr_info(prefix, current->comm, current->pid);
>>              ^
>>    ./include/linux/printk.h:288:21: note: in definition of macro ‘pr_fmt’
>>     #define pr_fmt(fmt) fmt
>>                       ^
>> 
>> `pr_info(prefix, ...)` expands to `printk("\001" "6" prefix, ...)`,
>> which is an invalid string concatenation.
>> 
>> `pr_info("%s", ...)` expands to `printk("\001" "6" "%s", ...)`, which is
>> valid.
>
> Then what about using directly:
>
> pr_info("%s[%d]: code: ", ...);

Yeah that's better, I'll fix it up when applying.

>>>> +#if !defined(CONFIG_BOOKE)
>>>> +		/* If executing with the IMMU off, adjust pc rather
>>>> +		 * than print XXXXXXXX.
>>>> +		 */
>>>> +		if (!(regs->msr & MSR_IR))
>>>> +			pc = (unsigned long)phys_to_virt(pc);
>>>
>>> Shouldn't this be done outside of the loop, only once ?
>> 
>> I don't think so.
>> 
>> pc gets incremented at the bottom of the loop:
>> 
>>    pc += sizeof(int);
>> 
>> Adjusting pc is necessary at each iteration.  Leaving this block inside
>> the loop seems correct.
>
> This looks pretty strange.
> The first time, pc is a physical address, that you change to a virtual 
> address. Then when you increment it it is still a virtual address.
> So when you call phys_to_virt(pc) for the second time, pc is already a 
> virt address, so what happens indeed ?

Yeah that's a bit fishy.

On 64-bit it works because phys_to_virt() == __va() which is:

  #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET))

ie. it uses bitwise or, so __va(__va(x)) == __va(x).

But it looks like on 32-bit it's going to do the wrong thing. Do we ever
actually hit that case though, I'm not sure?


However for this patch I'll just remove the whole thing, because we
don't expect to be dumping user instructions in realmode.

cheers

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

Hi, everyone.

I'd like to thank you all that contributed to refining and making this
series better.  I did appreciate.

Thank you!

Cheers
Murilo

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Christophe LEROY]

Le 03/08/2018 à 13:31, Murilo Opsfelder Araujo a écrit :
> Hi, everyone.
> 
> I'd like to thank you all that contributed to refining and making this
> series better.  I did appreciate.
> 
> Thank you!

You are welcome.

It seems that nested segfaults don't print very well. The code line is 
split in two, le second half being alone on one line. Any way to avoid 
that ?

[    4.365317] init[1]: segfault (11) at 0 nip 0 lr 0 code 1
[    4.370452] init[1]: code: XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[    4.372042] init[74]: segfault (11) at 10a74 nip 1000c198 lr 100078c8 
code 1 in sh[10000000+14000]
[    4.386829] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[    4.391542] init[1]: code: XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX 
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[    4.400863] init[74]: code: 90010024 bf61000c 91490a7c 3fa01002 
3be00000 7d3e4b78 3bbd0c20 3b600000
[    4.409867] init[74]: code: 3b9d0040 7c7fe02e 2f830000 419e0028 
<89230000> 2f890000 41be001c 4b7f6e79

Christophe

> 
> Cheers
> Murilo
> 

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Fri, Aug 10, 2018 at 11:29:43AM +0200, Christophe LEROY wrote:
>
>
> Le 03/08/2018 à 13:31, Murilo Opsfelder Araujo a écrit :
> > Hi, everyone.
> >
> > I'd like to thank you all that contributed to refining and making this
> > series better.  I did appreciate.
> >
> > Thank you!
>
> You are welcome.
>
> It seems that nested segfaults don't print very well. The code line is split
> in two, le second half being alone on one line. Any way to avoid that ?

What do you mean by "nested segfaults"?  I'd guess it's related to nested
virtualization, e.g.: host (kvm hv) -> guest1 (kvm pr) -> guest2?  So the
segfault would have been originated in guest2?

Can you provide us with more details about how to reproduce that?

Anyway, is "XXXXXXXX" so important that it needs to be printed at all?  Can't we
just discard it and not print it?

Murilo

[PATCH v4 6/6] powerpc/traps: Show instructions on exceptions

[Murilo Opsfelder Araujo]

Call show_user_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]
  pandafault[10524]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
  pandafault[10524]: code: 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 bcefbb1ee771..8494b0ff4904 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;
@@ -337,6 +338,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_user_instructions(regs);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1