On Wed, 19 Feb 2025 17:04:39 -0500
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> From: Steven Rostedt <rostedt@xxxxxxxxxxx>
>
> When the last fprobe is removed, it calls unregister_ftrace_graph() to
> remove the graph_ops from function graph. The issue is when it does so, it
> calls return before removing the function from its graph ops via
> ftrace_set_filter_ips(). This leaves the last function lingering in the
> fprobe's fgraph ops and if a probe is added it also enables that last
> function (even though the callback will just drop it, it does add unneeded
> overhead to make that call).
>
> # echo "f:myevent1 kernel_clone" >> /sys/kernel/tracing/dynamic_events
> # cat /sys/kernel/tracing/enabled_functions
> kernel_clone (1) tramp: 0xffffffffc02f3000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60
>
> # echo "f:myevent2 schedule_timeout" >> /sys/kernel/tracing/dynamic_events
> # cat /sys/kernel/tracing/enabled_functions
> kernel_clone (1) tramp: 0xffffffffc02f3000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60
> schedule_timeout (1) tramp: 0xffffffffc02f3000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60
>
> # > /sys/kernel/tracing/dynamic_events
> # cat /sys/kernel/tracing/enabled_functions
>
> # echo "f:myevent3 kmem_cache_free" >> /sys/kernel/tracing/dynamic_events
> # cat /sys/kernel/tracing/enabled_functions
> kmem_cache_free (1) tramp: 0xffffffffc0219000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60
> schedule_timeout (1) tramp: 0xffffffffc0219000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60
>
> The above enabled a fprobe on kernel_clone, and then on schedule_timeout.
> The content of the enabled_functions shows the functions that have a
> callback attached to them. The fprobe attached to those functions
> properly. Then the fprobes were cleared, and enabled_functions was empty
> after that. But after adding a fprobe on kmem_cache_free, the
> enabled_functions shows that the schedule_timeout was attached again. This
> is because it was still left in the fprobe ops that is used to tell
> function graph what functions it wants callbacks from.
>
This looks good to me.
Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
Thanks!
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 4346ba1604093 ("fprobe: Rewrite fprobe on function-graph tracer")
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> ---
> kernel/trace/fprobe.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 2560b312ad57..62e8f7d56602 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -403,11 +403,9 @@ static void fprobe_graph_remove_ips(unsigned long *addrs, int num)
> lockdep_assert_held(&fprobe_mutex);
>
> fprobe_graph_active--;
> - if (!fprobe_graph_active) {
> - /* Q: should we unregister it ? */
> + /* Q: should we unregister it ? */
> + if (!fprobe_graph_active)
> unregister_ftrace_graph(&fprobe_graph_ops);
> - return;
> - }
>
> ftrace_set_filter_ips(&fprobe_graph_ops.ops, addrs, num, 1, 0);
> }
> --
> 2.47.2
>
>
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
