Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
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. signature.asc Description: PGP signature
Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
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
Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
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
Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
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) _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
Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
On Thu, Apr 01, 2021 at 12:43:25PM -0500, Madhavan T. Venkataraman wrote:
> >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> >> + { (unsigned long) _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.
signature.asc
Description: PGP signature
Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
On Tue, Mar 30, 2021 at 02:09:54PM -0500, madve...@linux.microsoft.com wrote:
> + * FTRACE trampolines.
> + */
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> + { (unsigned long) _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.
signature.asc
Description: PGP signature
Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
On 4/1/21 9:27 AM, Mark Brown wrote:
> On Tue, Mar 30, 2021 at 02:09:54PM -0500, madve...@linux.microsoft.com wrote:
>
>> + * FTRACE trampolines.
>> + */
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>> +{ (unsigned long) _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
Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable
On 4/1/21 9:27 AM, Mark Brown wrote:
> On Tue, Mar 30, 2021 at 02:09:54PM -0500, madve...@linux.microsoft.com wrote:
>
>> + * FTRACE trampolines.
>> + */
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>> +{ (unsigned long) _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
