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 v2:
https://lore.kernel.org/lkml/20180727145811.12334-1-muriloo@linux.ibm.com/
v2..v3:
- Dropped patch 3
- Updated patch 4 to use %lx
Cheers!
Murilo Opsfelder Araujo (9):
powerpc/traps: Print unhandled signals in a separate function
powerpc/traps: Return early in show_signal_msg()
powerpc/traps: Use %lx format in show_signal_msg()
powerpc/traps: Print VMA for unhandled signals
powerpc/traps: Print signal name for unhandled signals
powerpc: Do not call __kernel_text_address() in show_instructions()
powerpc: Add stacktrace.h header
powerpc/traps: Show instructions on exceptions
powerpc/traps: Add line prefix in show_instructions()
arch/powerpc/include/asm/stacktrace.h | 13 +++++
arch/powerpc/kernel/process.c | 13 +++--
arch/powerpc/kernel/traps.c | 72 +++++++++++++++++++++++----
3 files changed, 83 insertions(+), 15 deletions(-)
create mode 100644 arch/powerpc/include/asm/stacktrace.h
--
2.17.1
Modify the logic of show_signal_msg() to return early, if possible.
Replace printk_ratelimited() by printk() and a default rate limit burst to
limit displaying unhandled signals messages.
Mainly reason of this change is to improve readability of the function.
The conditions to display the message were coupled together in one single
`if` statement.
Splitting out the rate limit check outside show_signal_msg() makes it
easier to the caller decide if it wants to respect a printk rate limit or
not.
Signed-off-by: Murilo Opsfelder Araujo <redacted>
---
arch/powerpc/kernel/traps.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
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 <redacted>
---
arch/powerpc/kernel/traps.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
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]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
Signed-off-by: Murilo Opsfelder Araujo <redacted>
---
arch/powerpc/kernel/traps.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
@@ -314,9 +314,13 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code,if(!unhandled_signal(current,signr))return;-pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",+pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x",current->comm,current->pid,signr,addr,regs->nip,regs->link,code);++print_vma_addr(KERN_CONT" in ",regs->nip);++pr_cont("\n");}void_exception_pkey(intsignr,structpt_regs*regs,intcode,
This adds a human-readable name in the unhandled signal message.
Before this patch, a page fault looked like:
pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
After this patch, a page fault looks like:
pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]
Signed-off-by: Murilo Opsfelder Araujo <redacted>
---
arch/powerpc/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
Move show_instructions() declaration to
arch/powerpc/include/asm/stacktrace.h and include asm/stracktrace.h in
arch/powerpc/kernel/process.c, which contains the implementation.
This allows show_instructions() to be called on, for example,
show_signal_msg().
Signed-off-by: Murilo Opsfelder Araujo <redacted>
---
arch/powerpc/include/asm/stacktrace.h | 13 +++++++++++++
arch/powerpc/kernel/process.c | 3 ++-
2 files changed, 15 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/stacktrace.h
Remove "Instruction dump:" line by adding a prefix to display current->comm
and current->pid, along with the instructions dump.
The prefix can serve as a glue that links the instructions dump to its
originator, allowing messages to be interleaved in the logs.
Before this patch, a page fault looked like:
pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
Instruction dump:
4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
After this patch, it looks like:
pandafault[10850]: segfault (11) at 100007d0 nip 1000061c lr 7fff9f3e5100 code 2 in pandafault[10000000+10000]
pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
Signed-off-by: Murilo Opsfelder Araujo <redacted>
---
arch/powerpc/kernel/process.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
@@ -1265,16 +1265,19 @@ static int instructions_to_print = 16;voidshow_instructions(structpt_regs*regs){inti;+constchar*prefix=KERN_INFO"%s[%d]: code: ";unsignedlongpc=regs->nip-(instructions_to_print*3/4*sizeof(int));-printk("Instruction dump:");+printk(prefix,current->comm,current->pid);for(i=0;i<instructions_to_print;i++){intinstr;-if(!(i%8))+if(!(i%8)&&(i>0)){pr_cont("\n");+printk(prefix,current->comm,current->pid);+}#if !defined(CONFIG_BOOKE)/* If executing with the IMMU off, adjust pc rather
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
quoted hunk
This adds a human-readable name in the unhandled signal message.
Before this patch, a page fault looked like:
pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
After this patch, a page fault looks like:
pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]
Signed-off-by: Murilo Opsfelder Araujo <redacted>
---
arch/powerpc/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
I don't think is is worth having that full table when we only use a few
of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/)
I would suggest to instead use a function like this:
static const char *signame(int signr)
{
if (signr == SIGBUS)
return "bus error";
if (signr == SIGFPE)
return "floating point exception";
if (signr == SIGILL)
return "illegal instruction";
if (signr == SIGILL)
return "segfault";
if (signr == SIGTRAP)
return "unhandled trap";
return "unknown signal";
}
Christophe
quoted hunk
+
/*
* Trap & Exception support
*/
@@ -314,8 +349,8 @@ static void show_signal_msg(int signr, struct pt_regs *regs, int code, if (!unhandled_signal(current, signr)) return;- pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x",- current->comm, current->pid, signr,+ pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",+ current->comm, current->pid, signames[signr], signr, addr, regs->nip, regs->link, code); print_vma_addr(KERN_CONT " in ", regs->nip);
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
Remove "Instruction dump:" line by adding a prefix to display current->comm
and current->pid, along with the instructions dump.
The prefix can serve as a glue that links the instructions dump to its
originator, allowing messages to be interleaved in the logs.
Before this patch, a page fault looked like:
pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
Instruction dump:
4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
After this patch, it looks like:
pandafault[10850]: segfault (11) at 100007d0 nip 1000061c lr 7fff9f3e5100 code 2 in pandafault[10000000+10000]
pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
Signed-off-by: Murilo Opsfelder Araujo <redacted>
Does the script scripts/decode_stacktrace.sh also works with this format
change ?
@@ -1265,16 +1265,19 @@ static int instructions_to_print = 16;voidshow_instructions(structpt_regs*regs){inti;+constchar*prefix=KERN_INFO"%s[%d]: code: ";unsignedlongpc=regs->nip-(instructions_to_print*3/4*sizeof(int));-printk("Instruction dump:");+printk(prefix,current->comm,current->pid);for(i=0;i<instructions_to_print;i++){intinstr;-if(!(i%8))+if(!(i%8)&&(i>0)){pr_cont("\n");+printk(prefix,current->comm,current->pid);+}#if !defined(CONFIG_BOOKE)/* If executing with the IMMU off, adjust pc rather
From: Joe Perches <joe@perches.com> Date: 2018-08-01 07:03:59
On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
quoted
This adds a human-readable name in the unhandled signal message.
Before this patch, a page fault looked like:
pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
After this patch, a page fault looks like:
pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]
I don't think is is worth having that full table when we only use a few
of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/)
I would suggest to instead use a function like this:
static const char *signame(int signr)
{
if (signr == SIGBUS)
return "bus error";
if (signr == SIGFPE)
return "floating point exception";
if (signr == SIGILL)
return "illegal instruction";
if (signr == SIGILL)
return "segfault";
if (signr == SIGTRAP)
return "unhandled trap";
return "unknown signal";
}
trivia:
Unless the if tests are ordered most to least likely,
perhaps it would be better to use a switch/case and
let the compiler decide.
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";
}
On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
quoted
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
I would suggest to instead use a function like this:
static const char *signame(int signr)
{
if (signr == SIGBUS)
return "bus error";
if (signr == SIGFPE)
return "floating point exception";
if (signr == SIGILL)
return "illegal instruction";
if (signr == SIGILL)
return "segfault";
if (signr == SIGTRAP)
return "unhandled trap";
return "unknown signal";
}
trivia:
Unless the if tests are ordered most to least likely,
perhaps it would be better to use a switch/case and
let the compiler decide.
That would also show there are two entries for SIGILL (here and in the
original patch), one of them very wrong.
Check the table with psignal or something?
Segher
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-08-01 14:14:56
Christophe LEROY [off-list ref] writes:
Le 31/07/2018 =C3=A0 16:50, Murilo Opsfelder Araujo a =C3=A9crit=C2=A0:
quoted
Remove "Instruction dump:" line by adding a prefix to display current->c=
omm
quoted
and current->pid, along with the instructions dump.
=20
The prefix can serve as a glue that links the instructions dump to its
originator, allowing messages to be interleaved in the logs.
=20
Before this patch, a page fault looked like:
=20
pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd29=
On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
quoted
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
quoted
This adds a human-readable name in the unhandled signal message.
Before this patch, a page fault looked like:
pandafault[6303]: unhandled signal 11 at 100007d0 nip 1000061c lr 7fff93c55100 code 2 in pandafault[10000000+10000]
After this patch, a page fault looks like:
pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 code 2 in pandafault[13a2a0000+10000]
I don't think is is worth having that full table when we only use a few
of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/)
I would suggest to instead use a function like this:
static const char *signame(int signr)
{
if (signr == SIGBUS)
return "bus error";
if (signr == SIGFPE)
return "floating point exception";
if (signr == SIGILL)
return "illegal instruction";
if (signr == SIGILL)
return "segfault";
if (signr == SIGTRAP)
return "unhandled trap";
return "unknown signal";
}
trivia:
Unless the if tests are ordered most to least likely,
perhaps it would be better to use a switch/case and
let the compiler decide.
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";
}
Hi, Joe, Christophe.
That's a nice enhancement. I'll do that in my next respin.
Cheers
Murilo
Hi, Segher.
On Wed, Aug 01, 2018 at 02:49:03AM -0500, Segher Boessenkool wrote:
On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
quoted
On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
quoted
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
I would suggest to instead use a function like this:
static const char *signame(int signr)
{
if (signr == SIGBUS)
return "bus error";
if (signr == SIGFPE)
return "floating point exception";
if (signr == SIGILL)
return "illegal instruction";
if (signr == SIGILL)
return "segfault";
if (signr == SIGTRAP)
return "unhandled trap";
return "unknown signal";
}
trivia:
Unless the if tests are ordered most to least likely,
perhaps it would be better to use a switch/case and
let the compiler decide.
That would also show there are two entries for SIGILL (here and in the
original patch), one of them very wrong.
Good catch. I'll take care of that in my next respin.
Hi, Christophe.
On Wed, Aug 01, 2018 at 08:41:15AM +0200, Christophe LEROY wrote:
Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
quoted
Remove "Instruction dump:" line by adding a prefix to display current->comm
and current->pid, along with the instructions dump.
The prefix can serve as a glue that links the instructions dump to its
originator, allowing messages to be interleaved in the logs.
Before this patch, a page fault looked like:
pandafault[10524]: segfault (11) at 100007d0 nip 1000061c lr 7fffbd295100 code 2 in pandafault[10000000+10000]
Instruction dump:
4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
After this patch, it looks like:
pandafault[10850]: segfault (11) at 100007d0 nip 1000061c lr 7fff9f3e5100 code 2 in pandafault[10000000+10000]
pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
Signed-off-by: Murilo Opsfelder Araujo <redacted>
Does the script scripts/decode_stacktrace.sh also works with this format
change ?
I've got more feedback from Michael about this. A better approach would be
having a new show_user_instructions(), a slightly modified version of
show_instructions(), that can be called from within show_signal_msg().
Since _exception_pkey() dies if the exception is in kernel mode, we'll be
safe to call the new show_user_instructions(), without interfering in
scripts/decode_stacktrace.sh.
Cheers
Murilo