| Message ID | 20180801213320.11203-6-muriloo@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 88b0fe17573592a8e3196bf143f865da460178e7 |
| 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 |
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; >
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
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 <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 >
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
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
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
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;
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