Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452611 > unrolled thread

Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack dump reliability issues

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-07-30 01:00 +0200
    Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-30 03:00 +0200
      Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-07-30 04:30 +0200
        Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-30 16:00 +0200
          Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-01 16:30 +0200
            Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-01 17:40 +0200
              Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-02 23:10 +0200
                Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-02 23:20 +0200
                  Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-03 00:20 +0200
                    Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-03 01:20 +0200
                      Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-03 04:10 +0200
                        Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-03 04:40 +0200
                          Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-03 05:00 +0200
                            Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-03 05:10 +0200
                              Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-03 05:20 +0200
                                Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-03 05:30 +0200
                                  Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-03 05:40 +0200
                                    Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-03 05:50 +0200
                                      Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-03 16:30 +0200
                                Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-03 05:40 +0200
                                  Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-03 05:40 +0200
    Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-01 18:20 +0200
    Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-01 18:20 +0200
    Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-01 18:40 +0200
      Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Steven Rostedt <rostedt@goodmis.org> - 2016-08-01 19:00 +0200
    Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack  dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-01 18:50 +0200

Page 1 of 2  [1] 2  Next page →

#1452611 — Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack dump reliability issues

On Thu, 21 Jul 2016 16:21:42 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> When function graph tracing is enabled for a function, its return
> address on the stack is replaced with the address of an ftrace handler
> (return_to_handler).  When dumping the stack of a task with graph
> tracing enabled, there are some subtle bugs:
> 
> - The fake return_to_handler() address can be reported as reliable.
>   Instead, because it's not the real caller, it should be considered
>   unreliable.

I have some mixed emotions about this. First, it's not "fake", the
function *is* going to return to it, but you are right, that's not the
function that was called.

I do like to see these in the trace, because sometimes these functions
are an issue. But I guess I can live with them being marked as
"unreliable".


> 
> - In print_context_stack(), the real caller's return address is always
>   reported as reliable, even if the return_to_handler() address wasn't
>   referred to by a frame pointer.

Hmm, if CONFIG_FRAME_POINTER is enabled, perhaps we should only call
the look up of ftrace_graph_ret_addr(). Hmm, playing with this, yeah,
we definitely should. It can report the wrong reliability.

Without doing the reliability check we can get out of sync with the
ret_stack. I have a patch to go on top of this patch below (hmm, it may
not apply fully, because I was using a different base tree than you).

