[PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[Jiri Olsa]

If unregister_fentry fails we still have trampoline image attached
to a function, so releasing it could trigger crash. Releasing the
trampoline image only when the unregister succeeds.

Cc: stable@vger.kernel.org
Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/trampoline.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index f02254a21585..01082ecc5c4f 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -618,8 +618,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 
 	if (total == 0) {
 		err = unregister_fentry(tr, orig_flags, tr->cur_image->image);
-		bpf_tramp_image_put(tr->cur_image);
-		tr->cur_image = NULL;
+		if (!err) {
+			bpf_tramp_image_put(tr->cur_image);
+			tr->cur_image = NULL;
+		}
 		goto out;
 	}
 
-- 
2.53.0

[PATCH bpf 2/2] bpf: Remove obsolete WARN_ON call

[Jiri Olsa]

The WARN_ON call in bpf_trampoline_update could never hit, because we
direct the code path with (total == 0) to out label, which effectively
skips the WARN_ON call.

The WARN_ON made sense back then when it checked tr->selector, but now
with total being set just inside the function it's useless.

Cc: stable@vger.kernel.org
Fixes: 47e79cbeea4b ("bpf: Remove bpf trampoline selector")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/trampoline.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 01082ecc5c4f..c09d64b83fa4 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -687,7 +687,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 	if (err)
 		goto out_free;
 
-	WARN_ON(tr->cur_image && total == 0);
 	if (tr->cur_image)
 		/* progs already running at this address */
 		err = modify_fentry(tr, orig_flags, tr->cur_image->image,
-- 
2.53.0

Re: [PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[Song Liu]

On Fri, Apr 24, 2026 at 8:39 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> If unregister_fentry fails we still have trampoline image attached
> to a function, so releasing it could trigger crash. Releasing the
> trampoline image only when the unregister succeeds.
>
> Cc: stable@vger.kernel.org
> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

The fix looks good.

Acked-by: Song Liu <song@kernel.org>

Can we add a test case that triggers this crash without the fix?

Thanks,
Song

Re: [PATCH bpf 2/2] bpf: Remove obsolete WARN_ON call

[Song Liu]

On Fri, Apr 24, 2026 at 8:39 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The WARN_ON call in bpf_trampoline_update could never hit, because we
> direct the code path with (total == 0) to out label, which effectively
> skips the WARN_ON call.
>
> The WARN_ON made sense back then when it checked tr->selector, but now
> with total being set just inside the function it's useless.
>
> Cc: stable@vger.kernel.org
> Fixes: 47e79cbeea4b ("bpf: Remove bpf trampoline selector")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <song@kernel.org>

Re: [PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[sashiko-bot]

Sashiko AI review found 3 potential issue(s):
- [Critical] Use-After-Free of BPF programs due to leaked trampoline image
- [High] Permanent memory leak of trampoline executable image
- [High] Use-After-Free of ftrace_ops and bpf_trampoline
--

commit 3c4d55e24f28fa5f7f6dd937603366805532d1a4
Author: Jiri Olsa <jolsa@kernel.org>

bpf: Do not release trampoline image in case off unregister error

If unregister_fentry fails we still have trampoline image attached
to a function, so releasing it could trigger crash. Releasing the
trampoline image only when the unregister succeeds.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index f02254a21585fa..01082ecc5c4f5f 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -618,8 +618,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>  
>  	if (total == 0) {
>  		err = unregister_fentry(tr, orig_flags, tr->cur_image->image);
> -		bpf_tramp_image_put(tr->cur_image);
> -		tr->cur_image = NULL;
> +		if (!err) {
> +			bpf_tramp_image_put(tr->cur_image);
> +			tr->cur_image = NULL;
> +		}

If unregister_fentry() fails, the ftrace or fentry hook remains active because
the image isn't put. Does the teardown path, like bpf_tracing_link_release(),
still proceed to tear down the link and free the bpf_prog structures?

If the BPF programs are freed but the leaked JIT executable image is still
attached and active, could subsequent calls to the hooked kernel function
execute the trampoline image, dereference the hardcoded pointers, and execute
freed BPF programs?

When unregister_fentry() fails and bpf_tramp_image_put() is skipped, does the
error propagate up to the caller where bpf_trampoline_put() might eventually
free the parent struct bpf_trampoline?

If the parent trampoline is destroyed without freeing tr->cur_image, does this
leak the struct bpf_tramp_image and its associated JIT memory page?

Additionally, if unregister_fentry() fails for an ftrace-managed trampoline,
tr->fops appears to remain registered in the ftrace subsystem.

If the link teardown ignores the error and unconditionally calls
bpf_trampoline_put(tr), will direct_ops_free() free tr->fops and tr?

Could this leave the ftrace subsystem with dangling pointers in its internal
tracking structures, potentially causing a use-after-free regression during
subsequent ftrace operations?

>  		goto out;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260424153905.354922-1-jolsa@kernel.org?part=1

Re: [PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

> bpf: Do not release trampoline image in case off unregister error

This isn't a bug, but there's a typo in the subject line: "case off"
should be "case of".


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24898396843

Re: [PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[Jiri Olsa]

On Fri, Apr 24, 2026 at 08:50:51AM -0700, Song Liu wrote:
> On Fri, Apr 24, 2026 at 8:39 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > If unregister_fentry fails we still have trampoline image attached
> > to a function, so releasing it could trigger crash. Releasing the
> > trampoline image only when the unregister succeeds.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> The fix looks good.
> 
> Acked-by: Song Liu <song@kernel.org>
> 
> Can we add a test case that triggers this crash without the fix?

I don't about valid scenario where unregister_fentry would fail now,
that'd be bug that we'd need to fix. We have WARN_ON_ONCE on tampoline
unlink fail.

The fix is meant for when this happens let's go with un-released
trampoline image rather than kernel crash.

jirka

Re: [PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[Jiri Olsa]

On Fri, Apr 24, 2026 at 04:12:31PM +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 3 potential issue(s):
> - [Critical] Use-After-Free of BPF programs due to leaked trampoline image
> - [High] Permanent memory leak of trampoline executable image
> - [High] Use-After-Free of ftrace_ops and bpf_trampoline
> --
> 
> commit 3c4d55e24f28fa5f7f6dd937603366805532d1a4
> Author: Jiri Olsa <jolsa@kernel.org>
> 
> bpf: Do not release trampoline image in case off unregister error
> 
> If unregister_fentry fails we still have trampoline image attached
> to a function, so releasing it could trigger crash. Releasing the
> trampoline image only when the unregister succeeds.
> 
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index f02254a21585fa..01082ecc5c4f5f 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -618,8 +618,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
> >  
> >  	if (total == 0) {
> >  		err = unregister_fentry(tr, orig_flags, tr->cur_image->image);
> > -		bpf_tramp_image_put(tr->cur_image);
> > -		tr->cur_image = NULL;
> > +		if (!err) {
> > +			bpf_tramp_image_put(tr->cur_image);
> > +			tr->cur_image = NULL;
> > +		}
> 
> If unregister_fentry() fails, the ftrace or fentry hook remains active because
> the image isn't put. Does the teardown path, like bpf_tracing_link_release(),
> still proceed to tear down the link and free the bpf_prog structures?
> 
> If the BPF programs are freed but the leaked JIT executable image is still
> attached and active, could subsequent calls to the hooked kernel function
> execute the trampoline image, dereference the hardcoded pointers, and execute
> freed BPF programs?
> 
> When unregister_fentry() fails and bpf_tramp_image_put() is skipped, does the
> error propagate up to the caller where bpf_trampoline_put() might eventually
> free the parent struct bpf_trampoline?
> 
> If the parent trampoline is destroyed without freeing tr->cur_image, does this
> leak the struct bpf_tramp_image and its associated JIT memory page?
> 
> Additionally, if unregister_fentry() fails for an ftrace-managed trampoline,
> tr->fops appears to remain registered in the ftrace subsystem.
> 
> If the link teardown ignores the error and unconditionally calls
> bpf_trampoline_put(tr), will direct_ops_free() free tr->fops and tr?
> 
> Could this leave the ftrace subsystem with dangling pointers in its internal
> tracking structures, potentially causing a use-after-free regression during
> subsequent ftrace operations?

ah right, released bpf_prog and bpf_trampoline will cause crash later,
maybe we could just inc bpf_prog ref and skip bpf_trampoline_put like
below for the tracing link release

jirka


---
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 3b1f0ba02f61..c609f15b146d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3505,9 +3505,12 @@ static void bpf_tracing_link_release(struct bpf_link *link)
 	struct bpf_tracing_link *tr_link =
 		container_of(link, struct bpf_tracing_link, link.link);
 
-	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
+	if (WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
 						tr_link->trampoline,
-						tr_link->tgt_prog));
+						tr_link->tgt_prog))) {
+		bpf_prog_inc(tr_link->link.link.prog);
+		return;
+	}
 
 	bpf_trampoline_put(tr_link->trampoline);