Re: [PATCH 3/5] fprobe: Fix accounting of when to unregister from function graph

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Tue, 18 Feb 2025 14:30:36 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> When adding a new fprobe, it will update the function hash to the
> functions the fprobe is attached to and register with function graph to
> have it call the registered functions. The fprobe_graph_active variable
> keeps track of the number of fprobes that are using function graph.
> 
> If two fprobes attach to the same function, it increments the
> fprobe_graph_active for each of them. But when they are removed, the first
> fprobe to be removed will see that the function it is attached to is also
> used by another fprobe and it will not remove that function from
> function_graph. The logic will skip decrementing the fprobe_graph_active
> variable.
> 
> This causes the fprobe_graph_active variable to not go to zero when all
> fprobes are removed, and in doing so it does not unregister from
> function graph. As the fgraph ops hash will now be empty, and an empty
> filter hash means all functions are enabled, this triggers function graph
> to add a callback to the fprobe infrastructure for every function!
> 
>  # echo "f:myevent1 kernel_clone" >> /sys/kernel/tracing/dynamic_events
>  # echo "f:myevent2 kernel_clone%return" >> /sys/kernel/tracing/dynamic_events
>  # cat /sys/kernel/tracing/enabled_functions
> kernel_clone (1)           	tramp: 0xffffffffc0024000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60
> 
>  # > /sys/kernel/tracing/dynamic_events
>  # cat /sys/kernel/tracing/enabled_functions
> trace_initcall_start_cb (1)             tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> run_init_process (1)            tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> try_to_run_init_process (1)             tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> x86_pmu_show_pmu_cap (1)                tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> cleanup_rapl_pmus (1)                   tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> uncore_free_pcibus_map (1)              tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> uncore_types_exit (1)                   tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> uncore_pci_exit.part.0 (1)              tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> kvm_shutdown (1)                tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> vmx_dump_msrs (1)               tramp: 0xffffffffc0026000 (function_trace_call+0x0/0x170) ->function_trace_call+0x0/0x170
> [..]
> 
>  # cat /sys/kernel/tracing/enabled_functions | wc -l
> 54702
> 
> If a fprobe is being removed and all its functions are also traced by
> other fprobes, still decrement the fprobe_graph_active counter.
> 


Ah, thanks! But I would like to change the fix a bit since
fprobe_graph_remove_ips() must be paired with fprobe_graph_add_ips()
to hide fprobe_graph_active counting.

Can you use this version?

diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 2560b312ad57..7d282c08549e 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -409,7 +409,8 @@ static void fprobe_graph_remove_ips(unsigned long *addrs, int num)
 		return;
 	}
 
-	ftrace_set_filter_ips(&fprobe_graph_ops.ops, addrs, num, 1, 0);
+	if (num)
+		ftrace_set_filter_ips(&fprobe_graph_ops.ops, addrs, num, 1, 0);
 }
 
 static int symbols_cmp(const void *a, const void *b)
@@ -679,8 +680,7 @@ int unregister_fprobe(struct fprobe *fp)
 	}
 	del_fprobe_hash(fp);
 
-	if (count)
-		fprobe_graph_remove_ips(addrs, count);
+	fprobe_graph_remove_ips(addrs, count);
 
 	kfree_rcu(hlist_array, rcu);
 	fp->hlist_array = NULL;


> Cc: stable@vger.kernel.org
> Fixes: 4346ba1604093 ("fprobe: Rewrite fprobe on function-graph tracer")
> Closes: https://lore.kernel.org/all/20250217114918.10397-A-hca@linux.ibm.com/
> Reported-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/fprobe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 2560b312ad57..90241091ca61 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -681,6 +681,8 @@ int unregister_fprobe(struct fprobe *fp)
>  
>  	if (count)
>  		fprobe_graph_remove_ips(addrs, count);
> +	else
> +		fprobe_graph_active--;
>  
>  	kfree_rcu(hlist_array, rcu);
>  	fp->hlist_array = NULL;
> -- 
> 2.47.2
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>