Message ID | 20180724192720.32417-8-muriloo@linux.ibm.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | powerpc: Modernize unhandled signals message | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
snowpatch_ozlabs/checkpatch | warning | Test checkpatch on branch next |
snowpatch_ozlabs/build-ppc64le | warning | Test build-ppc64le on branch next |
snowpatch_ozlabs/build-ppc64be | warning | Test build-ppc64be on branch next |
snowpatch_ozlabs/build-ppc64e | warning | Test build-ppc64e on branch next |
snowpatch_ozlabs/build-ppc32 | warning | Test build-ppc32 on branch next |
Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit : > 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. > > 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. > > Call show_instructions() in arch/powerpc/kernel/traps.c to dump > instructions at > faulty location, useful to debugging. Shouldn't this part be in a second patch ? Wouldn't it be better to also see regs in addition if we want to use this to understand what happened ? So you could call show_regs() instead of show_instructions() ? Christophe > > Before this patch, an unhandled signal message looked like: > > Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault > (11) at 00000000100007d0 nip 000000001000061c lr 00007fffbd295100 > code 2 in pandafault[10000000+10000] > > After this patch, it looks like: > > Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault > (11) at 00000000100007d0 nip 000000001000061c lr 00007fffbd295100 > code 2 in pandafault[10000000+10000] > Jul 24 09:57:00 localhost kernel: Instruction dump: > Jul 24 09:57:00 localhost kernel: 4bfffeec 4bfffee8 3c401002 > 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe > Jul 24 09:57:00 localhost kernel: 392988d0 f93f0020 e93f0020 > 39400048 <99490000> 39200000 7d234b78 383f0040 > > Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > --- > arch/powerpc/include/asm/stacktrace.h | 7 +++++++ > arch/powerpc/kernel/process.c | 6 +++--- > arch/powerpc/kernel/traps.c | 3 +++ > 3 files changed, 13 insertions(+), 3 deletions(-) > 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..46e5ef451578 > --- /dev/null > +++ b/arch/powerpc/include/asm/stacktrace.h > @@ -0,0 +1,7 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#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 b1af3390249c..ee1d63e03c52 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 * > @@ -1283,8 +1284,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) > 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
Hi, Christophe. On Wed, Jul 25, 2018 at 06:01:34PM +0200, LEROY Christophe wrote: > Murilo Opsfelder Araujo <muriloo@linux.ibm.com> a écrit : > > > 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. > > > > 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. > > > > Call show_instructions() in arch/powerpc/kernel/traps.c to dump > > instructions at > > faulty location, useful to debugging. > > Shouldn't this part be in a second patch ? Makes sense. Perhaps I should split this patch in two: one to remove __kernel_text_address() check in show_instructions(), and another to call show_instructions() in show_signal_msg(). > Wouldn't it be better to also see regs in addition if we want to use this to > understand what happened ? > So you could call show_regs() instead of show_instructions() ? I see that show_regs() prints more information and calls show_instructions() at the end if in privileged state. I'm not sure about which situations we might want to call show_regs() - and display a bunch of information - or just dump instructions for some signals. Isn't calling show_regs() in this case considered overkill? Cheers Murilo
diff --git a/arch/powerpc/include/asm/stacktrace.h b/arch/powerpc/include/asm/stacktrace.h new file mode 100644 index 000000000000..46e5ef451578 --- /dev/null +++ b/arch/powerpc/include/asm/stacktrace.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#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 b1af3390249c..ee1d63e03c52 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 * @@ -1283,8 +1284,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) 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,
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. 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. 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: Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault (11) at 00000000100007d0 nip 000000001000061c lr 00007fffbd295100 code 2 in pandafault[10000000+10000] After this patch, it looks like: Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault (11) at 00000000100007d0 nip 000000001000061c lr 00007fffbd295100 code 2 in pandafault[10000000+10000] Jul 24 09:57:00 localhost kernel: Instruction dump: Jul 24 09:57:00 localhost kernel: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe Jul 24 09:57:00 localhost kernel: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040 Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> --- arch/powerpc/include/asm/stacktrace.h | 7 +++++++ arch/powerpc/kernel/process.c | 6 +++--- arch/powerpc/kernel/traps.c | 3 +++ 3 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 arch/powerpc/include/asm/stacktrace.h