| Message ID | 20240618162342.28275-1-puranjay@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: stacktrace: fix the usage of ftrace_graph_ret_addr() | expand |
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))
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 >
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
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
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.
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.
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
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
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
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))
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(-)