[PATCH v3 bpf 1/3] ftrace: Fix BPF fexit with livepatch

[PATCH v3 bpf 1/3] ftrace: Fix BPF fexit with livepatch

[Song Liu]

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.

Also, move the code that resets ops->func and ops->trampoline to
the error path of register_ftrace_direct().

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/bpf/trampoline.c | 5 -----
 kernel/trace/ftrace.c   | 6 ++++++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 5949095e51c3..f2cb0b097093 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -479,11 +479,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 		 * BPF_TRAMP_F_SHARE_IPMODIFY is set, we can generate the
 		 * trampoline again, and retry register.
 		 */
-		/* reset fops->func and fops->trampoline for re-register */
-		tr->fops->func = NULL;
-		tr->fops->trampoline = 0;
-
-		/* free im memory and reallocate later */
 		bpf_tramp_image_free(im);
 		goto again;
 	}
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 42bd2ba68a82..725c224fb4e6 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6048,6 +6048,12 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
 	ops->direct_call = addr;
 
 	err = register_ftrace_function_nolock(ops);
+	if (err) {
+		/* cleanup for possible another register call */
+		ops->func = NULL;
+		ops->trampoline = 0;
+		remove_direct_functions_hash(hash, addr);
+	}
 
  out_unlock:
 	mutex_unlock(&direct_mutex);
-- 
2.47.3

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

[Jiri Olsa]

On Sun, Oct 26, 2025 at 01:54:43PM -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.
> 
> Also, move the code that resets ops->func and ops->trampoline to
> the error path of register_ftrace_direct().
> 
> 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/bpf/trampoline.c | 5 -----
>  kernel/trace/ftrace.c   | 6 ++++++
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 5949095e51c3..f2cb0b097093 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -479,11 +479,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>  		 * BPF_TRAMP_F_SHARE_IPMODIFY is set, we can generate the
>  		 * trampoline again, and retry register.
>  		 */
> -		/* reset fops->func and fops->trampoline for re-register */
> -		tr->fops->func = NULL;
> -		tr->fops->trampoline = 0;
> -
> -		/* free im memory and reallocate later */
>  		bpf_tramp_image_free(im);
>  		goto again;
>  	}
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 42bd2ba68a82..725c224fb4e6 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6048,6 +6048,12 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
>  	ops->direct_call = addr;
>  
>  	err = register_ftrace_function_nolock(ops);
> +	if (err) {
> +		/* cleanup for possible another register call */
> +		ops->func = NULL;
> +		ops->trampoline = 0;

nit, we could cleanup also flags and direct_call just to be complete,
but at the same time it does not seem to affect anything

jirka


> +		remove_direct_functions_hash(hash, addr);
> +	}
>  
>   out_unlock:
>  	mutex_unlock(&direct_mutex);
> -- 
> 2.47.3
> 

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

[Song Liu]

> On Oct 27, 2025, at 1:48 AM, Jiri Olsa <olsajiri@gmail.com> wrote:

[…]
> 
>> 
>> 
>> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
>> index 5949095e51c3..f2cb0b097093 100644
>> --- a/kernel/bpf/trampoline.c
>> +++ b/kernel/bpf/trampoline.c
>> @@ -479,11 +479,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>> * BPF_TRAMP_F_SHARE_IPMODIFY is set, we can generate the
>> * trampoline again, and retry register.
>> */
>> - /* reset fops->func and fops->trampoline for re-register */
>> - tr->fops->func = NULL;
>> - tr->fops->trampoline = 0;
>> -
>> - /* free im memory and reallocate later */
>> bpf_tramp_image_free(im);
>> goto again;
>> }
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 42bd2ba68a82..725c224fb4e6 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -6048,6 +6048,12 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
>> ops->direct_call = addr;
>> 
>> err = register_ftrace_function_nolock(ops);
>> + if (err) {
>> + /* cleanup for possible another register call */
>> + ops->func = NULL;
>> + ops->trampoline = 0;
> 
> nit, we could cleanup also flags and direct_call just to be complete,
> but at the same time it does not seem to affect anything

I actually thought about this and decided to use the same 
logic as unregister_ftrace_direct(). 

Thanks,
Song

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

[Steven Rostedt]

On Sun, 26 Oct 2025 13:54:43 -0700
Song Liu <song@kernel.org> wrote:

> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6048,6 +6048,12 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
>  	ops->direct_call = addr;
>  
>  	err = register_ftrace_function_nolock(ops);
> +	if (err) {
> +		/* cleanup for possible another register call */
> +		ops->func = NULL;
> +		ops->trampoline = 0;
> +		remove_direct_functions_hash(hash, addr);
> +	}
>  

As you AI bot noticed that it was missing what unregister_ftrace_direct()
does, instead, can we make a helper function that both use? This way it
will not get out of sync again.

static void reset_direct(struct ftrace_ops *ops, unsigned long addr)
{
	struct ftrace_hash *hash = ops->func_hash->filter_hash;

	ops->func = NULL;
	ops->trampoline = 0;
	remove_direct_functions_hash(hash, addr);
}

Then we could have:

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 0c91247a95ab..51c3f5d46fde 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6062,7 +6062,7 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
 
 	err = register_ftrace_function_nolock(ops);
 	if (err)
-		remove_direct_functions_hash(hash, addr);
+		reset_direct(ops, addr);
 
  out_unlock:
 	mutex_unlock(&direct_mutex);
@@ -6095,7 +6095,6 @@ EXPORT_SYMBOL_GPL(register_ftrace_direct);
 int unregister_ftrace_direct(struct ftrace_ops *ops, unsigned long addr,
 			     bool free_filters)
 {
-	struct ftrace_hash *hash = ops->func_hash->filter_hash;
 	int err;
 
 	if (check_direct_multi(ops))
@@ -6105,13 +6104,9 @@ int unregister_ftrace_direct(struct ftrace_ops *ops, unsigned long addr,
 
 	mutex_lock(&direct_mutex);
 	err = unregister_ftrace_function(ops);
-	remove_direct_functions_hash(hash, addr);
+	reset_direct(ops, addr);
 	mutex_unlock(&direct_mutex);
 
-	/* cleanup for possible another register call */
-	ops->func = NULL;
-	ops->trampoline = 0;
-
 	if (free_filters)
 		ftrace_free_filter(ops);
 	return err;

-- Steve

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

[Song Liu]

> On Oct 27, 2025, at 10:01 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Sun, 26 Oct 2025 13:54:43 -0700
> Song Liu <song@kernel.org> wrote:
> 
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -6048,6 +6048,12 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
>> ops->direct_call = addr;
>> 
>> err = register_ftrace_function_nolock(ops);
>> + if (err) {
>> + /* cleanup for possible another register call */
>> + ops->func = NULL;
>> + ops->trampoline = 0;
>> + remove_direct_functions_hash(hash, addr);
>> + }
>> 
> 
> As you AI bot noticed that it was missing what unregister_ftrace_direct()
> does, instead, can we make a helper function that both use? This way it
> will not get out of sync again.
> 
> static void reset_direct(struct ftrace_ops *ops, unsigned long addr)
> {
> struct ftrace_hash *hash = ops->func_hash->filter_hash;
> 
> ops->func = NULL;
> ops->trampoline = 0;
> remove_direct_functions_hash(hash, addr);
> }
> 
> Then we could have:
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 0c91247a95ab..51c3f5d46fde 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6062,7 +6062,7 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
> 
> err = register_ftrace_function_nolock(ops);
> if (err)
> - remove_direct_functions_hash(hash, addr);
> + reset_direct(ops, addr);
> 
>  out_unlock:
> mutex_unlock(&direct_mutex);
> @@ -6095,7 +6095,6 @@ EXPORT_SYMBOL_GPL(register_ftrace_direct);
> int unregister_ftrace_direct(struct ftrace_ops *ops, unsigned long addr,
>     bool free_filters)
> {
> - struct ftrace_hash *hash = ops->func_hash->filter_hash;
> int err;
> 
> if (check_direct_multi(ops))
> @@ -6105,13 +6104,9 @@ int unregister_ftrace_direct(struct ftrace_ops *ops, unsigned long addr,
> 
> mutex_lock(&direct_mutex);
> err = unregister_ftrace_function(ops);
> - remove_direct_functions_hash(hash, addr);
> + reset_direct(ops, addr);
> mutex_unlock(&direct_mutex);
> 
> - /* cleanup for possible another register call */
> - ops->func = NULL;
> - ops->trampoline = 0;
> -
> if (free_filters)
> ftrace_free_filter(ops);
> return err;

Make sense. I will use this in v4. 

Thanks,
Song