On ppc64le we HAVE_RELIABLE_STACKTRACE

PATCH Superseded Torsten Duwe 5 participants 12 messages Mar 5, 2018 – May 10, 2018 PowerPC

lore

Share & tools

lore

Torsten Duwe

author

2018-03-05 16:49 UTC #0 lore raw

PATCH v2 1/1 On ppc64le we HAVE_RELIABLE_STACKTRACE

The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:

[...] There are several rules that must be adhered to in order to ensure reliable and consistent call chain backtracing:

* Before a function calls any other function, it shall establish its own stack frame, whose size shall be a multiple of 16 bytes.

– In instances where a function’s prologue creates a stack frame, the back-chain word of the stack frame shall be updated atomically with the value of the stack pointer (r1) when a back chain is implemented. (This must be supported as default by all ELF V2 ABI-compliant environments.)
[...]
– The function shall save the link register that contains its return address in the LR save doubleword of its caller’s stack frame before calling another function.

To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE. This patch may be unneccessarily limited to ppc64le, but OTOH the only user of this flag so far is livepatching, which is only implemented on PPCs with 64-LE, a.k.a. ELF ABI v2.

This change also implements save_stack_trace_tsk_reliable() for ppc64 that checks for the above conditions, where possible.

Signed-off-by Torsten Duwe <[email protected]>