> 
> In addition to fixing these bugs, convert print_ftrace_graph_addr() to a
> more generic function which can be used outside of dump_trace()
> callbacks.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/include/asm/stacktrace.h | 13 ++++++++++
>  arch/x86/kernel/dumpstack.c       | 50 +++++++++++++++++----------------------
>  2 files changed, 35 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
> index 6f65995..5d3d258 100644
> --- a/arch/x86/include/asm/stacktrace.h
> +++ b/arch/x86/include/asm/stacktrace.h
> @@ -14,6 +14,19 @@ extern int kstack_depth_to_print;
>  struct thread_info;
>  struct stacktrace_ops;
>  
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +
> +unsigned long
> +ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long addr);
> +
> +#else
> +static inline unsigned long
> +ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long addr)
> +{
> +	return addr;
> +}
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +
>  typedef unsigned long (*walk_stack_t)(struct task_struct *task,
>  				      unsigned long *stack,
>  				      unsigned long bp,
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 692eecae..0a8694b 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -40,36 +40,25 @@ void printk_address(unsigned long address)
>  }
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -static void
> -print_ftrace_graph_addr(unsigned long addr, void *data,
> -			const struct stacktrace_ops *ops,
> -			struct task_struct *task, int *graph)
> +unsigned long
> +ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long addr)
>  {
> -	unsigned long ret_addr;
> -	int index;
> +	int task_idx;
>  
>  	if (addr != (unsigned long)return_to_handler)
> -		return;
> +		return addr;
>  
> -	index = task->curr_ret_stack;
> +	task_idx = task->curr_ret_stack;
>  
> -	if (!task->ret_stack || index < *graph)
> -		return;
> +	if (!task->ret_stack || task_idx < *idx)
> +		return addr;
>  
> -	index -= *graph;
> -	ret_addr = task->ret_stack[index].ret;
> +	task_idx -= *idx;
> +	(*idx)++;
>  
> -	ops->address(data, ret_addr, 1);
> -
> -	(*graph)++;
> +	return task->ret_stack[task_idx].ret;
>  }
> -#else
> -static inline void
> -print_ftrace_graph_addr(unsigned long addr, void *data,
> -			const struct stacktrace_ops *ops,
> -			struct task_struct *task, int *graph)
> -{ }
> -#endif
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>  
>  /*
>   * x86-64 can have up to three kernel stacks:
> @@ -108,18 +97,23 @@ print_context_stack(struct task_struct *task,
>  		stack = (unsigned long *)task_stack_page(task);
>  
>  	while (valid_stack_ptr(task, stack, sizeof(*stack), end)) {
> -		unsigned long addr;
> +		unsigned long addr = *stack;
>  
>  		addr = *stack;
>  		if (__kernel_text_address(addr)) {
> +			int reliable = 0;
> +			unsigned long real_addr;
> +
>  			if ((unsigned long) stack == bp + sizeof(long)) {
> -				ops->address(data, addr, 1);
> +				reliable = 1;
>  				frame = frame->next_frame;
>  				bp = (unsigned long) frame;
> -			} else {
> -				ops->address(data, addr, 0);
>  			}
> -			print_ftrace_graph_addr(addr, data, ops, task, graph);
> +
> +			real_addr = ftrace_graph_ret_addr(task, graph, addr);
> +			if (addr != real_addr)
> +				ops->address(data, addr, 0);

Note this changes behavior, as the original code had the ret_to_handler
first. This makes it second. (I fixed this below).

And that we should add a reliability check if CONFIG_FRAME_POINTER is
enabled.

> +			ops->address(data, real_addr, reliable);
>  		}
>  		stack++;
>  	}
> @@ -142,11 +136,11 @@ print_context_stack_bp(struct task_struct *task,
>  		if (!__kernel_text_address(addr))
>  			break;
>  
> +		addr = ftrace_graph_ret_addr(task, graph, addr);
>  		if (ops->address(data, addr, 1))
>  			break;
>  		frame = frame->next_frame;
>  		ret_addr = &frame->return_address;
> -		print_ftrace_graph_addr(addr, data, ops, task, graph);

This also changes the current code to print the return address as well.

>  	}
>  
>  	return (unsigned long)frame;

Here's my patch that should be applied on top.

Maybe add a Signed-off-by: Steven Rostedt <rostedt@goodmis.org> along
with your SOB. But you should remain Author.

-- Steve

---
 arch/x86/kernel/dumpstack.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux-trace.git/arch/x86/kernel/dumpstack.c
===================================================================
--- linux-trace.git.orig/arch/x86/kernel/dumpstack.c	2016-07-29 17:17:10.995002677 -0400
+++ linux-trace.git/arch/x86/kernel/dumpstack.c	2016-07-29 18:50:53.497633797 -0400
@@ -90,10 +90,9 @@ print_context_stack(struct task_struct *
 	while (valid_stack_ptr(task, stack, sizeof(*stack), end)) {
 		unsigned long addr = *stack;
 
-		addr = *stack;
 		if (__kernel_text_address(addr)) {
+			unsigned long real_addr = addr;
 			int reliable = 0;
-			unsigned long real_addr;
 
 			if ((unsigned long) stack == bp + sizeof(long)) {
 				reliable = 1;
@@ -101,10 +100,12 @@ print_context_stack(struct task_struct *
 				bp = (unsigned long) frame;
 			}
 
-			real_addr = ftrace_graph_ret_addr(task, graph, addr);
+			if (!IS_ENABLED(CONFIG_FRAME_POINTER) || reliable)
+				real_addr = ftrace_graph_ret_addr(task, graph, addr);
+
+			ops->address(data, real_addr, reliable);
 			if (addr != real_addr)
 				ops->address(data, addr, 0);
-			ops->address(data, real_addr, reliable);
 		}
 		stack++;
 	}
@@ -123,13 +124,16 @@ print_context_stack_bp(struct task_struc
 
 	while (valid_stack_ptr(task, ret_addr, sizeof(*ret_addr), end)) {
 		unsigned long addr = *ret_addr;
+		unsigned long real_addr;
 
 		if (!__kernel_text_address(addr))
 			break;
 
-		addr = ftrace_graph_ret_addr(task, graph, addr);
-		if (ops->address(data, addr, 1))
+		real_addr = ftrace_graph_ret_addr(task, graph, addr);
+		if (ops->address(data, real_addr, 1))
 			break;
+		if (real_addr != addr)
+			ops->address(data, addr, 0);
 		frame = frame->next_frame;
 		ret_addr = &frame->return_address;
 	}

[toc] | [next] | [standalone]

#1452624

On Fri, Jul 29, 2016 at 06:55:21PM -0400, Steven Rostedt wrote:
> On Thu, 21 Jul 2016 16:21:42 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > When function graph tracing is enabled for a function, its return
> > address on the stack is replaced with the address of an ftrace handler
> > (return_to_handler).  When dumping the stack of a task with graph
> > tracing enabled, there are some subtle bugs:
> > 
> > - The fake return_to_handler() address can be reported as reliable.
> >   Instead, because it's not the real caller, it should be considered
> >   unreliable.
> 
> I have some mixed emotions about this. First, it's not "fake", the
> function *is* going to return to it, but you are right, that's not the
> function that was called.
> 
> I do like to see these in the trace, because sometimes these functions
> are an issue. But I guess I can live with them being marked as
> "unreliable".

Yeah, this is a little iffy.  Calling return_to_handler() "fake" isn't
100% accurate.  It wasn't involved in the *call* path, but it will be
involved in the *return* path.

My thinking was that when either saving or dumping the stack, you
normally only care about what led up to that point (the call path),
rather than what will happen in the future (the return path).

That's especially true in the non-oops stack trace case, which isn't
used for debugging.  For example, reporting return_to_handler() in the
reliable trace of a perf profiling operation would just be confusing.

And in the oops case, where debugging is important, I think "unreliable"
is more appropriate because it serves as a hint that graph tracing was
involved, instead of trying to assert that it was the real caller, which
could create some confusion.

> > - In print_context_stack(), the real caller's return address is always
> >   reported as reliable, even if the return_to_handler() address wasn't
> >   referred to by a frame pointer.
> 
> Hmm, if CONFIG_FRAME_POINTER is enabled, perhaps we should only call
> the look up of ftrace_graph_ret_addr(). Hmm, playing with this, yeah,
> we definitely should. It can report the wrong reliability.
> 
> Without doing the reliability check we can get out of sync with the
> ret_stack. I have a patch to go on top of this patch below (hmm, it may
> not apply fully, because I was using a different base tree than you).

Yeah, your patch makes it better.  Thanks!

BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent
so we could get the "real" return address without having to pass in a
state variable.

For example we could add an "unsigned long *retp" pointer to
ftrace_ret_stack, which points to the return address on the stack.  Then
we could get rid of the index state variable in ftrace_graph_ret_addr,
and also then there would never be a chance of the stack dump getting
out of sync with the ret_stack.

What do you think?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1452632

On Fri, 29 Jul 2016 19:50:59 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent
> so we could get the "real" return address without having to pass in a
> state variable.
> 
> For example we could add an "unsigned long *retp" pointer to
> ftrace_ret_stack, which points to the return address on the stack.  Then
> we could get rid of the index state variable in ftrace_graph_ret_addr,
> and also then there would never be a chance of the stack dump getting
> out of sync with the ret_stack.
> 
> What do you think?
> 

I don't want to extend ret_stack as that is allocated 50 of these
structures for every task. That said, we have the "fp" field that's
used to check for frame pointer corruption when mcount is used. With
CC_USING_FENTRY, that field is ignored. Perhaps we could overload that
field for this.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1452686

On Fri, Jul 29, 2016 at 10:20:36PM -0400, Steven Rostedt wrote:
> On Fri, 29 Jul 2016 19:50:59 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent
> > so we could get the "real" return address without having to pass in a
> > state variable.
> > 
> > For example we could add an "unsigned long *retp" pointer to
> > ftrace_ret_stack, which points to the return address on the stack.  Then
> > we could get rid of the index state variable in ftrace_graph_ret_addr,
> > and also then there would never be a chance of the stack dump getting
> > out of sync with the ret_stack.
> > 
> > What do you think?
> > 
> 
> I don't want to extend ret_stack as that is allocated 50 of these
> structures for every task. That said, we have the "fp" field that's
> used to check for frame pointer corruption when mcount is used. With
> CC_USING_FENTRY, that field is ignored. Perhaps we could overload that
> field for this.

In that case, I guess we would need two versions of
ftrace_graph_ret_addr(), with the current implementation still needed
for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.

Or would you want to get rid of HAVE_FUNCTION_GRAPH_FP_TEST for x86?

BTW, on a different note, should I put ftrace_graph_ret_addr() to
kernel/trace/trace_functions_graph.c so other arches can use it?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1453240

On Sat, 30 Jul 2016 08:51:25 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Fri, Jul 29, 2016 at 10:20:36PM -0400, Steven Rostedt wrote:
> > On Fri, 29 Jul 2016 19:50:59 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >   
> > > BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent
> > > so we could get the "real" return address without having to pass in a
> > > state variable.
> > > 
> > > For example we could add an "unsigned long *retp" pointer to
> > > ftrace_ret_stack, which points to the return address on the stack.  Then
> > > we could get rid of the index state variable in ftrace_graph_ret_addr,
> > > and also then there would never be a chance of the stack dump getting
> > > out of sync with the ret_stack.
> > > 
> > > What do you think?
> > >   
> > 
> > I don't want to extend ret_stack as that is allocated 50 of these
> > structures for every task. That said, we have the "fp" field that's
> > used to check for frame pointer corruption when mcount is used. With
> > CC_USING_FENTRY, that field is ignored. Perhaps we could overload that
> > field for this.  
> 
> In that case, I guess we would need two versions of
> ftrace_graph_ret_addr(), with the current implementation still needed
> for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.

How hard would it be in that case?

> 
> Or would you want to get rid of HAVE_FUNCTION_GRAPH_FP_TEST for x86?

No, because there's gcc versions that we still support that mess up
mcount, and could still cause issues with function graph.


> 
> BTW, on a different note, should I put ftrace_graph_ret_addr() to
> kernel/trace/trace_functions_graph.c so other arches can use it?
> 

I guess you could. There doesn't seem to be any x86 specific code in
that right?

-- Steve

[toc] | [prev] | [next] | [standalone]

#1453284

On Mon, Aug 01, 2016 at 10:28:21AM -0400, Steven Rostedt wrote:
> On Sat, 30 Jul 2016 08:51:25 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Fri, Jul 29, 2016 at 10:20:36PM -0400, Steven Rostedt wrote:
> > > On Fri, 29 Jul 2016 19:50:59 -0500
> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >   
> > > > BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent
> > > > so we could get the "real" return address without having to pass in a
> > > > state variable.
> > > > 
> > > > For example we could add an "unsigned long *retp" pointer to
> > > > ftrace_ret_stack, which points to the return address on the stack.  Then
> > > > we could get rid of the index state variable in ftrace_graph_ret_addr,
> > > > and also then there would never be a chance of the stack dump getting
> > > > out of sync with the ret_stack.
> > > > 
> > > > What do you think?
> > > >   
> > > 
> > > I don't want to extend ret_stack as that is allocated 50 of these
> > > structures for every task. That said, we have the "fp" field that's
> > > used to check for frame pointer corruption when mcount is used. With
> > > CC_USING_FENTRY, that field is ignored. Perhaps we could overload that
> > > field for this.  
> > 
> > In that case, I guess we would need two versions of
> > ftrace_graph_ret_addr(), with the current implementation still needed
> > for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.
> 
> How hard would it be in that case?

Well, it would be easy enough, but then the caller would still need to
pass in the state variable.  So maybe it's not worth the trouble.

> > Or would you want to get rid of HAVE_FUNCTION_GRAPH_FP_TEST for x86?
> 
> No, because there's gcc versions that we still support that mess up
> mcount, and could still cause issues with function graph.
> 
> 
> > 
> > BTW, on a different note, should I put ftrace_graph_ret_addr() to
> > kernel/trace/trace_functions_graph.c so other arches can use it?
> > 
> 
> I guess you could. There doesn't seem to be any x86 specific code in
> that right?

Right.  And I noticed that several arches implement this same
functionality in a slightly different way to fit with their stack dump
code.  So it would be nice to make it common.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1455473

On Mon, Aug 01, 2016 at 10:36:33AM -0500, Josh Poimboeuf wrote:
> On Mon, Aug 01, 2016 at 10:28:21AM -0400, Steven Rostedt wrote:
> > On Sat, 30 Jul 2016 08:51:25 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > On Fri, Jul 29, 2016 at 10:20:36PM -0400, Steven Rostedt wrote:
> > > > On Fri, 29 Jul 2016 19:50:59 -0500
> > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > >   
> > > > > BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent
> > > > > so we could get the "real" return address without having to pass in a
> > > > > state variable.
> > > > > 
> > > > > For example we could add an "unsigned long *retp" pointer to
> > > > > ftrace_ret_stack, which points to the return address on the stack.  Then
> > > > > we could get rid of the index state variable in ftrace_graph_ret_addr,
> > > > > and also then there would never be a chance of the stack dump getting
> > > > > out of sync with the ret_stack.
> > > > > 
> > > > > What do you think?
> > > > >   
> > > > 
> > > > I don't want to extend ret_stack as that is allocated 50 of these
> > > > structures for every task. That said, we have the "fp" field that's
> > > > used to check for frame pointer corruption when mcount is used. With
> > > > CC_USING_FENTRY, that field is ignored. Perhaps we could overload that
> > > > field for this.  
> > > 
> > > In that case, I guess we would need two versions of
> > > ftrace_graph_ret_addr(), with the current implementation still needed
> > > for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.
> > 
> > How hard would it be in that case?
> 
> Well, it would be easy enough, but then the caller would still need to
> pass in the state variable.  So maybe it's not worth the trouble.

I did some stack trace testing on mainline with function graph tracing.
As it turns out, print_ftrace_graph_addr() is already buggy today if the
caller of dump_trace() specifies a stack pointer or a pt_regs (which is
usually done in order to skip some irrelevant stack frames in the
trace).

For example, here's a stack trace based on NMI regs:

  $ echo 1 > /proc/sys/kernel/sysrq
  $ echo l > /proc/sysrq-trigger
  ...
  Call Trace:
   [<ffffffff81066141>] ?  __x2apic_send_IPI_dest.constprop.4+0x31/0x40
   [<ffffffff810661e5>] __x2apic_send_IPI_mask+0x95/0xe0
   [<ffffffff81061d70>] ? irq_force_complete_move+0xf0/0xf0
   [<ffffffff810662a3>] x2apic_send_IPI_mask+0x13/0x20
   [<ffffffff81061d8b>] nmi_raise_cpu_backtrace+0x1b/0x20
   [<ffffffff8144ff76>] nmi_trigger_all_cpu_backtrace+0xc6/0xf0
   [<ffffffff81061de9>] arch_trigger_all_cpu_backtrace+0x19/0x20
   [<ffffffff8155c463>] sysrq_handle_showallcpus+0x13/0x20
   [<ffffffff8155cc18>] __handle_sysrq+0x138/0x220
   [<ffffffff8155cae5>] ? __handle_sysrq+0x5/0x220
   [<ffffffff8155d111>] write_sysrq_trigger+0x51/0x60
   [<ffffffff813104e2>] proc_reg_write+0x42/0x70
   [<ffffffff81291877>] __vfs_write+0x37/0x140
   [<ffffffff8110d161>] ? update_fast_ctr+0x51/0x80
   [<ffffffff8110d217>] ? percpu_down_read+0x57/0xa0
   [<ffffffff81296074>] ? __sb_start_write+0xb4/0xf0
   [<ffffffff81296074>] ? __sb_start_write+0xb4/0xf0
   [<ffffffff81292b38>] vfs_write+0xb8/0x1a0
   [<ffffffff81293fe8>] SyS_write+0x58/0xc0
   [<ffffffff818af97c>] entry_SYSCALL_64_fastpath+0x1f/0xbd

And here's the same trace with function graph tracing:

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer 
  $ echo l > /proc/sysrq-trigger
  ...
  Call Trace:
   [<ffffffff81066141>] ?  __x2apic_send_IPI_dest.constprop.4+0x31/0x40
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff810394cc>] print_context_stack+0xfc/0x100
   [<ffffffff81061d70>] ? irq_force_complete_move+0xf0/0xf0
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff8103891b>] dump_trace+0x12b/0x350
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff810396eb>] show_trace_log_lvl+0x4b/0x60
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff81038c76>] show_stack_log_lvl+0x136/0x1d0
   [<ffffffff81061de9>] arch_trigger_all_cpu_backtrace+0x19/0x20
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff81038db8>] show_regs+0xa8/0x1b0
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff8144fe96>] nmi_cpu_backtrace+0x46/0x60
   [<ffffffff8155cae5>] ? __handle_sysrq+0x5/0x220
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff81039b5f>] nmi_handle+0xbf/0x2f0
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff8103a2b3>] default_do_nmi+0x73/0x180
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff8103a4d9>] do_nmi+0x119/0x170
   [<ffffffff811bb3cd>] ?  ftrace_return_to_handler+0x9d/0x110
   [<ffffffff81291845>] ? __vfs_write+0x5/0x140
   [<ffffffff81291845>] ? __vfs_write+0x5/0x140
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff810661e5>] __x2apic_send_IPI_mask+0x95/0xe0
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff810662a3>] x2apic_send_IPI_mask+0x13/0x20
   [<ffffffff818b2428>] ftrace_graph_caller+0xa8/0xa8
   [<ffffffff81061d8b>] nmi_raise_cpu_backtrace+0x1b/0x20

The ret_stack is out of sync with the stack dump because the stack dump
was started with the regs from the NMI, instead of being started from
the current frame.

So I guess there are a couple of ways to fix it:

  a) keep track of the return address pointer like we discussed above;

     or

  b) have the unwinder count the # of skipped frames which refer to
     'return_to_handler', and pass that as the initial index value to
     ftrace_graph_ret_addr().

Option a) would be much cleaner.  But to fix it for both mcount and
fentry, we couldn't override 'fp' so I guess we'd need to add a new
field to ftrace_ret_stack.

Option b) is uglier, but I could probably make it work with the new
unwinder.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1455479

On Tue, 2 Aug 2016 16:00:11 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

   [<ffffffff81061d8b>] nmi_raise_cpu_backtrace+0x1b/0x20
> 
> The ret_stack is out of sync with the stack dump because the stack dump
> was started with the regs from the NMI, instead of being started from
> the current frame.
> 
> So I guess there are a couple of ways to fix it:
> 
>   a) keep track of the return address pointer like we discussed above;
> 
>      or
> 
>   b) have the unwinder count the # of skipped frames which refer to
>      'return_to_handler', and pass that as the initial index value to
>      ftrace_graph_ret_addr().
> 
> Option a) would be much cleaner.  But to fix it for both mcount and
> fentry, we couldn't override 'fp' so I guess we'd need to add a new
> field to ftrace_ret_stack.

Actually, what about calling ftrace_graph_ret_addr() to figure out the
next stack conversion only if reliable or CONFIG_FRAME_POINTER is not
enabled?

	unsigned long real_addr = addr;

	[...]

	if (!IS_ENABLED(CONFIG_FRAME_POINTER) || reliable)
		real_addr = ftrace_graph_ret_addr(task, graph, addr);
	if (addr != real_addr)
		ops->address(data, addr, 0);
	ops->address(data, real_addr, reliable);

Then we only need the fp use case when FRAME_POINTER is not set. As
mcount forces FRAME_POINTER, we only need to worry about the fentry
case.

-- Steve

> 
> Option b) is uglier, but I could probably make it work with the new
> unwinder.
> 

[toc] | [prev] | [next] | [standalone]

#1455502

On Tue, Aug 02, 2016 at 05:16:10PM -0400, Steven Rostedt wrote:
> On Tue, 2 Aug 2016 16:00:11 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
>    [<ffffffff81061d8b>] nmi_raise_cpu_backtrace+0x1b/0x20
> > 
> > The ret_stack is out of sync with the stack dump because the stack dump
> > was started with the regs from the NMI, instead of being started from
> > the current frame.
> > 
> > So I guess there are a couple of ways to fix it:
> > 
> >   a) keep track of the return address pointer like we discussed above;
> > 
> >      or
> > 
> >   b) have the unwinder count the # of skipped frames which refer to
> >      'return_to_handler', and pass that as the initial index value to
> >      ftrace_graph_ret_addr().
> > 
> > Option a) would be much cleaner.  But to fix it for both mcount and
> > fentry, we couldn't override 'fp' so I guess we'd need to add a new
> > field to ftrace_ret_stack.
> 
> Actually, what about calling ftrace_graph_ret_addr() to figure out the
> next stack conversion only if reliable or CONFIG_FRAME_POINTER is not
> enabled?
> 
> 	unsigned long real_addr = addr;
> 
> 	[...]
> 
> 	if (!IS_ENABLED(CONFIG_FRAME_POINTER) || reliable)
> 		real_addr = ftrace_graph_ret_addr(task, graph, addr);
> 	if (addr != real_addr)
> 		ops->address(data, addr, 0);
> 	ops->address(data, real_addr, reliable);
> 
> Then we only need the fp use case when FRAME_POINTER is not set. As
> mcount forces FRAME_POINTER, we only need to worry about the fentry
> case.

Hm, I'm confused.  First, I don't see where mcount forces FRAME_POINTER.

Second, I don't see why that even matters.  If mcount and frame pointers
are enabled, then the 'fp' field of ftrace_ret_stack is needed for the
gcc sanity check, right?  So we couldn't override 'fp', and the old
"stateful index" version of ftrace_graph_ret_addr() would have to be
used in the code above for reliable addresses, and we'd still have the
same out-of-sync bug.

Or am I missing something?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1455528

On Tue, 2 Aug 2016 17:13:59 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> > Then we only need the fp use case when FRAME_POINTER is not set. As
> > mcount forces FRAME_POINTER, we only need to worry about the fentry
> > case.  
> 
> Hm, I'm confused.  First, I don't see where mcount forces FRAME_POINTER.

Hmm, we should probably force it generally, as gcc itself requires
mcount to be used with framepointers. -mcount can't be used without
them.

> 
> Second, I don't see why that even matters.  If mcount and frame pointers
> are enabled, then the 'fp' field of ftrace_ret_stack is needed for the
> gcc sanity check, right?  So we couldn't override 'fp', and the old
> "stateful index" version of ftrace_graph_ret_addr() would have to be
> used in the code above for reliable addresses, and we'd still have the
> same out-of-sync bug.
> 
> Or am I missing something?
> 

Or I missed something. How did we get out of sync? If we have frame
pointers, shouldn't the "return_to_handler" be seen as reliable by the
code (not that we save it as such)? That is, if the frame pointer shows
that the next function is return_to_handler, then we increment the
index into ret_stack, otherwise we simply record the return_to_handler
as a normal "unreliable" function, without any processing of it.

I guess I don't actually understand how the NMI screwed it up, as
function graph doesn't trace "do_nmi()" itself nor anything before that.
I'm guessing it really got out of sync because there's a
"return_to_handler" in the stack that wasn't really called (not a frame
pointer). The ftrace_graph_ret_addr() will shift the index currently
regardless if the return_to_handler found is part of a stack frame, or
just left over in the stack. THAT is why I think it got out of sync.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1455577

On Tue, Aug 02, 2016 at 07:16:22PM -0400, Steven Rostedt wrote:
> On Tue, 2 Aug 2016 17:13:59 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > > Then we only need the fp use case when FRAME_POINTER is not set. As
> > > mcount forces FRAME_POINTER, we only need to worry about the fentry
> > > case.  
> > 
> > Hm, I'm confused.  First, I don't see where mcount forces FRAME_POINTER.
> 
> Hmm, we should probably force it generally, as gcc itself requires
> mcount to be used with framepointers. -mcount can't be used without
> them.
> 
> > 
> > Second, I don't see why that even matters.  If mcount and frame pointers
> > are enabled, then the 'fp' field of ftrace_ret_stack is needed for the
> > gcc sanity check, right?  So we couldn't override 'fp', and the old
> > "stateful index" version of ftrace_graph_ret_addr() would have to be
> > used in the code above for reliable addresses, and we'd still have the
> > same out-of-sync bug.
> > 
> > Or am I missing something?
> > 
> 
> Or I missed something. How did we get out of sync? If we have frame
> pointers, shouldn't the "return_to_handler" be seen as reliable by the
> code (not that we save it as such)? That is, if the frame pointer shows
> that the next function is return_to_handler, then we increment the
> index into ret_stack, otherwise we simply record the return_to_handler
> as a normal "unreliable" function, without any processing of it.
> 
> I guess I don't actually understand how the NMI screwed it up, as
> function graph doesn't trace "do_nmi()" itself nor anything before that.
> I'm guessing it really got out of sync because there's a
> "return_to_handler" in the stack that wasn't really called (not a frame
> pointer). The ftrace_graph_ret_addr() will shift the index currently
> regardless if the return_to_handler found is part of a stack frame, or
> just left over in the stack. THAT is why I think it got out of sync.

It's not specific to NMIs.  The problem is that dump_trace() is starting
from the frame pointed to by a pt_regs, rather than the current frame.
Instead of starting with the current frame, the first 10 functions on
the stack are skipped by the unwinder, but they're *not* skipped on the
ret_stack.  So it starts out out-of-sync.

If it had first initialized the graph index variable to 10 instead of 0
before passing it to ftrace_graph_ret_addr(), it would have worked.

The problem isn't specific to NMIs.  It happens anywhere the first few
stack frames are skipped, which is very common.  For example:

  $ cat /proc/self/stack
  [<ffffffff810489a2>] save_stack_trace_tsk+0x22/0x40
  [<ffffffff81311a89>] proc_pid_stack+0xb9/0x110
  [<ffffffff813127c4>] proc_single_show+0x54/0x80
  [<ffffffff812be088>] seq_read+0x108/0x3e0
  [<ffffffff812923d7>] __vfs_read+0x37/0x140
  [<ffffffff812929d9>] vfs_read+0x99/0x140
  [<ffffffff81293f28>] SyS_read+0x58/0xc0
  [<ffffffff818af97c>] entry_SYSCALL_64_fastpath+0x1f/0xbd
  [<ffffffffffffffff>] 0xffffffffffffffff

  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer 
  $ cat /proc/self/stack
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff810394cc>] print_context_stack+0xfc/0x100
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff8103891b>] dump_trace+0x12b/0x350
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff810489a2>] save_stack_trace_tsk+0x22/0x40
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff81311a89>] proc_pid_stack+0xb9/0x110
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff813127c4>] proc_single_show+0x54/0x80
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff812be088>] seq_read+0x108/0x3e0
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff812923d7>] __vfs_read+0x37/0x140
  [<ffffffff818b2428>] return_to_handler+0x0/0x27
  [<ffffffff812929d9>] vfs_read+0x99/0x140
  [<ffffffffffffffff>] 0xffffffffffffffff

In this case, it's offset by two frames.  With function graph tracing
enabled, it starts with print_context_stack() instead of
save_stack_trace_tsk(), and it doesn't show the last two frames.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1455584

On Tue, 2 Aug 2016 20:56:56 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> It's not specific to NMIs.  The problem is that dump_trace() is starting
> from the frame pointed to by a pt_regs, rather than the current frame.
> Instead of starting with the current frame, the first 10 functions on
> the stack are skipped by the unwinder, but they're *not* skipped on the
> ret_stack.  So it starts out out-of-sync.

OK, I see what you mean. If we do a dumpstack from interrupt passing in
the pt_regs of the kernel thread that was interrupted, even though
functions up to the interrupt was called and traced, which will show up
in the dump stack that shouldn't.

OK, you convinced me. Add the extra pointer, then we will have 4 longs
and 2 long longs in ftrace_ret_stack. That's not that bad.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1455595

On Tue, Aug 02, 2016 at 10:30:11PM -0400, Steven Rostedt wrote:
> On Tue, 2 Aug 2016 20:56:56 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > It's not specific to NMIs.  The problem is that dump_trace() is starting
> > from the frame pointed to by a pt_regs, rather than the current frame.
> > Instead of starting with the current frame, the first 10 functions on
> > the stack are skipped by the unwinder, but they're *not* skipped on the
> > ret_stack.  So it starts out out-of-sync.
> 
> OK, I see what you mean. If we do a dumpstack from interrupt passing in
> the pt_regs of the kernel thread that was interrupted, even though
> functions up to the interrupt was called and traced, which will show up
> in the dump stack that shouldn't.
> 
> OK, you convinced me. Add the extra pointer, then we will have 4 longs
> and 2 long longs in ftrace_ret_stack. That's not that bad.

Hm, since 'fp' is only used for mcount, I guess we could avoid
allocating it for fentry?  That would save a long when a modern compiler
is used.  Like:

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1e814ae..fc508a7 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -795,7 +795,9 @@ struct ftrace_ret_stack {
 	unsigned long func;
 	unsigned long long calltime;
 	unsigned long long subtime;
+#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)
 	unsigned long fp;
+#endif
 };
 
 /*
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 9caa9b2..86b2719 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -171,7 +171,9 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
 	current->ret_stack[index].func = func;
 	current->ret_stack[index].calltime = calltime;
 	current->ret_stack[index].subtime = 0;
+#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)
 	current->ret_stack[index].fp = frame_pointer;
+#endif
 	*depth = current->curr_ret_stack;
 
 	return 0;

[toc] | [prev] | [next] | [standalone]

#1455602

On Tue, 2 Aug 2016 21:50:12 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1e814ae..fc508a7 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -795,7 +795,9 @@ struct ftrace_ret_stack {
>  	unsigned long func;
>  	unsigned long long calltime;
>  	unsigned long long subtime;
> +#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)

We need to make a new defined in ftrace.h:

#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)
# define HAVE_FUNCTION_GRAPH_FP_TEST
#endif

And use that instead of this && complexity.

Or better yet, get rid of the CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST define
and only have HAVE_FUNCTION_GRAPH_FP_TEST defined in the asm/ftrace.h
in each arch. Then, x86 could just do;

#ifndef CC_USING_FENTRY
# define HAVE_FUNCTION_GRAPH_FP_TEST
#endif


-- Steve

>  	unsigned long fp;
> +#endif
>  };
>  
>  /*
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 9caa9b2..86b2719 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -171,7 +171,9 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
>  	current->ret_stack[index].func = func;
>  	current->ret_stack[index].calltime = calltime;
>  	current->ret_stack[index].subtime = 0;
> +#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)
>  	current->ret_stack[index].fp = frame_pointer;
> +#endif
>  	*depth = current->curr_ret_stack;
>  
>  	return 0;

[toc] | [prev] | [next] | [standalone]

#1455606

On Tue, Aug 02, 2016 at 10:59:36PM -0400, Steven Rostedt wrote:
> On Tue, 2 Aug 2016 21:50:12 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 1e814ae..fc508a7 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -795,7 +795,9 @@ struct ftrace_ret_stack {
> >  	unsigned long func;
> >  	unsigned long long calltime;
> >  	unsigned long long subtime;
> > +#if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)
> 
> We need to make a new defined in ftrace.h:
> 
> #if defined(CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST) && !defined(CC_USING_FENTRY)
> # define HAVE_FUNCTION_GRAPH_FP_TEST
> #endif
> 
> And use that instead of this && complexity.
> 
> Or better yet, get rid of the CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST define
> and only have HAVE_FUNCTION_GRAPH_FP_TEST defined in the asm/ftrace.h
> in each arch. Then, x86 could just do;
> 
> #ifndef CC_USING_FENTRY
> # define HAVE_FUNCTION_GRAPH_FP_TEST
> #endif

Sounds good.  I was thinking I could also add a similar define to
indicate whether an arch passes the return address stack pointer to
ftrace_push_return_trace().  HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1455607

On Tue, 2 Aug 2016 23:18:57 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 2 Aug 2016 22:12:33 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > Sounds good.  I was thinking I could also add a similar define to
> > indicate whether an arch passes the return address stack pointer to
> > ftrace_push_return_trace().  HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?
> >   
> 
> If you are making this function global, might as well make all pass
> that pointer when you do the conversion. I don't think we need a define
> to differentiate it.
> 

Bah, I was thinking of your ftrace_graph_ret_addr() function. /me needs
to go to bed.

Anyway, if we have to add a parameter, we probably need to update all
the callers anyway. We do need to add a parameter for this, right?

-- Steve

[toc] | [prev] | [next] | [standalone]

#1455610

On Tue, Aug 02, 2016 at 11:21:04PM -0400, Steven Rostedt wrote:
> On Tue, 2 Aug 2016 23:18:57 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 2 Aug 2016 22:12:33 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > 
> > > Sounds good.  I was thinking I could also add a similar define to
> > > indicate whether an arch passes the return address stack pointer to
> > > ftrace_push_return_trace().  HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?
> > >   
> > 
> > If you are making this function global, might as well make all pass
> > that pointer when you do the conversion. I don't think we need a define
> > to differentiate it.
> > 
> 
> Bah, I was thinking of your ftrace_graph_ret_addr() function. /me needs
> to go to bed.
> 
> Anyway, if we have to add a parameter, we probably need to update all
> the callers anyway. We do need to add a parameter for this, right?

Yeah, we do need to add a parameter to ftrace_push_return_trace().  But
callers which don't implement it could just pass zero like they do with
'fp'.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1455615

On Tue, 2 Aug 2016 22:31:25 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> Yeah, we do need to add a parameter to ftrace_push_return_trace().  But
> callers which don't implement it could just pass zero like they do with
> 'fp'.
> 

Right, if zero is passed in, then just ignore it.

Bed time!

-- Steve

[toc] | [prev] | [next] | [standalone]

#1455822

On Tue, Aug 02, 2016 at 11:45:30PM -0400, Steven Rostedt wrote:
> On Tue, 2 Aug 2016 22:31:25 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > Yeah, we do need to add a parameter to ftrace_push_return_trace().  But
> > callers which don't implement it could just pass zero like they do with
> > 'fp'.
> > 
> 
> Right, if zero is passed in, then just ignore it.

I still think we need the define though, because there will be two
versions of ftrace_graph_ret_addr().

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1455611

On Tue, 2 Aug 2016 22:12:33 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> Sounds good.  I was thinking I could also add a similar define to
> indicate whether an arch passes the return address stack pointer to
> ftrace_push_return_trace().  HAVE_FUNCTION_GRAPH_RET_ADDR_PTR?
> 

If you are making this function global, might as well make all pass
that pointer when you do the conversion. I don't think we need a define
to differentiate it.

-- Steve

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web