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

[Mark Rutland <mark.rutland@arm.com>]

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
>