Skip to content

Commit 29c1c24

Browse files
committed
function_graph: Fix up ftrace_graph_ret_addr()
Yang Li sent a patch to fix the kerneldoc of ftrace_graph_ret_addr(). While reviewing it, I realized that the comments in the entire function header needed a rewrite. When doing that, I realized that @idx parameter was being ignored. Every time this was called by the unwinder, it would start the loop at the top of the shadow stack and look for the matching stack pointer. When it found it, it would return it. When the unwinder asked for the next function, it would search from the beginning again. In reality, it should start from where it left off. That was the reason for the @idx parameter in the first place. The first time the unwinder calls this function, the @idx pointer would contain zero. That would mean to start from the top of the stack. The function was supposed to update the @idx with the index where it found the return address, so that the next time the unwinder calls this function it doesn't have to search through the previous addresses it found (making it O(n^2)!). This speeds up the unwinder's use of ftrace_graph_ret_addr() by an order of magnitude. Link: https://lore.kernel.org/linux-trace-kernel/[email protected]/ Link: https://lore.kernel.org/linux-trace-kernel/[email protected] Cc: Masami Hiramatsu <[email protected]> Cc: Mark Rutland <[email protected]> Cc: Mathieu Desnoyers <[email protected]> Cc: Jonathan Corbet <[email protected]> Cc: Catalin Marinas <[email protected]> Cc: Will Deacon <[email protected]> Cc: Guo Ren <[email protected]> Cc: Huacai Chen <[email protected]> Cc: WANG Xuerui <[email protected]> Cc: Michael Ellerman <[email protected]> Cc: Nicholas Piggin <[email protected]> Cc: Christophe Leroy <[email protected]> Cc: "Naveen N. Rao" <[email protected]> Cc: Paul Walmsley <[email protected]> Cc: Palmer Dabbelt <[email protected]> Cc: Albert Ou <[email protected]> Cc: Heiko Carstens <[email protected]> Cc: Vasily Gorbik <[email protected]> Cc: Alexander Gordeev <[email protected]> Cc: Christian Borntraeger <[email protected]> Cc: Sven Schnelle <[email protected]> Cc: Thomas Gleixner <[email protected]> Cc: Ingo Molnar <[email protected]> Cc: Borislav Petkov <[email protected]> Cc: Dave Hansen <[email protected]> Cc: "H. Peter Anvin" <[email protected]> Reported-by: Yang Li <[email protected]> Fixes: 7aa1eae ("function_graph: Allow multiple users to attach to function graph") Signed-off-by: Steven Rostedt (Google) <[email protected]>
1 parent 4267fda commit 29c1c24

File tree

1 file changed

+20
-8
lines changed

1 file changed

+20
-8
lines changed

kernel/trace/fgraph.c

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -870,18 +870,24 @@ ftrace_graph_get_ret_stack(struct task_struct *task, int idx)
870870
}
871871

872872
/**
873-
* ftrace_graph_ret_addr - convert a potentially modified stack return address
874-
* to its original value
873+
* ftrace_graph_ret_addr - return the original value of the return address
874+
* @task: The task the unwinder is being executed on
875+
* @idx: An initialized pointer to the next stack index to use
876+
* @ret: The current return address (likely pointing to return_handler)
877+
* @retp: The address on the stack of the current return location
875878
*
876879
* This function can be called by stack unwinding code to convert a found stack
877-
* return address ('ret') to its original value, in case the function graph
880+
* return address (@ret) to its original value, in case the function graph
878881
* tracer has modified it to be 'return_to_handler'. If the address hasn't
879-
* been modified, the unchanged value of 'ret' is returned.
882+
* been modified, the unchanged value of @ret is returned.
880883
*
881-
* 'idx' is a state variable which should be initialized by the caller to zero
882-
* before the first call.
884+
* @idx holds the last index used to know where to start from. It should be
885+
* initialized to zero for the first iteration as that will mean to start
886+
* at the top of the shadow stack. If the location is found, this pointer
887+
* will be assigned that location so that if called again, it will continue
888+
* where it left off.
883889
*
884-
* 'retp' is a pointer to the return address on the stack. It's ignored if
890+
* @retp is a pointer to the return address on the stack. It's ignored if
885891
* the arch doesn't have HAVE_FUNCTION_GRAPH_RET_ADDR_PTR defined.
886892
*/
887893
#ifdef HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
@@ -895,6 +901,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
895901
if (ret != return_handler)
896902
return ret;
897903

904+
if (!idx)
905+
return ret;
906+
907+
i = *idx ? : task->curr_ret_stack;
898908
while (i > 0) {
899909
ret_stack = get_ret_stack(current, i, &i);
900910
if (!ret_stack)
@@ -908,8 +918,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
908918
* Thus we will continue to find real return address.
909919
*/
910920
if (ret_stack->retp == retp &&
911-
ret_stack->ret != return_handler)
921+
ret_stack->ret != return_handler) {
922+
*idx = i;
912923
return ret_stack->ret;
924+
}
913925
}
914926

915927
return ret;

0 commit comments

Comments
 (0)