[PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Puranjay Mohan]

ftrace_graph_ret_addr() takes an 'idx' integer pointer that is used to
optimize the stack unwinding process. arm64 currently passes `NULL` for
this parameter which stops it from utilizing these optimizations.

Further, the current code for ftrace_graph_ret_addr() will just return
the passed in return address if it is NULL which will break this usage.

Pass a valid integer pointer to ftrace_graph_ret_addr() similar to
x86_64's stack unwinder.

Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 6b3258860377..2729faaee4b4 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -25,6 +25,7 @@
  *
  * @common:      Common unwind state.
  * @task:        The task being unwound.
+ * @graph_idx:   Used by ftrace_graph_ret_addr() for optimized stack unwinding.
  * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
  *               associated with the most recently encountered replacement lr
  *               value.
@@ -32,6 +33,7 @@
 struct kunwind_state {
 	struct unwind_state common;
 	struct task_struct *task;
+	int graph_idx;
 #ifdef CONFIG_KRETPROBES
 	struct llist_node *kr_cur;
 #endif
@@ -106,7 +108,7 @@ kunwind_recover_return_address(struct kunwind_state *state)
 	if (state->task->ret_stack &&
 	    (state->common.pc == (unsigned long)return_to_handler)) {
 		unsigned long orig_pc;
-		orig_pc = ftrace_graph_ret_addr(state->task, NULL,
+		orig_pc = ftrace_graph_ret_addr(state->task, &state->graph_idx,
 						state->common.pc,
 						(void *)state->common.fp);
 		if (WARN_ON_ONCE(state->common.pc == orig_pc))
-- 
2.40.1

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Steven Rostedt]

On Tue, 18 Jun 2024 16:23:42 +0000
Puranjay Mohan <puranjay@kernel.org> wrote:

> ftrace_graph_ret_addr() takes an 'idx' integer pointer that is used to
> optimize the stack unwinding process. arm64 currently passes `NULL` for
> this parameter which stops it from utilizing these optimizations.

It no longer is an optimization (in linux-next). If it's not included,
it doesn't bother to find what the "return_to_handler" actually points to.

> 
> Further, the current code for ftrace_graph_ret_addr() will just return
> the passed in return address if it is NULL which will break this usage.
> 
> Pass a valid integer pointer to ftrace_graph_ret_addr() similar to
> x86_64's stack unwinder.

In the next merge window, this will not work. Besides the comment about
"optimization" not the real reason for this change...

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve


> 
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 6b3258860377..2729faaee4b4 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -25,6 +25,7 @@
>   *
>   * @common:      Common unwind state.
>   * @task:        The task being unwound.
> + * @graph_idx:   Used by ftrace_graph_ret_addr() for optimized stack unwinding.
>   * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
>   *               associated with the most recently encountered replacement lr
>   *               value.
> @@ -32,6 +33,7 @@
>  struct kunwind_state {
>  	struct unwind_state common;
>  	struct task_struct *task;
> +	int graph_idx;
>  #ifdef CONFIG_KRETPROBES
>  	struct llist_node *kr_cur;
>  #endif
> @@ -106,7 +108,7 @@ kunwind_recover_return_address(struct kunwind_state *state)
>  	if (state->task->ret_stack &&
>  	    (state->common.pc == (unsigned long)return_to_handler)) {
>  		unsigned long orig_pc;
> -		orig_pc = ftrace_graph_ret_addr(state->task, NULL,
> +		orig_pc = ftrace_graph_ret_addr(state->task, &state->graph_idx,
>  						state->common.pc,
>  						(void *)state->common.fp);
>  		if (WARN_ON_ONCE(state->common.pc == orig_pc))

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Mark Rutland]

On Tue, Jun 18, 2024 at 04:23:42PM +0000, Puranjay Mohan wrote:
> ftrace_graph_ret_addr() takes an 'idx' integer pointer that is used to
> optimize the stack unwinding process. arm64 currently passes `NULL` for
> this parameter which stops it from utilizing these optimizations.

Yep, we removed that back in commit:

  c6d3cd32fd0064af ("arm64: ftrace: use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR")

... because at the time, ftrace_graph_ret_addr() didn't use the 'idx'
argument when HAVE_FUNCTION_GRAPH_RET_ADDR_PTR was set, and I assumed
that was intentional.

AFAICT this is a fix (or preparatory patch) for commit:

  29c1c24a2707a579 ("function_graph: Fix up ftrace_graph_ret_addr())"

... which is queued up in linux-next and makes ftrace_graph_ret_addr()
use 'idx' when HAVE_FUNCTION_GRAPH_RET_ADDR_PTR is set.

Prior to that commit passing (or not passing) 'idx' has no effect
whatsoever, and after that commit not passing 'idx' is a bug.

> Further, the current code for ftrace_graph_ret_addr() will just return
> the passed in return address if it is NULL which will break this usage.
> 
> Pass a valid integer pointer to ftrace_graph_ret_addr() similar to
> x86_64's stack unwinder.
> 
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 6b3258860377..2729faaee4b4 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -25,6 +25,7 @@
>   *
>   * @common:      Common unwind state.
>   * @task:        The task being unwound.
> + * @graph_idx:   Used by ftrace_graph_ret_addr() for optimized stack unwinding.
>   * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
>   *               associated with the most recently encountered replacement lr
>   *               value.
> @@ -32,6 +33,7 @@
>  struct kunwind_state {
>  	struct unwind_state common;
>  	struct task_struct *task;
> +	int graph_idx;

Minor nit, but could we make that:

#ifdef CONFIG_FUNCTION_GRAPH_TRACER
	int graph_idx;
#endif
	
Regardless, this looks good to me, and I've tested it with a few
stacktrace scenarios including the example from commit:

  c6d3cd32fd0064af ("arm64: ftrace: use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR")

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Steve, what's your plan for merging the ftrace bits, and how should we
stage this relative to that? e.g. would it make sense for this to go
through the ftrace tree along with those changes so that this doesn't
end up transiently broken during the merge window?

Catalin, Will, do you have any preference?

Mark.

>  #ifdef CONFIG_KRETPROBES
>  	struct llist_node *kr_cur;
>  #endif
> @@ -106,7 +108,7 @@ kunwind_recover_return_address(struct kunwind_state *state)
>  	if (state->task->ret_stack &&
>  	    (state->common.pc == (unsigned long)return_to_handler)) {
>  		unsigned long orig_pc;
> -		orig_pc = ftrace_graph_ret_addr(state->task, NULL,
> +		orig_pc = ftrace_graph_ret_addr(state->task, &state->graph_idx,
>  						state->common.pc,
>  						(void *)state->common.fp);
>  		if (WARN_ON_ONCE(state->common.pc == orig_pc))
> -- 
> 2.40.1
> 

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Will Deacon]

On Tue, Jun 18, 2024 at 06:42:54PM +0100, Mark Rutland wrote:
> On Tue, Jun 18, 2024 at 04:23:42PM +0000, Puranjay Mohan wrote:
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index 6b3258860377..2729faaee4b4 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -25,6 +25,7 @@
> >   *
> >   * @common:      Common unwind state.
> >   * @task:        The task being unwound.
> > + * @graph_idx:   Used by ftrace_graph_ret_addr() for optimized stack unwinding.
> >   * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
> >   *               associated with the most recently encountered replacement lr
> >   *               value.
> > @@ -32,6 +33,7 @@
> >  struct kunwind_state {
> >  	struct unwind_state common;
> >  	struct task_struct *task;
> > +	int graph_idx;
> 
> Minor nit, but could we make that:
> 
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> 	int graph_idx;
> #endif
> 	
> Regardless, this looks good to me, and I've tested it with a few
> stacktrace scenarios including the example from commit:
> 
>   c6d3cd32fd0064af ("arm64: ftrace: use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR")
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> 
> Steve, what's your plan for merging the ftrace bits, and how should we
> stage this relative to that? e.g. would it make sense for this to go
> through the ftrace tree along with those changes so that this doesn't
> end up transiently broken during the merge window?
> 
> Catalin, Will, do you have any preference?

I think it makes most sense if this patch travels together with
29c1c24a2707 ("function_graph: Fix up ftrace_graph_ret_addr()"), so that
would be via Steve's tree. In which case:

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Steven Rostedt]

On Wed, 19 Jun 2024 13:43:18 +0100
Will Deacon <will@kernel.org> wrote:

> > Catalin, Will, do you have any preference?  
> 
> I think it makes most sense if this patch travels together with
> 29c1c24a2707 ("function_graph: Fix up ftrace_graph_ret_addr()"), so that
> would be via Steve's tree. In which case:

That makes sense to me. I'll go around pulling in all the updates to the
arch code here (with the respective acks).


> 
> Acked-by: Will Deacon <will@kernel.org>

Thanks,

-- Steve

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Mark Rutland]

Hi Steve,

On Mon, Jun 24, 2024 at 04:17:41PM -0400, Steven Rostedt wrote:
> On Wed, 19 Jun 2024 13:43:18 +0100
> Will Deacon <will@kernel.org> wrote:
> 
> > > Catalin, Will, do you have any preference?  
> > 
> > I think it makes most sense if this patch travels together with
> > 29c1c24a2707 ("function_graph: Fix up ftrace_graph_ret_addr()"), so that
> > would be via Steve's tree. In which case:
> 
> That makes sense to me. I'll go around pulling in all the updates to the
> arch code here (with the respective acks).

Are you still planning to pick this up? On next-20240829 the ftrace
tests blow up (as below) due to having the core ftrace change to
ftrace_graph_ret_addr() but lacking the arm64 fix.

Splat on next-20240829:

  [2] Basic test for tracers
  ------------[ cut here ]------------
  WARNING: CPU: 0 PID: 198 at arch/arm64/kernel/stacktrace.c:112 arch_stack_walk+0x1d0/0x350
  Modules linked in:
  CPU: 0 UID: 0 PID: 198 Comm: ftracetest Not tainted 6.11.0-rc5-next-20240829 #1
  Hardware name: linux,dummy-virt (DT)
  pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : arch_stack_walk+0x1d0/0x350
  lr : arch_stack_walk+0x1c4/0x350
  sp : ffff800080003e10
  x29: ffff800080003e10 x28: ffff800080003fe0 x27: ffff800080003eb8
  x26: ffff000003813600 x25: ffff800080004000 x24: ffff80008002ebd8
  x23: ffff800080003ed8 x22: ffff800080023c78 x21: ffff80008002ebd8
  x20: ffff800080003f40 x19: ffff800080003f30 x18: 0000000082000000
  x17: 0000000000000000 x16: ffff800080000000 x15: 00003d0900000000
  x14: 00000000000c3500 x13: 0000000000000000 x12: 0001f4e457e39c1c
  x11: ffff800082f06ca8 x10: ffff800080003f30 x9 : ffff80008002ebd8
  x8 : ffff800080003db8 x7 : 7fffffffffffffff x6 : 0000000006dd7418
  x5 : 0000000000000023 x4 : ffff800080003e88 x3 : ffff800080003fe0
  x2 : ffff80008002ebd8 x1 : 0000000000000000 x0 : ffff80008002ebd8
  Call trace:
   arch_stack_walk+0x1d0/0x350
   return_address+0x40/0x80
   trace_hardirqs_on+0x8c/0x190
   handle_softirqs+0xf0/0x400
  ---[ end trace 0000000000000000 ]---

Mark.

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Steven Rostedt]

On Thu, 29 Aug 2024 18:54:01 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> Hi Steve,
> 
> On Mon, Jun 24, 2024 at 04:17:41PM -0400, Steven Rostedt wrote:
> > On Wed, 19 Jun 2024 13:43:18 +0100
> > Will Deacon <will@kernel.org> wrote:
> >   
> > > > Catalin, Will, do you have any preference?    
> > > 
> > > I think it makes most sense if this patch travels together with
> > > 29c1c24a2707 ("function_graph: Fix up ftrace_graph_ret_addr()"), so that
> > > would be via Steve's tree. In which case:  
> > 
> > That makes sense to me. I'll go around pulling in all the updates to the
> > arch code here (with the respective acks).  
> 
> Are you still planning to pick this up? On next-20240829 the ftrace
> tests blow up (as below) due to having the core ftrace change to
> ftrace_graph_ret_addr() but lacking the arm64 fix.
> 
> Splat on next-20240829:

Ah, I didn't check to see if it was sent to linux-trace-kernel, so it
wasn't in patchwork, hence I forgot :-p

I can add it this week.

-- Steve


> 
>   [2] Basic test for tracers
>   ------------[ cut here ]------------
>   WARNING: CPU: 0 PID: 198 at arch/arm64/kernel/stacktrace.c:112 arch_stack_walk+0x1d0/0x350
>   Modules linked in:
>   CPU: 0 UID: 0 PID: 198 Comm: ftracetest Not tainted 6.11.0-rc5-next-20240829 #1
>   Hardware name: linux,dummy-virt (DT)
>   pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : arch_stack_walk+0x1d0/0x350
>   lr : arch_stack_walk+0x1c4/0x350
>   sp : ffff800080003e10
>   x29: ffff800080003e10 x28: ffff800080003fe0 x27: ffff800080003eb8
>   x26: ffff000003813600 x25: ffff800080004000 x24: ffff80008002ebd8
>   x23: ffff800080003ed8 x22: ffff800080023c78 x21: ffff80008002ebd8
>   x20: ffff800080003f40 x19: ffff800080003f30 x18: 0000000082000000
>   x17: 0000000000000000 x16: ffff800080000000 x15: 00003d0900000000
>   x14: 00000000000c3500 x13: 0000000000000000 x12: 0001f4e457e39c1c
>   x11: ffff800082f06ca8 x10: ffff800080003f30 x9 : ffff80008002ebd8
>   x8 : ffff800080003db8 x7 : 7fffffffffffffff x6 : 0000000006dd7418
>   x5 : 0000000000000023 x4 : ffff800080003e88 x3 : ffff800080003fe0
>   x2 : ffff80008002ebd8 x1 : 0000000000000000 x0 : ffff80008002ebd8
>   Call trace:
>    arch_stack_walk+0x1d0/0x350
>    return_address+0x40/0x80
>    trace_hardirqs_on+0x8c/0x190
>    handle_softirqs+0xf0/0x400
>   ---[ end trace 0000000000000000 ]---
> 
> Mark.

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Steven Rostedt]

On Tue, 3 Sep 2024 16:07:51 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 29 Aug 2024 18:54:01 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Hi Steve,
> > 
> > On Mon, Jun 24, 2024 at 04:17:41PM -0400, Steven Rostedt wrote:  
> > > On Wed, 19 Jun 2024 13:43:18 +0100
> > > Will Deacon <will@kernel.org> wrote:
> > >     
> > > > > Catalin, Will, do you have any preference?      
> > > > 
> > > > I think it makes most sense if this patch travels together with
> > > > 29c1c24a2707 ("function_graph: Fix up ftrace_graph_ret_addr()"), so that
> > > > would be via Steve's tree. In which case:    
> > > 
> > > That makes sense to me. I'll go around pulling in all the updates to the
> > > arch code here (with the respective acks).    
> > 
> > Are you still planning to pick this up? On next-20240829 the ftrace
> > tests blow up (as below) due to having the core ftrace change to
> > ftrace_graph_ret_addr() but lacking the arm64 fix.
> > 
> > Splat on next-20240829:  
> 
> Ah, I didn't check to see if it was sent to linux-trace-kernel, so it
> wasn't in patchwork, hence I forgot :-p
> 
> I can add it this week.
> 

Actually, the code is in Linus's tree now, so it's probably better if it
goes through your tree.

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Steven Rostedt]

On Wed, 4 Sep 2024 12:50:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > Ah, I didn't check to see if it was sent to linux-trace-kernel, so it
> > wasn't in patchwork, hence I forgot :-p
> > 
> > I can add it this week.
> >   
> 
> Actually, the code is in Linus's tree now, so it's probably better if it
> goes through your tree.
> 
> Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> 

And you may want to add:

Fixes: 29c1c24a2707 ("function_graph: Fix up ftrace_graph_ret_addr()")

so that it gets backported with if that change gets backported.

-- Steve

Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()

[Catalin Marinas]

On Tue, 18 Jun 2024 16:23:42 +0000, Puranjay Mohan wrote:
> ftrace_graph_ret_addr() takes an 'idx' integer pointer that is used to
> optimize the stack unwinding process. arm64 currently passes `NULL` for
> this parameter which stops it from utilizing these optimizations.
> 
> Further, the current code for ftrace_graph_ret_addr() will just return
> the passed in return address if it is NULL which will break this usage.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr()
      https://git.kernel.org/arm64/c/c060f93253ca

-- 
Catalin