Re: [PATCH v2 3/5] fprobe: Always unregister fgraph function from ops

[Google]

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>