On Fri, Oct 24, 2025 at 03:42:44PM +0000, Song Liu wrote:
>
>
> > On Oct 24, 2025, at 4:42 AM, Jiri Olsa <[email protected]> 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: [email protected] # v6.6+
> >> Reported-by: Andrey Grodzovsky <[email protected]>
> >> Closes:
> >> https://lore.kernel.org/live-patching/[email protected]/
> >>
> >> Cc: Steven Rostedt (Google) <[email protected]>
> >> Cc: Masami Hiramatsu (Google) <[email protected]>
> >> Acked-and-tested-by: Andrey Grodzovsky <[email protected]>
> >> Signed-off-by: Song Liu <[email protected]>
> >> ---
> >> 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
