* [PATCH bpf-next 1/3] ftrace: Fix BPF fexit with livepatch [not found] <20251024071257.3956031-1-song@kernel.org> @ 2025-10-24 7:12 ` Song Liu 2025-10-24 11:42 ` Jiri Olsa 0 siblings, 1 reply; 4+ messages in thread From: Song Liu @ 2025-10-24 7:12 UTC (permalink / raw) To: bpf, linux-trace-kernel, live-patching Cc: ast, daniel, andrii, rostedt, andrey.grodzovsky, mhiramat, kernel-team, Song Liu, stable 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); out_unlock: mutex_unlock(&direct_mutex); -- 2.47.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 1/3] ftrace: Fix BPF fexit with livepatch 2025-10-24 7:12 ` [PATCH bpf-next 1/3] ftrace: Fix BPF fexit with livepatch Song Liu @ 2025-10-24 11:42 ` Jiri Olsa 2025-10-24 15:42 ` Song Liu 0 siblings, 1 reply; 4+ messages in thread From: Jiri Olsa @ 2025-10-24 11:42 UTC (permalink / raw) To: Song Liu Cc: bpf, linux-trace-kernel, live-patching, ast, daniel, andrii, rostedt, andrey.grodzovsky, mhiramat, kernel-team, stable 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 seems like it's should be caller responsibility, also you could do that just for (err == -EAGAIN) case to address the use case directly jirka > > out_unlock: > mutex_unlock(&direct_mutex); > -- > 2.47.3 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 1/3] ftrace: Fix BPF fexit with livepatch 2025-10-24 11:42 ` Jiri Olsa @ 2025-10-24 15:42 ` Song Liu 2025-10-24 18:51 ` Jiri Olsa 0 siblings, 1 reply; 4+ messages in thread From: Song Liu @ 2025-10-24 15:42 UTC (permalink / raw) To: Jiri Olsa Cc: Song Liu, bpf@vger.kernel.org, linux-trace-kernel@vger.kernel.org, live-patching@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, rostedt@goodmis.org, andrey.grodzovsky@crowdstrike.com, mhiramat@kernel.org, Kernel Team, stable@vger.kernel.org > 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. Thanks, Song ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 1/3] ftrace: Fix BPF fexit with livepatch 2025-10-24 15:42 ` Song Liu @ 2025-10-24 18:51 ` Jiri Olsa 0 siblings, 0 replies; 4+ messages in thread From: Jiri Olsa @ 2025-10-24 18:51 UTC (permalink / raw) To: Song Liu Cc: Jiri Olsa, Song Liu, bpf@vger.kernel.org, linux-trace-kernel@vger.kernel.org, live-patching@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, rostedt@goodmis.org, andrey.grodzovsky@crowdstrike.com, mhiramat@kernel.org, Kernel Team, stable@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-24 18:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20251024071257.3956031-1-song@kernel.org>
2025-10-24 7:12 ` [PATCH bpf-next 1/3] ftrace: Fix BPF fexit with livepatch Song Liu
2025-10-24 11:42 ` Jiri Olsa
2025-10-24 15:42 ` Song Liu
2025-10-24 18:51 ` Jiri Olsa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox