Skip to contentSkip to conversation

ftrace: Fixes for v6.13

PATCH In review Steven Rostedt 1 participant 4 messages Dec 17, 2024

Subsystems (3)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

v2 cover ftrace: Fixes for v6.13

Ftrace fixes for 6.13:

- Always try to initialize the idle functions when graph tracer starts

A bug was found that when a CPU is offline when graph tracing starts and then comes online, that CPU is not traced. The fix to that was to move the initialization of the idle shadow stack over to the hot plug online logic, which also handle onlined CPUs. The issue was that it removed the initialization of the shadow stack when graph tracing starts, but the callbacks to the hot plug logic do nothing if graph tracing isn't currently running. Although that fix fixed the onlining of a CPU during tracing, it broke the CPUs that were already online.

- Have microblaze not try to get the "true parent" in function tracing

If function tracing and graph tracing are both enabled at the same time the parent of the functions traced by the function tracer may sometimes be the graph tracing trampoline. The graph tracing hijacks the return pointer of the function to trace it, but that can interfere with the function tracing parent output. This was fixed by using the ftrace_graph_ret_addr() function passing in the kernel stack pointer using the ftrace_regs_get_stack_pointer() function. But Al Viro reported that Microblaze does not implement the kernel_stack_pointer(regs) helper function that ftrace_regs_get_stack_pointer() uses and fails to compile when function graph tracing is enabled.

It was first thought that this was a microblaze issue, but the real cause is that this only works when an architecture implements HAVE_DYNAMIC_FTRACE_WITH_ARGS, as a requirement for that config is to have ftrace always pass a valid ftrace_regs to the callbacks. That also means that the architecture supports ftrace_regs_get_stack_pointer() Microblaze does not set HAVE_DYNAMIC_FTRACE_WITH_ARGS nor does it implement ftrace_regs_get_stack_pointer() which caused it to fail to build. Only implement the "true parent" logic if an architecture has that config set.

Changes since v1: https://lore.kernel.org/all/[email protected]/

- Removed the hash-ptr fix as Linus was unhappy with the code it was fixing and I have another series to address that. It didn't even belong in this pull request, as this is the ftrace topic and that was a tracing topic.

- Properly fix the function_get_true_parent_ip(), which wasn't a microblaze issue at all, but an issue for any architecture that does not support HAVE_DYNAMIC_FTRACE_WITH_ARGS.

author maintainer
#1 lore raw

PATCH v2 1/2 fgraph: Still initialize idle shadow stacks when starting

From: Steven Rostedt <[email protected]>

A bug was discovered where the idle shadow stacks were not initialized for offline CPUs when starting function graph tracer, and when they came online they were not traced due to the missing shadow stack. To fix this, the idle task shadow stack initialization was moved to using the CPU hotplug callbacks. But it removed the initialization when the function graph was enabled. The problem here is that the hotplug callbacks are called when the CPUs come online, but the idle shadow stack initialization only happens if function graph is currently active. This caused the online CPUs to not get their shadow stack initialized.

The idle shadow stack initialization still needs to be done when the function graph is registered, as they will not be allocated if function graph is not registered.

---
 kernel/trace/fgraph.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
