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/[email protected]/
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
Isolate the logic of printing unhandled signals out of _exception_pkey().
No functional change, only code rearrangement.
Signed-off-by: Murilo Opsfelder Araujo <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]>
> > ---
> > 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
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);
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 <[email protected]>
>>> ---
>>> 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
>
Christophe LEROY <[email protected]> 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
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
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 <[email protected]>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/658b0f92bc7003bc734471f61bf7cd
cheers
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
>
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