| Message ID | 20171212113912.GA1907@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | On ppc64le we HAVE_RELIABLE_STACKTRACE | expand |
On Tue, 12 Dec 2017, Torsten Duwe wrote: > Hi all, > > 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. > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > --- > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index c51e6ce42e7a..3e3a6ab2e089 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -218,6 +218,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 I think that this is not enough. You need to also implement save_stack_trace_tsk_reliable() for powerpc defined as __weak in kernel/stacktrace.c. See arch/x86/kernel/stacktrace.c for reference, but I think it would be much much simpler here given the changelog description. Thanks, Miroslav
On Tue, Dec 12, 2017 at 01:12:37PM +0100, Miroslav Benes wrote: > > I think that this is not enough. You need to also implement > save_stack_trace_tsk_reliable() for powerpc defined as __weak in > kernel/stacktrace.c. See arch/x86/kernel/stacktrace.c for reference, but I > think it would be much much simpler here given the changelog description. Is there an exhaustive, definite, non-x86-centric description of the cases this function needs to look for? Maybe some sort of formal specification? Torsten
On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: > Hi all, > > 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. What about leaf functions? If a leaf function doesn't establish a stack frame, and it has inline asm which contains a blr to another function, this ABI is broken. Also, even for non-leaf functions, is it possible for GCC to insert the inline asm before it sets up the stack frame? (This is an occasional problem on x86.) Also, what about hand-coded asm? > 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. In addition to fixing the above issues, the unwinder also needs to detect interrupts (i.e., preemption) and page faults on the stack of a blocked task. If a function were preempted before it created a stack frame, or if a leaf function blocked on a page fault, the stack trace will skip the function's caller, so such a trace will need to be reported to livepatch as unreliable. Furthermore, the "reliable" unwinder needs to have a way to report an error if it doesn't reach the end. This probably just means ensuring that it reaches the user mode registers on the stack. And as Miroslav mentioned, once that's all done, implement save_stack_trace_tsk_reliable(). I don't think the above is documented anywhere, it would be good to put it in the livepatch doc.
On Tue, 12 Dec 2017 08:05:01 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: > > Hi all, > > > > 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. > > What about leaf functions? If a leaf function doesn't establish a stack > frame, and it has inline asm which contains a blr to another function, > this ABI is broken. > > Also, even for non-leaf functions, is it possible for GCC to insert the > inline asm before it sets up the stack frame? (This is an occasional > problem on x86.) Inline asm must not have control transfer out of the statement unless it is asm goto. > > Also, what about hand-coded asm? Should follow the same rules if it uses the stack. > > > 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. > > In addition to fixing the above issues, the unwinder also needs to > detect interrupts (i.e., preemption) and page faults on the stack of a > blocked task. If a function were preempted before it created a stack > frame, or if a leaf function blocked on a page fault, the stack trace > will skip the function's caller, so such a trace will need to be > reported to livepatch as unreliable. I don't think there is much problem there for powerpc. Stack frame creation and function call with return pointer are each atomic. > > Furthermore, the "reliable" unwinder needs to have a way to report an > error if it doesn't reach the end. This probably just means ensuring > that it reaches the user mode registers on the stack. > > And as Miroslav mentioned, once that's all done, implement > save_stack_trace_tsk_reliable(). > > I don't think the above is documented anywhere, it would be good to put > it in the livepatch doc. > Thanks, Nick
On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > On Tue, 12 Dec 2017 08:05:01 -0600 > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: > > > Hi all, > > > > > > 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. > > > > What about leaf functions? If a leaf function doesn't establish a stack > > frame, and it has inline asm which contains a blr to another function, > > this ABI is broken. Oops, I meant to say "bl" instead of "blr". > > Also, even for non-leaf functions, is it possible for GCC to insert the > > inline asm before it sets up the stack frame? (This is an occasional > > problem on x86.) > > Inline asm must not have control transfer out of the statement unless > it is asm goto. Can inline asm have calls to other functions? > > Also, what about hand-coded asm? > > Should follow the same rules if it uses the stack. How is that enforced? > > > 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. > > > > In addition to fixing the above issues, the unwinder also needs to > > detect interrupts (i.e., preemption) and page faults on the stack of a > > blocked task. If a function were preempted before it created a stack > > frame, or if a leaf function blocked on a page fault, the stack trace > > will skip the function's caller, so such a trace will need to be > > reported to livepatch as unreliable. > > I don't think there is much problem there for powerpc. Stack frame > creation and function call with return pointer are each atomic. What if the function is interrupted before it creates the stack frame?
On Mon, Dec 18, 2017 at 1:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: >> On Tue, 12 Dec 2017 08:05:01 -0600 >> Josh Poimboeuf <jpoimboe@redhat.com> wrote: >> >> > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: >> > > Hi all, >> > > >> > > 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. >> > >> > What about leaf functions? If a leaf function doesn't establish a stack >> > frame, and it has inline asm which contains a blr to another function, >> > this ABI is broken. > > Oops, I meant to say "bl" instead of "blr". I was wondering why "blr" mattered, but I guess we should speak of the consistency model. By walking a stack trace we expect to find whether a function is in use or not and can/cannot be live-patched at this point in time. Right? > >> > Also, even for non-leaf functions, is it possible for GCC to insert the >> > inline asm before it sets up the stack frame? (This is an occasional >> > problem on x86.) >> >> Inline asm must not have control transfer out of the statement unless >> it is asm goto. > > Can inline asm have calls to other functions? > >> > Also, what about hand-coded asm? >> >> Should follow the same rules if it uses the stack. > > How is that enforced? > >> > > 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. >> > >> > In addition to fixing the above issues, the unwinder also needs to >> > detect interrupts (i.e., preemption) and page faults on the stack of a >> > blocked task. If a function were preempted before it created a stack >> > frame, or if a leaf function blocked on a page fault, the stack trace >> > will skip the function's caller, so such a trace will need to be >> > reported to livepatch as unreliable. >> >> I don't think there is much problem there for powerpc. Stack frame >> creation and function call with return pointer are each atomic. > > What if the function is interrupted before it creates the stack frame? > If it is interrupted, the exception handler will establish a new stack frame. From a consistency viewpoint, I guess the question is -- has the function been entered or considered to be entered when a stack frame has not yet been established Balbir Singh.
On Mon, Dec 18, 2017 at 02:39:06PM +1100, Balbir Singh wrote: > On Mon, Dec 18, 2017 at 1:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > >> On Tue, 12 Dec 2017 08:05:01 -0600 > >> Josh Poimboeuf <jpoimboe@redhat.com> wrote: > >> > >> > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: > >> > > Hi all, > >> > > > >> > > 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. > >> > > >> > What about leaf functions? If a leaf function doesn't establish a stack > >> > frame, and it has inline asm which contains a blr to another function, > >> > this ABI is broken. > > > > Oops, I meant to say "bl" instead of "blr". > > I was wondering why "blr" mattered, but I guess we should speak of the > consistency > model. By walking a stack trace we expect to find whether a function is in use > or not and can/cannot be live-patched at this point in time. Right? Right. > >> > Also, even for non-leaf functions, is it possible for GCC to insert the > >> > inline asm before it sets up the stack frame? (This is an occasional > >> > problem on x86.) > >> > >> Inline asm must not have control transfer out of the statement unless > >> it is asm goto. > > > > Can inline asm have calls to other functions? > > > >> > Also, what about hand-coded asm? > >> > >> Should follow the same rules if it uses the stack. > > > > How is that enforced? > > > >> > > 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. > >> > > >> > In addition to fixing the above issues, the unwinder also needs to > >> > detect interrupts (i.e., preemption) and page faults on the stack of a > >> > blocked task. If a function were preempted before it created a stack > >> > frame, or if a leaf function blocked on a page fault, the stack trace > >> > will skip the function's caller, so such a trace will need to be > >> > reported to livepatch as unreliable. > >> > >> I don't think there is much problem there for powerpc. Stack frame > >> creation and function call with return pointer are each atomic. > > > > What if the function is interrupted before it creates the stack frame? > > > > If it is interrupted, the exception handler will establish a new stack frame. > From a consistency viewpoint, I guess the question is -- has the function > been entered or considered to be entered when a stack frame has not > yet been established Actually I think it's the function's *caller* which gets skipped. r1 (stack pointer) will point to the caller's stack frame, and presumably the unwinder would read the caller's caller's stack frame to get the next LR, skipping the caller's return address because it hasn't been saved yet.
On Sun, 17 Dec 2017 20:58:54 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > > On Tue, 12 Dec 2017 08:05:01 -0600 > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: > > > > Hi all, > > > > > > > > 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. > > > > > > What about leaf functions? If a leaf function doesn't establish a stack > > > frame, and it has inline asm which contains a blr to another function, > > > this ABI is broken. > > Oops, I meant to say "bl" instead of "blr". > > > > Also, even for non-leaf functions, is it possible for GCC to insert the > > > inline asm before it sets up the stack frame? (This is an occasional > > > problem on x86.) > > > > Inline asm must not have control transfer out of the statement unless > > it is asm goto. > > Can inline asm have calls to other functions? I don't believe so. > > > > Also, what about hand-coded asm? > > > > Should follow the same rules if it uses the stack. > > How is that enforced? It's not, AFAIK. Gcc doesn't understand what's inside asm(""). > > > > > 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. > > > > > > In addition to fixing the above issues, the unwinder also needs to > > > detect interrupts (i.e., preemption) and page faults on the stack of a > > > blocked task. If a function were preempted before it created a stack > > > frame, or if a leaf function blocked on a page fault, the stack trace > > > will skip the function's caller, so such a trace will need to be > > > reported to livepatch as unreliable. > > > > I don't think there is much problem there for powerpc. Stack frame > > creation and function call with return pointer are each atomic. > > What if the function is interrupted before it creates the stack frame? > Then there will be no stack frame, but you still get the caller address because it's saved in LR register as part of the function call. Then you get the caller's caller in its stack frame. Thanks, Nick
On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote: > On Sun, 17 Dec 2017 20:58:54 -0600 > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > > > On Tue, 12 Dec 2017 08:05:01 -0600 > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: > > > > > Hi all, > > > > > > > > > > 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. > > > > > > > > What about leaf functions? If a leaf function doesn't establish a stack > > > > frame, and it has inline asm which contains a blr to another function, > > > > this ABI is broken. > > > > Oops, I meant to say "bl" instead of "blr". > > > > > > Also, even for non-leaf functions, is it possible for GCC to insert the > > > > inline asm before it sets up the stack frame? (This is an occasional > > > > problem on x86.) > > > > > > Inline asm must not have control transfer out of the statement unless > > > it is asm goto. > > > > Can inline asm have calls to other functions? > > I don't believe so. It's allowed on x86, I don't see why it wouldn't be allowed on powerpc. As you mentioned, GCC doesn't pay attention to what's inside asm(""). > > > > Also, what about hand-coded asm? > > > > > > Should follow the same rules if it uses the stack. > > > > How is that enforced? > > It's not, AFAIK. Gcc doesn't understand what's inside asm(""). Here I was talking about .S files. > > > > > 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. > > > > > > > > In addition to fixing the above issues, the unwinder also needs to > > > > detect interrupts (i.e., preemption) and page faults on the stack of a > > > > blocked task. If a function were preempted before it created a stack > > > > frame, or if a leaf function blocked on a page fault, the stack trace > > > > will skip the function's caller, so such a trace will need to be > > > > reported to livepatch as unreliable. > > > > > > I don't think there is much problem there for powerpc. Stack frame > > > creation and function call with return pointer are each atomic. > > > > What if the function is interrupted before it creates the stack frame? > > > > Then there will be no stack frame, but you still get the caller address > because it's saved in LR register as part of the function call. Then > you get the caller's caller in its stack frame. Ok. So what about the interrupted function itself? Looking at the powerpc version of save_context_stack(), it doesn't do anything special for exception frames like checking regs->nip. Though it looks like that should be possible since show_stack() has a way to identify exception frames.
On Mon, 18 Dec 2017 12:56:22 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote: > > On Sun, 17 Dec 2017 20:58:54 -0600 > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > > > > On Tue, 12 Dec 2017 08:05:01 -0600 > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > > > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote: > > > > > > Hi all, > > > > > > > > > > > > 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. > > > > > > > > > > What about leaf functions? If a leaf function doesn't establish a stack > > > > > frame, and it has inline asm which contains a blr to another function, > > > > > this ABI is broken. > > > > > > Oops, I meant to say "bl" instead of "blr". > > > > > > > > Also, even for non-leaf functions, is it possible for GCC to insert the > > > > > inline asm before it sets up the stack frame? (This is an occasional > > > > > problem on x86.) > > > > > > > > Inline asm must not have control transfer out of the statement unless > > > > it is asm goto. > > > > > > Can inline asm have calls to other functions? > > > > I don't believe so. > > It's allowed on x86, I don't see why it wouldn't be allowed on powerpc. It's not allowed in general, but there's a lot of architecture specific differences there, so maybe x86 has an exception. I don't think such an exception exists for powerpc.... If you know exactly how the code generation works then you could write inline asm works. > As you mentioned, GCC doesn't pay attention to what's inside asm(""). And as you mentioned, it doesn't set up the stack frame properly for leaf functions that call others in asm. There's other concerns too like different ABI versions (v1/v2) have different stack and calling conventions. > > > > > Also, what about hand-coded asm? > > > > > > > > Should follow the same rules if it uses the stack. > > > > > > How is that enforced? > > > > It's not, AFAIK. Gcc doesn't understand what's inside asm(""). > > Here I was talking about .S files. Not enforced, but you can assume hand coded asm will not set up a non-standard stack frame and calling convention, that would be a bug. > > > > > > > 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. > > > > > > > > > > In addition to fixing the above issues, the unwinder also needs to > > > > > detect interrupts (i.e., preemption) and page faults on the stack of a > > > > > blocked task. If a function were preempted before it created a stack > > > > > frame, or if a leaf function blocked on a page fault, the stack trace > > > > > will skip the function's caller, so such a trace will need to be > > > > > reported to livepatch as unreliable. > > > > > > > > I don't think there is much problem there for powerpc. Stack frame > > > > creation and function call with return pointer are each atomic. > > > > > > What if the function is interrupted before it creates the stack frame? > > > > > > > Then there will be no stack frame, but you still get the caller address > > because it's saved in LR register as part of the function call. Then > > you get the caller's caller in its stack frame. > > Ok. So what about the interrupted function itself? Looking at the > powerpc version of save_context_stack(), it doesn't do anything special > for exception frames like checking regs->nip. > > Though it looks like that should be possible since show_stack() has a > way to identify exception frames. > Yes, the low level interrupt code stores a marker in the stack frame which identifies where an exception occurs. Thanks, Nick
On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote: > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote: > > On Sun, 17 Dec 2017 20:58:54 -0600 > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > > > > On Tue, 12 Dec 2017 08:05:01 -0600 > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > > > What about leaf functions? If a leaf function doesn't establish a stack > > > > > frame, and it has inline asm which contains a blr to another function, > > > > > this ABI is broken. > > > > > > Oops, I meant to say "bl" instead of "blr". You need to save LR, one way or the other. If gcc thinks it's a leaf function and does not do it, nor does your asm code, you'll return in an endless loop => bug. > > > > > Also, even for non-leaf functions, is it possible for GCC to insert the > > > > > inline asm before it sets up the stack frame? (This is an occasional > > > > > problem on x86.) > > > > > > > > Inline asm must not have control transfer out of the statement unless > > > > it is asm goto. > > > > > > Can inline asm have calls to other functions? > > > > I don't believe so. > > It's allowed on x86, I don't see why it wouldn't be allowed on powerpc. > As you mentioned, GCC doesn't pay attention to what's inside asm(""). > > > > > > Also, what about hand-coded asm? > > > > > > > > Should follow the same rules if it uses the stack. > > > > > > How is that enforced? > > > > It's not, AFAIK. Gcc doesn't understand what's inside asm(""). > > Here I was talking about .S files. asm("") or .S ... the ABI spec is clear, and it's quite easy to follow. You need a place to save LR before you call another function, and STDU is so convenient to create a stack frame with a single instruction. My impression is one would have to be very determined to break the ABI deliberately. > > > > > In addition to fixing the above issues, the unwinder also needs to > > > > > detect interrupts (i.e., preemption) and page faults on the stack of a > > > > > blocked task. If a function were preempted before it created a stack > > > > > frame, or if a leaf function blocked on a page fault, the stack trace > > > > > will skip the function's caller, so such a trace will need to be > > > > > reported to livepatch as unreliable. > > > > > > > > I don't think there is much problem there for powerpc. Stack frame > > > > creation and function call with return pointer are each atomic. > > > > > > What if the function is interrupted before it creates the stack frame? There should be a pt_regs that shows exactly this situation, see below. > > Then there will be no stack frame, but you still get the caller address > > because it's saved in LR register as part of the function call. Then > > you get the caller's caller in its stack frame. > > Ok. So what about the interrupted function itself? Looking at the > powerpc version of save_context_stack(), it doesn't do anything special > for exception frames like checking regs->nip. > > Though it looks like that should be possible since show_stack() has a > way to identify exception frames. IIRC x86 errors out if a task was interrupted in kernel context. PPC save_stack_trace_tsk_reliable() could do the same. Would that be sufficient? Torsten
On Tue, Dec 19, 2017 at 12:28:33PM +0100, Torsten Duwe wrote: > On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote: > > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote: > > > On Sun, 17 Dec 2017 20:58:54 -0600 > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > > > > > On Tue, 12 Dec 2017 08:05:01 -0600 > > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > > > > > What about leaf functions? If a leaf function doesn't establish a stack > > > > > > frame, and it has inline asm which contains a blr to another function, > > > > > > this ABI is broken. > > > > > > > > Oops, I meant to say "bl" instead of "blr". > > You need to save LR, one way or the other. If gcc thinks it's a leaf function and > does not do it, nor does your asm code, you'll return in an endless loop => bug. Ah, so the function's return path would be corrupted, and an unreliable stack trace would be the least of our problems. So it sounds like .c files should be fine, unless I'm missing something else. > > > > > > Also, even for non-leaf functions, is it possible for GCC to insert the > > > > > > inline asm before it sets up the stack frame? (This is an occasional > > > > > > problem on x86.) > > > > > > > > > > Inline asm must not have control transfer out of the statement unless > > > > > it is asm goto. > > > > > > > > Can inline asm have calls to other functions? > > > > > > I don't believe so. > > > > It's allowed on x86, I don't see why it wouldn't be allowed on powerpc. > > As you mentioned, GCC doesn't pay attention to what's inside asm(""). > > > > > > > > Also, what about hand-coded asm? > > > > > > > > > > Should follow the same rules if it uses the stack. > > > > > > > > How is that enforced? > > > > > > It's not, AFAIK. Gcc doesn't understand what's inside asm(""). > > > > Here I was talking about .S files. > > asm("") or .S ... the ABI spec is clear, and it's quite easy to follow. You > need a place to save LR before you call another function, and STDU is so > convenient to create a stack frame with a single instruction. > My impression is one would have to be very determined to break the ABI > deliberately. Ok. However, I perused the powerpc crypto code, because that was the source of a lot of frame pointer breakage in x86. I noticed that some of the crypto functions create their stack frame *before* writing the LR to the caller's stack, which is the opposite of what compiled C code does. That might confuse the unwinder if it were preempted in between. But more on that below... > > > > > > In addition to fixing the above issues, the unwinder also needs to > > > > > > detect interrupts (i.e., preemption) and page faults on the stack of a > > > > > > blocked task. If a function were preempted before it created a stack > > > > > > frame, or if a leaf function blocked on a page fault, the stack trace > > > > > > will skip the function's caller, so such a trace will need to be > > > > > > reported to livepatch as unreliable. > > > > > > > > > > I don't think there is much problem there for powerpc. Stack frame > > > > > creation and function call with return pointer are each atomic. > > > > > > > > What if the function is interrupted before it creates the stack frame? > > There should be a pt_regs that shows exactly this situation, see below. > > > > Then there will be no stack frame, but you still get the caller address > > > because it's saved in LR register as part of the function call. Then > > > you get the caller's caller in its stack frame. > > > > Ok. So what about the interrupted function itself? Looking at the > > powerpc version of save_context_stack(), it doesn't do anything special > > for exception frames like checking regs->nip. > > > > Though it looks like that should be possible since show_stack() has a > > way to identify exception frames. > > IIRC x86 errors out if a task was interrupted in kernel context. PPC > save_stack_trace_tsk_reliable() could do the same. > > Would that be sufficient? Yes, I think that would cover most of my concerns, including the above crypto asm issue. (Otherwise, we'd at least need to make sure that nothing gets skipped by the unwinder when getting preempted/page faulted in a variety of scenarios, including before LR is saved to caller, after LR is saved to caller, the crypto issue I mentioned above, etc) So with your proposal, I think I'm convinced that we don't need objtool for ppc64le. Does anyone disagree? There are still a few more things that need to be looked at: 1) With function graph tracing enabled, is the unwinder smart enough to get the original function return address, e.g. by calling ftrace_graph_ret_addr()? 2) Similar question for kretprobes. 3) Any other issues with generated code (e.g., bpf, ftrace trampolines), runtime patching (e.g., CPU feature alternatives), kprobes, paravirt, etc, that might confuse the unwinder? 4) As a sanity check, it *might* be a good idea for save_stack_trace_tsk_reliable() to ensure that it always reaches the end of the stack. There are several ways to do that: - If the syscall entry stack frame is always the same size, then the "end" would simply mean that the stack pointer is at a certain offset from the end of the task stack page. However this might not work for kthreads and idle tasks, unless their stacks also start at the same offset. (On x86 we actually standardized the end of stack location for all tasks, both user and kernel.) - If the unwinder can get to the syscall frame, it can presumably examine regs->msr to check the PR bit to ensure it got all the way to syscall entry. But again this might only work for user tasks, depending on how kernel task stacks are set up. - Or a different approach would be to do error checking along the way, and reporting an error for any unexpected conditions. However, given that backlink/LR corruption doesn't seem possible with this architecture, maybe #4 would be overkill. Personally I would feel more comfortable with an "end" check and a WARN() if it doesn't reach the end. But I could just be overly paranoid due to my x86 frame pointer experiences.
Josh Poimboeuf <jpoimboe@redhat.com> writes: > On Tue, Dec 19, 2017 at 12:28:33PM +0100, Torsten Duwe wrote: >> On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote: >> > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote: >> > > On Sun, 17 Dec 2017 20:58:54 -0600 >> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: >> > > >> > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: >> > > > > On Tue, 12 Dec 2017 08:05:01 -0600 >> > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: >> > > > > >> > > > > > What about leaf functions? If a leaf function doesn't establish a stack >> > > > > > frame, and it has inline asm which contains a blr to another function, >> > > > > > this ABI is broken. >> > > > >> > > > Oops, I meant to say "bl" instead of "blr". >> >> You need to save LR, one way or the other. If gcc thinks it's a leaf function and >> does not do it, nor does your asm code, you'll return in an endless loop => bug. > > Ah, so the function's return path would be corrupted, and an unreliable > stack trace would be the least of our problems. That's mostly true. It is possible to save LR somewhere other than the correct stack slot, in which case you can return correctly but still confuse the unwinder. A function can hide its caller that way. It's stupid and we should never do it, but it's not impossible. ... > So with your proposal, I think I'm convinced that we don't need objtool > for ppc64le. Does anyone disagree? I don't disagree, but I'd be happier if we did have objtool support. Just because it would give us a lot more certainty that we're doing the right thing everywhere, including in hand-coded asm and inline asm. It's easy to write powerpc asm such that stack traces are reliable, but it is *possible* to break them. > There are still a few more things that need to be looked at: > > 1) With function graph tracing enabled, is the unwinder smart enough to > get the original function return address, e.g. by calling > ftrace_graph_ret_addr()? No I don't think so. > 2) Similar question for kretprobes. > > 3) Any other issues with generated code (e.g., bpf, ftrace trampolines), > runtime patching (e.g., CPU feature alternatives), kprobes, paravirt, > etc, that might confuse the unwinder? We'll have to look, I can't be sure off the top of my head. > 4) As a sanity check, it *might* be a good idea for > save_stack_trace_tsk_reliable() to ensure that it always reaches the > end of the stack. There are several ways to do that: > > - If the syscall entry stack frame is always the same size, then the > "end" would simply mean that the stack pointer is at a certain > offset from the end of the task stack page. However this might not > work for kthreads and idle tasks, unless their stacks also start at > the same offset. (On x86 we actually standardized the end of stack > location for all tasks, both user and kernel.) Yeah it differs between user and kernel. > - If the unwinder can get to the syscall frame, it can presumably > examine regs->msr to check the PR bit to ensure it got all the way > to syscall entry. But again this might only work for user tasks, > depending on how kernel task stacks are set up. That sounds like a good idea. We could possibly mark the last frame of kernel tasks somehow. > - Or a different approach would be to do error checking along the > way, and reporting an error for any unexpected conditions. > > However, given that backlink/LR corruption doesn't seem possible with > this architecture, maybe #4 would be overkill. Personally I would > feel more comfortable with an "end" check and a WARN() if it doesn't > reach the end. Yeah I agree. cheers
On Thu, Dec 21, 2017 at 11:10:46PM +1100, Michael Ellerman wrote: > Josh Poimboeuf <jpoimboe@redhat.com> writes: > > > On Tue, Dec 19, 2017 at 12:28:33PM +0100, Torsten Duwe wrote: > >> On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote: > >> > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote: > >> > > On Sun, 17 Dec 2017 20:58:54 -0600 > >> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > >> > > > >> > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote: > >> > > > > On Tue, 12 Dec 2017 08:05:01 -0600 > >> > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > >> > > > > > >> > > > > > What about leaf functions? If a leaf function doesn't establish a stack > >> > > > > > frame, and it has inline asm which contains a blr to another function, > >> > > > > > this ABI is broken. > >> > > > > >> > > > Oops, I meant to say "bl" instead of "blr". > >> > >> You need to save LR, one way or the other. If gcc thinks it's a leaf function and > >> does not do it, nor does your asm code, you'll return in an endless loop => bug. > > > > Ah, so the function's return path would be corrupted, and an unreliable > > stack trace would be the least of our problems. > > That's mostly true. > > It is possible to save LR somewhere other than the correct stack slot, > in which case you can return correctly but still confuse the unwinder. A > function can hide its caller that way. > > It's stupid and we should never do it, but it's not impossible. > > ... > > > So with your proposal, I think I'm convinced that we don't need objtool > > for ppc64le. Does anyone disagree? > > I don't disagree, but I'd be happier if we did have objtool support. > > Just because it would give us a lot more certainty that we're doing the > right thing everywhere, including in hand-coded asm and inline asm. > > It's easy to write powerpc asm such that stack traces are reliable, but > it is *possible* to break them. In the unlikely case where some asm code had its own custom stack format, I guess there are two things which could go wrong: 1) bad LR: If LR isn't a kernel text address, the unwinder can stop the stack trace, WARN(), and report an error. Although if we were _extremely_ unlucky and a random leftover text address just happened to be in the LR slot, then the real function would get skipped in the stack trace. But even then, it's probably only going to be an asm function getting skipped, and we don't patch asm functions anyway, so it shouldn't affect livepatch. 2) bad back chain pointer: I'm not sure if this is even a reasonable concern. I doubt it. But if it were to happen, presumably the unwinder would abort the stack trace after reading the bad value. In this case I think the "end" check (#4 below) would be sufficient to catch it. So even if there were some stupid ppc asm code out there with its own stack magic, it still sounds to me like objtool wouldn't be needed. > > There are still a few more things that need to be looked at: > > > > 1) With function graph tracing enabled, is the unwinder smart enough to > > get the original function return address, e.g. by calling > > ftrace_graph_ret_addr()? > > No I don't think so. > > > 2) Similar question for kretprobes. > > > > 3) Any other issues with generated code (e.g., bpf, ftrace trampolines), > > runtime patching (e.g., CPU feature alternatives), kprobes, paravirt, > > etc, that might confuse the unwinder? > > We'll have to look, I can't be sure off the top of my head. > > > 4) As a sanity check, it *might* be a good idea for > > save_stack_trace_tsk_reliable() to ensure that it always reaches the > > end of the stack. There are several ways to do that: > > > > - If the syscall entry stack frame is always the same size, then the > > "end" would simply mean that the stack pointer is at a certain > > offset from the end of the task stack page. However this might not > > work for kthreads and idle tasks, unless their stacks also start at > > the same offset. (On x86 we actually standardized the end of stack > > location for all tasks, both user and kernel.) > > Yeah it differs between user and kernel. > > > - If the unwinder can get to the syscall frame, it can presumably > > examine regs->msr to check the PR bit to ensure it got all the way > > to syscall entry. But again this might only work for user tasks, > > depending on how kernel task stacks are set up. > > That sounds like a good idea. We could possibly mark the last frame of > kernel tasks somehow. > > > - Or a different approach would be to do error checking along the > > way, and reporting an error for any unexpected conditions. > > > > However, given that backlink/LR corruption doesn't seem possible with > > this architecture, maybe #4 would be overkill. Personally I would > > feel more comfortable with an "end" check and a WARN() if it doesn't > > reach the end. > > Yeah I agree.
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index c51e6ce42e7a..3e3a6ab2e089 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -218,6 +218,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
Hi all, 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. Signed-off-by: Torsten Duwe <duwe@suse.de> ---