[PATCH] arm64: ftrace: use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR

[Will Deacon]

On Wed, Oct 27, 2021 at 02:25:28PM +0100, Mark Rutland wrote:
> When CONFIG_FUNCTION_GRAPH_TRACER is selected, and the function graph:

Weird ':' on the end of "graph"

> tracer is in use, unwind_frame() may erroneously asscociate a traced

associate

> function with an incorrect return address. This can happen when starting
> an unwind from a pt_regs, or when unwinding across an exception
> boundary.
> 
> The underlying problem is that ftrace_graph_get_ret_stack() takes an
> index offset from the most recent entry added to the fgraph return
> stack. We start an unwind at offset 0, and increment the offset each
> time we encounter a rewritten return address (i.e. when we see
> `return_to_handler`). This is broken in two cases:
> 
> 1) Between creating a pt_regs and starting the unwind, function calls
>    may place entries on the stack, leaving an abitrary offset which we
>    can only determine by performing a full unwind from the caller of the
>    unwind code (and relying on none of the unwind code being
>    instrumented). While this initial unwind is open-coded in
>    dump_backtrace(), this is not performed for other unwinders such as
>    perf_callchain_kernel().

What is the user-visible impact of this?

> 2) When unwinding across an exception boundary (whether continuing an
>    unwind or starting a new unwind from regs), we currently always skip
>    the LR of the interrupted context. Where this was live and contained
>    a rewritten address, we won't consume the corresponding fgraph ret
>    stack entry, leaving subsequent entries off-by-one.
> 
> To fix this, we need to be able to uniquely identify each rewritten
> return address such that we can map this back to the original reeturn

return

> address. We can do so using HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, and
> associating each rewritten return address with a unique location on the
> stack. As the return address is passed in the LR (and so is not
> guaranteed a unique location in memory), we use the FP upon entry to the
> function (i.e. the address of the caller's frame record) as the return
> address pointer. Any nested call will have a different FP value as the
> caller must create its own frame record and update FP to point to this.
> 
> This doesn't fix the issue of skipping a live LR at an exception
> boundary, which is a more general problem and requires more substantial
> rework. Note that were we to consume the LR in all cases this would in

would in?

> warnings where the interrupted context's LR contains
> `return_to_handler`, but the FP has been altered, e.g.
> 
> | func:
> |	<--- ftrace entry ---> 	// logs FP & LR, rewrites FP
> | 	STP	FP, LR, [SP, #-16]!
> | 	MOV	FP, SP
> | 	<--- INTERRUPT --->
> 
> ... as ftrace_graph_get_ret_stack() fill not find a matching entry,
> triggering the WARN_ON_ONCE() in unwind_frame().

Did you run the ftrace selftests with this? If not, please can you do it?

> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Madhavan T. Venkataraman <madvenka at linux.microsoft.com>
> Cc: Mark Brown <broonie at kernel.org>
> Cc: Will Deacon <will at kernel.org>

(Adding rostedt FYI)

> ---
>  arch/arm64/include/asm/ftrace.h     | 11 +++++++++++
>  arch/arm64/include/asm/stacktrace.h |  6 ------
>  arch/arm64/kernel/ftrace.c          |  6 +++---
>  arch/arm64/kernel/stacktrace.c      | 12 +++++-------
>  4 files changed, 19 insertions(+), 16 deletions(-)
> 
> I spotted this latent issue when reviewing Madhavan's stack tracing rework:
> 
>   https://lore.kernel.org/r/20211015025847.17694-1-madvenka@linux.microsoft.com
> 
> This supercedes the initial version I posted as an inline reply in:
> 
>   https://lore.kernel.org/r/20211025164925.GB2001@C02TD0UTHF1T.local

Please add a Link: tag referring to this.

> I believe we will need this as a prerequisite for other cleanups in that
> series. As it fixes an existing bug I'm posting it on its own.
> 
> Mark.
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 91fa4baa1a93..c96d47cb8f46 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -12,6 +12,17 @@
>  
>  #define HAVE_FUNCTION_GRAPH_FP_TEST
>  
> +/*
> + * HAVE_FUNCTION_GRAPH_RET_ADDR_PTR means that the architecture can provide a
> + * "return address pointer" which can be used to uniquely identify a return
> + * address which has been overwritten.
> + *
> + * On arm64 we use the address of the caller's frame record, which remains the
> + * same for the lifetime of the instrumented function, unlike the return
> + * address in the LR.
> + */
> +#define HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>  #define ARCH_SUPPORTS_FTRACE_OPS 1
>  #else
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 8aebc00c1718..9a319eca5cab 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -46,9 +46,6 @@ struct stack_info {
>   * @prev_type:   The type of stack this frame record was on, or a synthetic
>   *               value of STACK_TYPE_UNKNOWN. This is used to detect a
>   *               transition from one stack to another.
> - *
> - * @graph:       When FUNCTION_GRAPH_TRACER is selected, holds the index of a
> - *               replacement lr value in the ftrace graph stack.
>   */
>  struct stackframe {
>  	unsigned long fp;
> @@ -56,9 +53,6 @@ struct stackframe {
>  	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
>  	unsigned long prev_fp;
>  	enum stack_type prev_type;
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -	int graph;
> -#endif
>  };
>  
>  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 7f467bd9db7a..3e5d0ed63eb7 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -249,8 +249,6 @@ int __init ftrace_dyn_arch_init(void)
>   * on the way back to parent. For this purpose, this function is called
>   * in _mcount() or ftrace_caller() to replace return address (*parent) on
>   * the call stack to return_to_handler.
> - *
> - * Note that @frame_pointer is used only for sanity check later.
>   */
>  void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  			   unsigned long frame_pointer)
> @@ -268,8 +266,10 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  	 */
>  	old = *parent;
>  
> -	if (!function_graph_enter(old, self_addr, frame_pointer, NULL))
> +	if (!function_graph_enter(old, self_addr, frame_pointer,
> +	    (void *)frame_pointer)) {
>  		*parent = return_hooker;
> +	}
>  }
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 8982a2b78acf..749b680b4999 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -38,9 +38,6 @@ void start_backtrace(struct stackframe *frame, unsigned long fp,
>  {
>  	frame->fp = fp;
>  	frame->pc = pc;
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -	frame->graph = 0;
> -#endif
>  
>  	/*
>  	 * Prime the first unwind.
> @@ -116,17 +113,18 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  	if (tsk->ret_stack &&
>  		(ptrauth_strip_insn_pac(frame->pc) == (unsigned long)return_to_handler)) {
> -		struct ftrace_ret_stack *ret_stack;
> +		unsigned long orig_pc;
>  		/*
>  		 * This is a case where function graph tracer has
>  		 * modified a return address (LR) in a stack frame
>  		 * to hook a function return.
>  		 * So replace it to an original value.
>  		 */
> -		ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph++);
> -		if (WARN_ON_ONCE(!ret_stack))
> +		orig_pc = ftrace_graph_ret_addr(tsk, NULL, frame->pc,
> +						(void *)frame->fp);

Should we be using ptrauth_strip_insn_pac(frame->pc) again here?
ftrace_graph_ret_addr() appears to repeat the comparison against
return_to_handler.

> +		if (WARN_ON_ONCE(frame->pc == orig_pc))
>  			return -EINVAL;
> -		frame->pc = ret_stack->ret;
> +		frame->pc = orig_pc;
>  	}
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */

This will conflict (trivially) with the kretprobe stuff in -next but might
be worth routing via the tracing tree anyway.

Will