Re: [PATCH bpf-next 1/3] ftrace: Fix BPF fexit with livepatch

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Oct 24, 2025 at 03:42:44PM +0000, Song Liu wrote:
> 
> 
> > On Oct 24, 2025, at 4:42 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
> > 
> > On Fri, Oct 24, 2025 at 12:12:55AM -0700, Song Liu wrote:
> >> When livepatch is attached to the same function as bpf trampoline with
> >> a fexit program, bpf trampoline code calls register_ftrace_direct()
> >> twice. The first time will fail with -EAGAIN, and the second time it
> >> will succeed. This requires register_ftrace_direct() to unregister
> >> the address on the first attempt. Otherwise, the bpf trampoline cannot
> >> attach. Here is an easy way to reproduce this issue:
> >> 
> >>  insmod samples/livepatch/livepatch-sample.ko
> >>  bpftrace -e 'fexit:cmdline_proc_show {}'
> >>  ERROR: Unable to attach probe: fexit:vmlinux:cmdline_proc_show...
> >> 
> >> Fix this by cleaning up the hash when register_ftrace_function_nolock hits
> >> errors.
> >> 
> >> Fixes: d05cb470663a ("ftrace: Fix modification of direct_function hash while in use")
> >> Cc: stable@vger.kernel.org # v6.6+
> >> Reported-by: Andrey Grodzovsky <andrey.grodzovsky@crowdstrike.com>
> >> Closes: https://lore.kernel.org/live-patching/c5058315a39d4615b333e485893345be@crowdstrike.com/ 
> >> Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
> >> Cc: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >> Acked-and-tested-by: Andrey Grodzovsky <andrey.grodzovsky@crowdstrike.com>
> >> Signed-off-by: Song Liu <song@kernel.org>
> >> ---
> >> kernel/trace/ftrace.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >> index 42bd2ba68a82..7f432775a6b5 100644
> >> --- a/kernel/trace/ftrace.c
> >> +++ b/kernel/trace/ftrace.c
> >> @@ -6048,6 +6048,8 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
> >> ops->direct_call = addr;
> >> 
> >> err = register_ftrace_function_nolock(ops);
> >> + if (err)
> >> + remove_direct_functions_hash(hash, addr);
> > 
> > should this be handled by the caller of the register_ftrace_direct?
> > fops->hash is updated by ftrace_set_filter_ip in register_fentry
> 
> We need to clean up here. This is because register_ftrace_direct added 
> the new entries to direct_functions. It need to clean these entries 
> for the caller so that the next call of register_ftrace_direct can 
> work. 
> 
> > seems like it's should be caller responsibility, also you could do that
> > just for (err == -EAGAIN) case to address the use case directly
> 
> The cleanup is valid for any error cases, as we need to remove unused
> entries from direct_functions. 

I see, I wonder then we could use free_hash to restore original
direct_functions, something like:

	if (err) {
		call_direct_funcs = rcu_assign_pointer(free_hash);
		free_hash = new_hash;
	}

we'd need to keep new_hash value

but feel free to ignore, removal is also fine ;-)

thanks,
jirka