Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

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/20240610181746.656e3759@gandalf.local.home/
Link: https://lore.kernel.org/linux-trace-kernel/20240611031737.821995106@goodmis.org

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Guo Ren <guoren@kernel.org>
Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: WANG Xuerui <kernel@xen0n.name>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Reported-by: Yang Li <yang.lee@linux.alibaba.com>
Fixes: 7aa1eaef9f428 ("function_graph: Allow multiple users to attach to function graph")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

+20 -8
+20 -8
kernel/trace/fgraph.c
··· 870 870 } 871 871 872 872 /** 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 875 878 * 876 879 * 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 878 881 * 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. 880 883 * 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. 883 889 * 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 885 891 * the arch doesn't have HAVE_FUNCTION_GRAPH_RET_ADDR_PTR defined. 886 892 */ 887 893 #ifdef HAVE_FUNCTION_GRAPH_RET_ADDR_PTR ··· 901 895 if (ret != return_handler) 902 896 return ret; 903 897 898 + if (!idx) 899 + return ret; 900 + 901 + i = *idx ? : task->curr_ret_stack; 904 902 while (i > 0) { 905 903 ret_stack = get_ret_stack(current, i, &i); 906 904 if (!ret_stack) ··· 918 908 * Thus we will continue to find real return address. 919 909 */ 920 910 if (ret_stack->retp == retp && 921 - ret_stack->ret != return_handler) 911 + ret_stack->ret != return_handler) { 912 + *idx = i; 922 913 return ret_stack->ret; 914 + } 923 915 } 924 916 925 917 return ret;