| Message ID | 20210330190955.13707-4-madvenka@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Implement stack trace reliability checks | expand |
On Tue, Mar 30, 2021 at 02:09:54PM -0500, madvenka@linux.microsoft.com wrote: > + * FTRACE trampolines. > + */ > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + { (unsigned long) &ftrace_graph_call, 0 }, > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + { (unsigned long) ftrace_graph_caller, 0 }, > + { (unsigned long) return_to_handler, 0 }, > +#endif > +#endif It's weird that we take the address of ftrace_graph_call but not the other functions - we should be consistent or explain why. It'd probably also look nicer to not nest the ifdefs, the dependencies in Kconfig will ensure we only get things when we should.
On 4/1/21 9:27 AM, Mark Brown wrote: > On Tue, Mar 30, 2021 at 02:09:54PM -0500, madvenka@linux.microsoft.com wrote: > >> + * FTRACE trampolines. >> + */ >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS >> + { (unsigned long) &ftrace_graph_call, 0 }, >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >> + { (unsigned long) ftrace_graph_caller, 0 }, >> + { (unsigned long) return_to_handler, 0 }, >> +#endif >> +#endif > > It's weird that we take the address of ftrace_graph_call but not the > other functions - we should be consistent or explain why. It'd probably > also look nicer to not nest the ifdefs, the dependencies in Kconfig will > ensure we only get things when we should. > I have explained it in the comment in the FTRACE trampoline right above ftrace_graph_call(). /* * The only call in the FTRACE trampoline code is above. The above * instruction is patched to call a tracer function. Its return * address is below (ftrace_graph_call). In a stack trace taken from * a tracer function, ftrace_graph_call() will show. The unwinder * checks this for reliable stack trace. Please see the comments * in stacktrace.c. If another call is added in the FTRACE * trampoline code, the special_functions[] array in stacktrace.c * must be updated. */ I also noticed that I have to fix something here. The label ftrace_graph_call is defined like this: #ifdef CONFIG_FUNCTION_GRAPH_TRACER SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); nop // If enabled, this will be replaced // "b ftrace_graph_caller" #endif So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address this as well as your comment by defining another label whose name is more meaningful to our use: +SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder #ifdef CONFIG_FUNCTION_GRAPH_TRACER SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); nop // If enabled, this will be replaced // "b ftrace_graph_caller" #endif Is this acceptable? Thanks. Madhavan
On 4/1/21 9:27 AM, Mark Brown wrote: > On Tue, Mar 30, 2021 at 02:09:54PM -0500, madvenka@linux.microsoft.com wrote: > >> + * FTRACE trampolines. >> + */ >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS >> + { (unsigned long) &ftrace_graph_call, 0 }, >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >> + { (unsigned long) ftrace_graph_caller, 0 }, >> + { (unsigned long) return_to_handler, 0 }, >> +#endif >> +#endif > > It's weird that we take the address of ftrace_graph_call but not the > other functions - we should be consistent or explain why. It'd probably > also look nicer to not nest the ifdefs, the dependencies in Kconfig will > ensure we only get things when we should. > Sorry. I forgot to respond to the nested ifdef comment. I will fix that. Thanks! Madhavan
On Thu, Apr 01, 2021 at 12:43:25PM -0500, Madhavan T. Venkataraman wrote: > >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > >> + { (unsigned long) &ftrace_graph_call, 0 }, > >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > >> + { (unsigned long) ftrace_graph_caller, 0 }, > > It's weird that we take the address of ftrace_graph_call but not the > > other functions - we should be consistent or explain why. It'd probably > > also look nicer to not nest the ifdefs, the dependencies in Kconfig will > > ensure we only get things when we should. > I have explained it in the comment in the FTRACE trampoline right above > ftrace_graph_call(). Ah, right - it's a result of it being an inner label. I'd suggest putting a brief note right at that line of code explaining this (eg, "Inner label, not a function"), it wasn't confusing due to the use of that symbol but rather due to it being different from everything else in the list and that's kind of lost in the main comment. > So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address > this as well as your comment by defining another label whose name is more meaningful > to our use: > +SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); > nop // If enabled, this will be replaced > // "b ftrace_graph_caller" > #endif I'm not sure we need to bother with that, you'd still need the & I think.
On 4/1/21 1:28 PM, Mark Brown wrote: > On Thu, Apr 01, 2021 at 12:43:25PM -0500, Madhavan T. Venkataraman wrote: > >>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS >>>> + { (unsigned long) &ftrace_graph_call, 0 }, >>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >>>> + { (unsigned long) ftrace_graph_caller, 0 }, > >>> It's weird that we take the address of ftrace_graph_call but not the >>> other functions - we should be consistent or explain why. It'd probably >>> also look nicer to not nest the ifdefs, the dependencies in Kconfig will >>> ensure we only get things when we should. > >> I have explained it in the comment in the FTRACE trampoline right above >> ftrace_graph_call(). > > Ah, right - it's a result of it being an inner label. I'd suggest > putting a brief note right at that line of code explaining this (eg, > "Inner label, not a function"), it wasn't confusing due to the use of > that symbol but rather due to it being different from everything else > in the list and that's kind of lost in the main comment. > OK, So, I will add a note in the main comment above the list. I will add the comment line you have suggested at the exact line. >> So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address >> this as well as your comment by defining another label whose name is more meaningful >> to our use: > >> +SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder >> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >> SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); >> nop // If enabled, this will be replaced >> // "b ftrace_graph_caller" >> #endif > > I'm not sure we need to bother with that, you'd still need the & I think. I think we need to bother with that. If CONFIG_FUNCTION_GRAPH_TRACER is not on but CONFIG_DYNAMIC_FTRACE_WITH_REGS is, then ftrace_graph_call() will not occur in the stack trace taken from a tracer function. The unwinder still needs to recognize an ftrace frame. I don't want to assume ftrace_common_return which is the label that currently follows the above code. So, we need a different label outside the above ifdef. As for the &, I needed it because ftrace_graph_call is currently defined elsewhere as: extern unsigned long ftrace_graph_call; I did not want to change that. Thanks, Madhavan
On 4/1/21 1:40 PM, Madhavan T. Venkataraman wrote: >>> So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address >>> this as well as your comment by defining another label whose name is more meaningful >>> to our use: >>> +SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder >>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >>> SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); >>> nop // If enabled, this will be replaced >>> // "b ftrace_graph_caller" >>> #endif >> I'm not sure we need to bother with that, you'd still need the & I think. > I think we need to bother with that. If CONFIG_FUNCTION_GRAPH_TRACER is not on but > CONFIG_DYNAMIC_FTRACE_WITH_REGS is, then ftrace_graph_call() will not occur in the stack > trace taken from a tracer function. The unwinder still needs to recognize an ftrace frame. > I don't want to assume ftrace_common_return which is the label that currently follows > the above code. So, we need a different label outside the above ifdef. Alternatively, I could just move the SYM_INNER_LABEL(ftrace_graph_call..) to outside the ifdef. Madhavan
On 4/1/21 1:53 PM, Madhavan T. Venkataraman wrote: > > > On 4/1/21 1:40 PM, Madhavan T. Venkataraman wrote: >>>> So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address >>>> this as well as your comment by defining another label whose name is more meaningful >>>> to our use: >>>> +SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder >>>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >>>> SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); >>>> nop // If enabled, this will be replaced >>>> // "b ftrace_graph_caller" >>>> #endif >>> I'm not sure we need to bother with that, you'd still need the & I think. >> I think we need to bother with that. If CONFIG_FUNCTION_GRAPH_TRACER is not on but >> CONFIG_DYNAMIC_FTRACE_WITH_REGS is, then ftrace_graph_call() will not occur in the stack >> trace taken from a tracer function. The unwinder still needs to recognize an ftrace frame. >> I don't want to assume ftrace_common_return which is the label that currently follows >> the above code. So, we need a different label outside the above ifdef. > > Alternatively, I could just move the SYM_INNER_LABEL(ftrace_graph_call..) to outside the ifdef. > > Madhavan > Or, even better, I could just use ftrace_call+4 because that would be the return address for the tracer function at ftrace_call: SYM_CODE_START(ftrace_common) sub x0, x30, #AARCH64_INSN_SIZE // ip (callsite's BL insn) mov x1, x9 // parent_ip (callsite's LR) ldr_l x2, function_trace_op // op mov x3, sp // regs SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL) bl ftrace_stub I think that would be cleaner. And, I don't need the complicated comments for ftrace_graph_call. Is this acceptable? Madhavan
On Thu, Apr 01, 2021 at 02:47:11PM -0500, Madhavan T. Venkataraman wrote: > On 4/1/21 1:53 PM, Madhavan T. Venkataraman wrote: > > Alternatively, I could just move the SYM_INNER_LABEL(ftrace_graph_call..) to outside the ifdef. > Or, even better, I could just use ftrace_call+4 because that would be the return > address for the tracer function at ftrace_call: > I think that would be cleaner. And, I don't need the complicated comments for ftrace_graph_call. > Is this acceptable? I think either of those should be fine.
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S index b3e4f9a088b1..5373f88b4c44 100644 --- a/arch/arm64/kernel/entry-ftrace.S +++ b/arch/arm64/kernel/entry-ftrace.S @@ -95,6 +95,16 @@ SYM_CODE_START(ftrace_common) SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL) bl ftrace_stub + /* + * The only call in the FTRACE trampoline code is above. The above + * instruction is patched to call a tracer function. Its return + * address is below (ftrace_graph_call). In a stack trace taken from + * a tracer function, ftrace_graph_call() will show. The unwinder + * checks this for reliable stack trace. Please see the comments + * in stacktrace.c. If another call is added in the FTRACE + * trampoline code, the special_functions[] array in stacktrace.c + * must be updated. + */ #ifdef CONFIG_FUNCTION_GRAPH_TRACER SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); nop // If enabled, this will be replaced diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 7662f57d3e88..8b493a90c9f3 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -51,6 +51,52 @@ struct function_range { * unreliable. Breakpoints are used for executing probe code. Stack traces * taken while in the probe code will show an EL1 frame and will be considered * unreliable. This is correct behavior. + * + * FTRACE + * ====== + * + * When CONFIG_DYNAMIC_FTRACE_WITH_REGS is enabled, the FTRACE trampoline code + * is called from a traced function even before the frame pointer prolog. + * FTRACE sets up two stack frames (one for the traced function and one for + * its caller) so that the unwinder can provide a sensible stack trace for + * any tracer function called from the FTRACE trampoline code. + * + * There are two cases where the stack trace is not reliable. + * + * (1) The task gets preempted before the two frames are set up. Preemption + * involves an interrupt which is an EL1 exception. The unwinder already + * handles EL1 exceptions. + * + * (2) The tracer function that gets called by the FTRACE trampoline code + * changes the return PC (e.g., livepatch). + * + * Not all tracer functions do that. But to err on the side of safety, + * consider the stack trace as unreliable in all cases. + * + * When Function Graph Tracer is used, FTRACE modifies the return address of + * the traced function in its stack frame to an FTRACE return trampoline + * (return_to_handler). When the traced function returns, control goes to + * return_to_handler. return_to_handler calls FTRACE to gather tracing data + * and to obtain the original return address. Then, return_to_handler returns + * to the original return address. + * + * There are two cases to consider from a stack trace reliability point of + * view: + * + * (1) Stack traces taken within the traced function (and functions that get + * called from there) will show return_to_handler instead of the original + * return address. The original return address can be obtained from FTRACE. + * The unwinder already obtains it and modifies the return PC in its copy + * of the stack frame to the original return address. So, this is handled. + * + * (2) return_to_handler calls FTRACE as mentioned before. FTRACE discards + * the record of the original return address along the way as it does not + * need to maintain it anymore. This means that the unwinder cannot get + * the original return address beyond that point while the task is still + * executing in return_to_handler. So, consider the stack trace unreliable + * if return_to_handler is detected on the stack. + * + * NOTE: The unwinder must do (1) before (2). */ static struct function_range special_functions[] = { /* @@ -64,6 +110,17 @@ static struct function_range special_functions[] = { { (unsigned long) el1_fiq_invalid, 0 }, { (unsigned long) el1_error_invalid, 0 }, + /* + * FTRACE trampolines. + */ +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + { (unsigned long) &ftrace_graph_call, 0 }, +#ifdef CONFIG_FUNCTION_GRAPH_TRACER + { (unsigned long) ftrace_graph_caller, 0 }, + { (unsigned long) return_to_handler, 0 }, +#endif +#endif + { 0, 0 } };