---
v2:
* implemented save_stack_trace_tsk_reliable(), with a bunch of sanity checks. The test for a kernel code pointer is much nicer now, and the exit condition is exact (when compared to last week's follow-up)

---

arch/powerpc/Kconfig +1 -0

index 73ce5dd07642..9f49913e19e3 100644
@@ -220,6 +220,7 @@ config PPC
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_RCU_TABLE_FREE		if SMP
 	select HAVE_REGS_AND_STACK_ACCESS_API
+	select HAVE_RELIABLE_STACKTRACE		if PPC64 && CPU_LITTLE_ENDIAN
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_VIRT_CPU_ACCOUNTING
 	select HAVE_IRQ_TIME_ACCOUNTING

arch/powerpc/kernel/stacktrace.c +77 -0

88 lines not on this page · raw message

Segher Boessenkool

2018-03-05 17:09 UTC #1 lore raw

Quoted 8 lines · Torsten Duwe

The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
[...] There are several rules that must be adhered to in order to ensure reliable and consistent call chain backtracing:
* Before a function calls any other function, it shall establish its own stack frame, whose size shall be a multiple of 16 bytes.

Quoted 9 lines

– In instances where a function’s prologue creates a stack frame, the back-chain word of the stack frame shall be updated atomically with the value of the stack pointer (r1) when a back chain is implemented. (This must be supported as default by all ELF V2 ABI-compliant environments.)
[...]
– The function shall save the link register that contains its return address in the LR save doubleword of its caller’s stack frame before calling another function.

All of this is also true for the other PowerPC ABIs, fwiw (both 32-bit and 64-bit; the offset of the LR save slot isn't the same in all ABIs).

Segher

Josh Poimboeuf

2018-03-08 16:26 UTC #2 lore raw

In reply to Torsten Duwe

Quoted 23 lines · Torsten Duwe

The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
[...] There are several rules that must be adhered to in order to ensure reliable and consistent call chain backtracing:
* Before a function calls any other function, it shall establish its own stack frame, whose size shall be a multiple of 16 bytes.
– In instances where a function’s prologue creates a stack frame, the back-chain word of the stack frame shall be updated atomically with the value of the stack pointer (r1) when a back chain is implemented. (This must be supported as default by all ELF V2 ABI-compliant environments.)
[...]
– The function shall save the link register that contains its return address in the LR save doubleword of its caller’s stack frame before calling another function.
To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE. This patch may be unneccessarily limited to ppc64le, but OTOH the only user of this flag so far is livepatching, which is only implemented on PPCs with 64-LE, a.k.a. ELF ABI v2.

Quoted 4 lines

This change also implements save_stack_trace_tsk_reliable() for ppc64 that checks for the above conditions, where possible.
Signed-off-by Torsten Duwe <[email protected]>

This doesn't seem to address some of my previous concerns:

- Bailing on interrupt/exception frames

- Function graph tracing return address conversion

- kretprobes return address conversion

Signature · 1 line

Josh

Torsten Duwe

author

2018-03-09 16:47 UTC #3 lore raw

Quoted 1 line · On Thu, 8 Mar 2018 10:26:16 -0600 Josh Poimboeuf <[email protected]> wrote:

This doesn't seem to address some of my previous concerns:

You're right. That discussion quickly headed towards objtool and I forgot about this one paragraph with the remarks.

Quoted 1 line

- Bailing on interrupt/exception frames

That is a good question. My current code keeps unwinding as long as the trace looks sane. If the exception frame has a valid code pointer in the LR slot it will continue. Couldn't there be cases where this is desirable? Should this be configurable? Not that I have an idea how this situation could occur for a thread that is current or sleeping...

Michael, Balbir: is that possible? Any Idea how to reliably detect an exception frame? My approach would be to look at the next return address and compare it to the usual suspects (i.e. collect all "b ret" addresses in the EXCEPTION_COMMON macro, for BookS).

Quoted 3 lines

- Function graph tracing return address conversion
- kretprobes return address conversion

You mean like in arch/x86/kernel/unwind_frame.c the call to ftrace_graph_ret_addr ?

Forgive me my directness but I don't see why these should be handled in arch-dependent code, other than maybe a hook, if inevitable, that calls back into the graph tracer / kretprobes in order to get the proper address, or simply call the trace unreliable in case it finds such a return address.

I understand that x86 has a hard time with its multiple unwinders, but there should be a simpler, generic solution for all other architectures.

Torsten

Josh Poimboeuf

2018-03-12 15:35 UTC #4 lore raw

Quoted 8 lines · Torsten Duwe

Quoted 1 line · On Thu, 8 Mar 2018 10:26:16 -0600 Josh Poimboeuf <[email protected]> wrote:
This doesn't seem to address some of my previous concerns:
You're right. That discussion quickly headed towards objtool and I forgot about this one paragraph with the remarks.

Quoted 6 lines

Quoted 1 line
- Bailing on interrupt/exception frames
That is a good question. My current code keeps unwinding as long as the trace looks sane. If the exception frame has a valid code pointer in the LR slot it will continue. Couldn't there be cases where this is desirable?

I thought we established in the previous discussion that this could cause some functions to get skipped in the stack trace:

https://lkml.kernel.org/r/20171219214652.u7qeb7fxov62ttke@treble

Quoted 3 lines

Should this be configurable? Not that
I have an idea how this situation could occur for a thread that is current or sleeping...

Page faults and preemption.

Quoted 4 lines

Michael, Balbir: is that possible? Any Idea how to reliably detect an exception frame? My approach would be to look at the next return address and compare it to the usual suspects (i.e. collect all "b ret" addresses in the EXCEPTION_COMMON macro, for BookS).

It looks like show_stack() already knows how to do this:

		/*
		 * See if this is an exception frame.
		 * We look for the "regshere" marker in the current frame.
		 */
		if (validate_sp(sp, tsk, STACK_INT_FRAME_SIZE)
		    && stack[STACK_FRAME_MARKER] == STACK_FRAME_REGS_MARKER) {

So you could do something similar.

Quoted 11 lines

Quoted 3 lines
- Function graph tracing return address conversion
- kretprobes return address conversion
You mean like in arch/x86/kernel/unwind_frame.c the call to ftrace_graph_ret_addr ?
Forgive me my directness but I don't see why these should be handled in arch-dependent code, other than maybe a hook, if inevitable, that calls back into the graph tracer / kretprobes in order to get the proper address,

I don't really follow, where exactly would you propose calling ftrace_graph_ret_addr() from?

Quoted 2 lines

or simply call the trace unreliable in case it finds such a return address.

If you're going to make livepatch incompatible with function graph tracing, there needs to be a good justification for it (and we'd need to make sure existing users are fine with it).

Signature · 1 line

Josh

Torsten Duwe

author

2018-05-04 12:35 UTC #5 lore raw

PATCH v3 1/1 On ppc64le we HAVE_RELIABLE_STACKTRACE

The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:

[...] There are several rules that must be adhered to in order to ensure reliable and consistent call chain backtracing:

* Before a function calls any other function, it shall establish its own stack frame, whose size shall be a multiple of 16 bytes.

Feel free to add other ppc variants, but so far only ppc64le got tested.

This change also implements save_stack_trace_tsk_reliable() for ppc64le that checks for the above conditions, where possible.

Signed-off-by Torsten Duwe <[email protected]>

Signed-off-by Nicolai Stange <[email protected]>

---
v3:
* big bunch of fixes, credits go to Nicolai Stange:
- get the correct return address from the graph tracer, should it be active.
IMO this should be moved into to generic code, but I'll leave it like this for now, to get things going.
- bail out on a kretprobe
- also stop at an exception frame
- accomodate for the different stack layout of the idle task
- use an even more beautiful test: __kernel_text_address()

v2:
* implemented save_stack_trace_tsk_reliable(), with a bunch of sanity checks. The test for a kernel code pointer is much nicer now, and the exit condition is exact (when compared to last week's follow-up)

---

arch/powerpc/Kconfig +1 -0

index c32a181a7cbb..54f1daf4f9e5 100644
@@ -220,6 +220,7 @@ config PPC
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_RCU_TABLE_FREE		if SMP
 	select HAVE_REGS_AND_STACK_ACCESS_API
+	select HAVE_RELIABLE_STACKTRACE		if PPC64 && CPU_LITTLE_ENDIAN
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_VIRT_CPU_ACCOUNTING
 	select HAVE_IRQ_TIME_ACCOUNTING

arch/powerpc/kernel/stacktrace.c +117 -1

138 lines not on this page · raw message

Josh Poimboeuf

Acked-by on v3 1/1

2018-05-07 15:42 UTC #6 lore raw

Quoted 24 lines · Torsten Duwe

The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
[...] There are several rules that must be adhered to in order to ensure reliable and consistent call chain backtracing:
* Before a function calls any other function, it shall establish its own stack frame, whose size shall be a multiple of 16 bytes.
– In instances where a function’s prologue creates a stack frame, the back-chain word of the stack frame shall be updated atomically with the value of the stack pointer (r1) when a back chain is implemented. (This must be supported as default by all ELF V2 ABI-compliant environments.)
[...]
– The function shall save the link register that contains its return address in the LR save doubleword of its caller’s stack frame before calling another function.
To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE. This patch may be unneccessarily limited to ppc64le, but OTOH the only user of this flag so far is livepatching, which is only implemented on PPCs with 64-LE, a.k.a. ELF ABI v2.

Quoted 7 lines

Feel free to add other ppc variants, but so far only ppc64le got tested.
This change also implements save_stack_trace_tsk_reliable() for ppc64le that checks for the above conditions, where possible.
Signed-off-by Torsten Duwe <[email protected]>
Signed-off-by Nicolai Stange <[email protected]>

The subject doesn't actively describe what the patch does, maybe change it to something like:

powerpc: Add support for HAVE_RELIABLE_STACKTRACE

or maybe

powerpc: Add support for livepatch consistency model

Otherwise it looks great to me.

Acked-by Josh Poimboeuf <[email protected]>

Signature · 1 line

Josh

Torsten Duwe

author

2018-05-08 08:38 UTC #7 lore raw

Re: [PATCH v3] ppc64le livepatch: implement reliable stacktrace for newer consistency models

Quoted 8 lines · On Mon, 7 May 2018 10:42:08 -0500 Josh Poimboeuf <[email protected]> wrote:

The subject doesn't actively describe what the patch does, maybe change it to something like:
powerpc: Add support for HAVE_RELIABLE_STACKTRACE
or maybe
powerpc: Add support for livepatch consistency model

Maybe $SUBJECT? You're absolutely right, the old subject was just a leftover of my original attempt to just set the flag, before Miroslav corrected me. I just kept on copying it without a second thought. Thanks for noting.

Quoted 4 lines

Otherwise it looks great to me.
Acked-by Josh Poimboeuf <[email protected]>

Thanks.

Torsten

Josh Poimboeuf

2018-05-09 00:14 UTC #8 lore raw

Re: [PATCH v3] ppc64le livepatch: implement reliable stacktrace for newer consistency models

Quoted 8 lines · Torsten Duwe

Quoted 5 lines · On Mon, 7 May 2018 10:42:08 -0500 Josh Poimboeuf <[email protected]> wrote:
The subject doesn't actively describe what the patch does, maybe change it to something like:
powerpc: Add support for HAVE_RELIABLE_STACKTRACE

Quoted 8 lines

Quoted 3 lines
or maybe
powerpc: Add support for livepatch consistency model
Maybe $SUBJECT? You're absolutely right, the old subject was just a leftover of my original attempt to just set the flag, before Miroslav corrected me. I just kept on copying it without a second thought. Thanks for noting.

Generally we refer to it as "the consistency model". So I would propose a minor edit:

ppc64le/livepatch: Implement reliable stack tracing for the consistency model

Quoted 8 lines

Quoted 4 lines
Otherwise it looks great to me.
Acked-by Josh Poimboeuf <[email protected]>
Thanks.
Torsten

Signature · 1 line

Josh

Michael Ellerman

reviewer

2018-05-09 01:41 UTC #9 lore raw

Re: [PATCH v3] ppc64le livepatch: implement reliable stacktrace for newer consistency models

Quoted 18 lines · Josh Poimboeuf

Quoted 8 lines · Torsten Duwe
Quoted 5 lines · On Mon, 7 May 2018 10:42:08 -0500 Josh Poimboeuf <[email protected]> wrote:
The subject doesn't actively describe what the patch does, maybe change it to something like:
powerpc: Add support for HAVE_RELIABLE_STACKTRACE
Quoted 8 lines
Quoted 3 lines
or maybe
powerpc: Add support for livepatch consistency model
Maybe $SUBJECT? You're absolutely right, the old subject was just a leftover of my original attempt to just set the flag, before Miroslav corrected me. I just kept on copying it without a second thought. Thanks for noting.

Quoted 4 lines

Generally we refer to it as "the consistency model". So I would propose a minor edit:
ppc64le/livepatch: Implement reliable stack tracing for the consistency model

We use "powerpc" as the prefix.

So I've used:

powerpc/livepatch: Implement reliable stack tracing for the consistency model

cheers

Torsten Duwe

author

2018-05-09 10:31 UTC #10 lore raw

Re: [PATCH v3] ppc64le livepatch: implement reliable stacktrace for newer consistency models

Quoted 3 lines · Michael Ellerman

Quoted 1 line · Josh Poimboeuf
Generally we refer to it as "the consistency model".

[...]

Quoted 6 lines

We use "powerpc" as the prefix.
So I've used:
powerpc/livepatch: Implement reliable stack tracing for the consistency model

Perfect. Thanks!

Torsten

Michael Ellerman

Applied

2018-05-10 14:06 UTC #11 lore raw

In reply to Torsten Duwe

Quoted 23 lines · Torsten Duwe

The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
[...] There are several rules that must be adhered to in order to ensure reliable and consistent call chain backtracing:
* Before a function calls any other function, it shall establish its own stack frame, whose size shall be a multiple of 16 bytes.
– In instances where a function’s prologue creates a stack frame, the back-chain word of the stack frame shall be updated atomically with the value of the stack pointer (r1) when a back chain is implemented. (This must be supported as default by all ELF V2 ABI-compliant environments.)
[...]
– The function shall save the link register that contains its return address in the LR save doubleword of its caller’s stack frame before calling another function.
To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE. This patch may be unneccessarily limited to ppc64le, but OTOH the only user of this flag so far is livepatching, which is only implemented on PPCs with 64-LE, a.k.a. ELF ABI v2.

Quoted 8 lines

Feel free to add other ppc variants, but so far only ppc64le got tested.
This change also implements save_stack_trace_tsk_reliable() for ppc64le that checks for the above conditions, where possible.
Signed-off-by Torsten Duwe <[email protected]>
Signed-off-by Nicolai Stange <[email protected]>
Acked-by Josh Poimboeuf <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/df78d3f6148092d33a9a24c7a9cfac

cheers