kernel/trace/fgraph.c +7 -1
index 0bf78517b5d4..ddedcb50917f 100644
@@ -1215,7 +1215,7 @@ void fgraph_update_pid_func(void)
 static int start_graph_tracing(void)
 {
 	unsigned long **ret_stack_list;
-	int ret;
+	int ret, cpu;
 
 	ret_stack_list = kcalloc(FTRACE_RETSTACK_ALLOC_SIZE,
 				 sizeof(*ret_stack_list), GFP_KERNEL);
@@ -1223,6 +1223,12 @@ static int start_graph_tracing(void)
 	if (!ret_stack_list)
 		return -ENOMEM;
 
+	/* The cpu_boot init_task->ret_stack will never be freed */
+	for_each_online_cpu(cpu) {
+		if (!idle_task(cpu)->ret_stack)
+			ftrace_graph_init_idle_task(idle_task(cpu), cpu);
+	}
+
 	do {
 		ret = alloc_retstack_tasklist(ret_stack_list);
 	} while (ret == -EAGAIN);
Signature · 1 line
2.45.2
author maintainer
#2 lore raw

PATCH v2 2/2 ftrace: Do not find "true_parent" if HAVE_DYNAMIC_FTRACE_WITH_ARGS is not set

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

When function tracing and function graph tracing are both enabled (in different instances) the "parent" of some of the function tracing events is "return_to_handler" which is the trampoline used by function graph tracing. To fix this, ftrace_get_true_parent_ip() was introduced that returns the "true" parent ip instead of the trampoline.

To do this, the ftrace_regs_get_stack_pointer() is used, which uses kernel_stack_pointer(). The problem is that microblaze does not implement kerenl_stack_pointer() so when function graph tracing is enabled, the build fails. But microblaze also does not enabled HAVE_DYNAMIC_FTRACE_WITH_ARGS. That option has to be enabled by the architecture to reliably get the values from the fregs parameter passed in. When that config is not set, the architecture can also pass in NULL, which is not tested for in that function and could cause the kernel to crash.

Cc Masami Hiramatsu <[email protected]>
Cc Mathieu Desnoyers <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Al Viro <[email protected]>
Cc Michal Simek <[email protected]>
Cc Jeff Xie <[email protected]>
Fixes 60b1f578b578 ("ftrace: Get the true parent ip for function tracer")
---
 kernel/trace/trace_functions.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/trace/trace_functions.c +2 -1
index 74c353164ca1..d358c9935164 100644
@@ -176,7 +176,8 @@ static void function_trace_start(struct trace_array *tr)
 	tracing_reset_online_cpus(&tr->array_buffer);
 }
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+/* fregs are guaranteed not to be NULL if HAVE_DYNAMIC_FTRACE_WITH_ARGS is set */
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
 static __always_inline unsigned long
 function_get_true_parent_ip(unsigned long parent_ip, struct ftrace_regs *fregs)
 {
Signature · 1 line
2.45.2
author maintainer
#3 lore raw
In reply to Steven Rostedt
Quoted 38 lines · On Tue, 17 Dec 2024 11:18:40 -0500 Steven Rostedt <[email protected]> wrote:

Ftrace fixes for 6.13:

- Always try to initialize the idle functions when graph tracer starts

A bug was found that when a CPU is offline when graph tracing starts and then comes online, that CPU is not traced. The fix to that was to move the initialization of the idle shadow stack over to the hot plug online logic, which also handle onlined CPUs. The issue was that it removed the initialization of the shadow stack when graph tracing starts, but the callbacks to the hot plug logic do nothing if graph tracing isn't currently running. Although that fix fixed the onlining of a CPU during tracing, it broke the CPUs that were already online.

- Have microblaze not try to get the "true parent" in function tracing

If function tracing and graph tracing are both enabled at the same time the parent of the functions traced by the function tracer may sometimes be the graph tracing trampoline. The graph tracing hijacks the return pointer of the function to trace it, but that can interfere with the function tracing parent output. This was fixed by using the ftrace_graph_ret_addr() function passing in the kernel stack pointer using the ftrace_regs_get_stack_pointer() function. But Al Viro reported that Microblaze does not implement the kernel_stack_pointer(regs) helper function that ftrace_regs_get_stack_pointer() uses and fails to compile when function graph tracing is enabled.

It was first thought that this was a microblaze issue, but the real cause is that this only works when an architecture implements HAVE_DYNAMIC_FTRACE_WITH_ARGS, as a requirement for that config is to have ftrace always pass a valid ftrace_regs to the callbacks. That also means that the architecture supports ftrace_regs_get_stack_pointer() Microblaze does not set HAVE_DYNAMIC_FTRACE_WITH_ARGS nor does it implement ftrace_regs_get_stack_pointer() which caused it to fail to build. Only implement the "true parent" logic if an architecture has that config set.

Changes since v1: https://lore.kernel.org/all/[email protected]/

Quoted 8 lines

- Removed the hash-ptr fix as Linus was unhappy with the code it was fixing and I have another series to address that. It didn't even belong in this pull request, as this is the ftrace topic and that was a tracing topic.

- Properly fix the function_get_true_parent_ip(), which wasn't a microblaze issue at all, but an issue for any architecture that does not support HAVE_DYNAMIC_FTRACE_WITH_ARGS.

I forgot to append the diffstat and branch where this lives:

git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
ftrace/fixes

Head SHA1: 166438a432d76c68d3f0da60667248f3c2303d6c

Steven Rostedt (2):
fgraph: Still initialize idle shadow stacks when starting
ftrace: Do not find "true_parent" if HAVE_DYNAMIC_FTRACE_WITH_ARGS is not set

----

 kernel/trace/fgraph.c          | 8 +++++++-
 kernel/trace/trace_functions.c | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)