[PATCH 0/2] riscv: ftrace: use frame CFA as the function graph retp identity

[PATCH 0/2] riscv: ftrace: use frame CFA as the function graph retp identity

[Rui Qi]

RISC-V dynamic function graph tracing currently saves &fregs->ra as the
graph retp -- the lookup key that ftrace_graph_ret_addr() matches against
shadow stack entries. That address points into ftrace_caller's temporary
fregs frame, which disappears once ftrace_caller returns. Later stack
unwinding finds return_to_handler in the traced function's own frame and
looks it up with the frame CFA, so the saved entry can never match. The
same mismatch breaks function_get_true_parent_ip(), which looks up the
original parent with the saved entry SP rather than &fregs->ra.

Patch 1 clarifies the ftrace_graph_ret_addr() documentation: retp is
compared, not dereferenced, and may be any stable frame identity as long
as the graph entry path and the unwinder use the same value. 

Patch 2 makes RISC-V use the frame CFA for that identity: the static _mcount
path derives it from &frame->ra, the dynamic ftrace path uses the saved
entry SP, and the frame-pointer unwinder tracks the same CFA when
recovering graph return addresses.

Build-tested for rv64 with gcc and clang (full vmlinux).

Rui Qi (2):
  ftrace: Clarify function graph retp identity
  riscv: ftrace: Use frame CFA for function graph retp

 arch/riscv/kernel/ftrace.c     | 10 ++++++++--
 arch/riscv/kernel/stacktrace.c |  2 +-
 kernel/trace/fgraph.c          |  7 +++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH 1/2] ftrace: Clarify function graph retp identity

[Rui Qi]

ftrace_graph_ret_addr() does not dereference retp. It compares the value
against the retp saved by function_graph_enter*() to find the matching
graph return entry.

Update the documentation to describe retp as a return location identity.
Most architectures use the stack return address slot, but an architecture
may use another stable frame identity as long as its graph entry path and
unwinder use the same value.

Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 kernel/trace/fgraph.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index ed455b53513b..f95e2a9ed352 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -946,7 +946,7 @@ unsigned long ftrace_graph_top_ret_addr(struct task_struct *task)
  * @task: The task the unwinder is being executed on
  * @idx: An initialized pointer to the next stack index to use
  * @ret: The current return address (likely pointing to return_handler)
- * @retp: The address on the stack of the current return location
+ * @retp: The identity of the current return location
  *
  * This function can be called by stack unwinding code to convert a found stack
  * return address (@ret) to its original value, in case the function graph
@@ -959,7 +959,10 @@ unsigned long ftrace_graph_top_ret_addr(struct task_struct *task)
  * will be assigned that location so that if called again, it will continue
  * where it left off.
  *
- * @retp is a pointer to the return address on the stack.
+ * @retp is compared against the value saved by function_graph_enter*().
+ * It is usually the address of the return address on the stack, but may
+ * be another stable frame identity as long as the graph entry code and
+ * the unwinder use the same value.
  */
 unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
 				    unsigned long ret, unsigned long *retp)
-- 
2.20.1

[PATCH 2/2] riscv: ftrace: Use frame CFA for function graph retp

[Rui Qi]

RISC-V dynamic function graph tracing currently uses &fregs->ra for two
different purposes. As the parent argument, it is the temporary slot where
ftrace_caller saved the incoming ra. Reading that slot and replacing it
with return_to_handler is correct, because ftrace_caller reloads the slot
into the hardware ra register before returning to the traced function.

It is wrong to save the same address as the function graph retp. The retp
is not used to patch the return address later; ftrace_graph_ret_addr()
uses it as a lookup key for the shadow stack entry. Once ftrace_caller
returns, its temporary fregs frame is gone. Later stack unwinding finds
return_to_handler in the traced function's own frame, so the unwinder
cannot match a key that points back into the vanished ftrace_caller frame.

This also breaks function_get_true_parent_ip(), which looks up the
original parent with ftrace_regs_get_stack_pointer(fregs). On RISC-V that
is the saved entry SP, not &fregs->ra, so ftrace_graph_ret_addr() cannot
match the graph return entry.

Use the frame CFA as the RISC-V graph retp identity instead. The static
_mcount path derives it from &frame->ra, the dynamic ftrace path uses the
saved entry SP, and the frame-pointer unwinder uses the same CFA when
recovering graph return addresses.

Fixes: 35e61e8827ee ("riscv: ftrace: Make function graph use ftrace directly")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 arch/riscv/kernel/ftrace.c     | 10 ++++++++--
 arch/riscv/kernel/stacktrace.c |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index be8b68514417..faad2608216f 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -229,11 +229,16 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 /*
  * Most of this function is copied from arm64.
+ *
+ * Use the frame CFA as the RISC-V graph return address identity: static
+ * _mcount derives it from &frame->ra, dynamic ftrace uses the saved entry
+ * SP, and the unwinder tracks the same value when walking frame records.
  */
 void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 			   unsigned long frame_pointer)
 {
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
+	unsigned long *retp = parent + 1;
 	unsigned long old;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
@@ -245,7 +250,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	 */
 	old = *parent;
 
-	if (!function_graph_enter(old, self_addr, frame_pointer, parent))
+	if (!function_graph_enter(old, self_addr, frame_pointer, retp))
 		*parent = return_hooker;
 }
 
@@ -256,6 +261,7 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
 	unsigned long frame_pointer = arch_ftrace_regs(fregs)->s0;
 	unsigned long *parent = &arch_ftrace_regs(fregs)->ra;
+	unsigned long *retp = (unsigned long *)arch_ftrace_regs(fregs)->sp;
 	unsigned long old;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
@@ -267,7 +273,7 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
 	 */
 	old = *parent;
 
-	if (!function_graph_enter_regs(old, ip, frame_pointer, parent, fregs))
+	if (!function_graph_enter_regs(old, ip, frame_pointer, retp, fregs))
 		*parent = return_hooker;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index c7555447149b..96fb7d29ebb1 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -88,7 +88,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 			fp = READ_ONCE_TASK_STACK(task, frame->fp);
 			pc = READ_ONCE_TASK_STACK(task, frame->ra);
 			pc = ftrace_graph_ret_addr(task, &graph_idx, pc,
-						   &frame->ra);
+						   (unsigned long *)sp);
 			if (pc >= (unsigned long)handle_exception &&
 			    pc < (unsigned long)&ret_from_exception_end) {
 				if (unlikely(!fn(arg, pc)))
-- 
2.20.1