[PATCH bpf-next] bpf: Roll back freplace link state when bpf_arch_text_poke() fails

[PATCH bpf-next] bpf: Roll back freplace link state when bpf_arch_text_poke() fails

[chenyuan_fl]

From: Yuan Chen <chenyuan@kylinos.cn>

A freplace attach claims the target prog by bumping
tgt_prog->aux->freplace_link_cnt and setting tr->extension_prog.
bpf_arch_text_poke() then makes the extension take effect.  If the
poke fails, the claims are never released: the attach unwinds
through bpf_link_cleanup(), which clears link->prog, so
bpf_trampoline_unlink_prog() never runs.

Drop the link count under ext_mutex on the error path, and set
tr->extension_prog only after the poke succeeded.  The count is
still bumped before the poke: it blocks prog_array updates while
the entry is patched.

Fixes: d6083f040d5d ("bpf: Prevent tailcall infinite loop caused by freplace")
Suggested-by: Leon Hwang <leon.hwang@linux.dev>
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
 kernel/bpf/trampoline.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index da85bd580ef0..bf4ab0ac264e 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -973,10 +973,17 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_node *node,
 		err = bpf_freplace_link_tgt_prog(tgt_prog);
 		if (err)
 			return err;
+		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
+					 BPF_MOD_JUMP, NULL,
+					 node->link->prog->bpf_func);
+		if (err) {
+			/* Undo the claim from bpf_freplace_link_tgt_prog(). */
+			guard(mutex)(&tgt_prog->aux->ext_mutex);
+			tgt_prog->aux->freplace_link_cnt--;
+			return err;
+		}
 		tr->extension_prog = node->link->prog;
-		return bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
-					  BPF_MOD_JUMP, NULL,
-					  node->link->prog->bpf_func);
+		return 0;
 	}
 	err = bpf_trampoline_add_prog(tr, node, cnt);
 	if (err)
-- 
2.54.0

Re: [PATCH bpf-next] bpf: Roll back freplace link state when bpf_arch_text_poke() fails

[bot+bpf-ci]

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

> bpf: Roll back freplace link state when bpf_arch_text_poke() fails
>
> A freplace attach claims the target prog by bumping
> tgt_prog->aux->freplace_link_cnt and setting tr->extension_prog.
> bpf_arch_text_poke() then makes the extension take effect.  If the
> poke fails, the claims are never released: the attach unwinds
> through bpf_link_cleanup(), which clears link->prog, so
> bpf_trampoline_unlink_prog() never runs.
>
> Drop the link count under ext_mutex on the error path, and set
> tr->extension_prog only after the poke succeeded.  The count is
> still bumped before the poke: it blocks prog_array updates while
> the entry is patched.
>
> Fixes: d6083f040d5d ("bpf: Prevent tailcall infinite loop caused by freplace")
> Suggested-by: Leon Hwang <leon.hwang@linux.dev>
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>

The Fixes: tag points to d6083f040d5d, but should this be commit
c43c348a8446 instead?

Looking at the commit history, c43c348a8446 ("bpf: Keep target extended
until its last freplace link detaches") introduced the freplace_link_cnt
field and the bpf_freplace_link_tgt_prog() function that increments it.

The bug being fixed here is that when bpf_freplace_link_tgt_prog() succeeds
but bpf_arch_text_poke() subsequently fails, the incremented count is not
rolled back. This exact code pattern was introduced by c43c348a8446, not
d6083f040d5d.


---
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/36096074687

Re: [PATCH bpf-next] bpf: Roll back freplace link state when bpf_arch_text_poke() fails

[Leon Hwang]

On 25/9/26 12:35, chenyuan_fl@163.com wrote:
> From: Yuan Chen <chenyuan@kylinos.cn>
> 
> A freplace attach claims the target prog by bumping
> tgt_prog->aux->freplace_link_cnt and setting tr->extension_prog.
> bpf_arch_text_poke() then makes the extension take effect.  If the
> poke fails, the claims are never released: the attach unwinds
> through bpf_link_cleanup(), which clears link->prog, so
> bpf_trampoline_unlink_prog() never runs.
> 
> Drop the link count under ext_mutex on the error path, and set
> tr->extension_prog only after the poke succeeded.  The count is
> still bumped before the poke: it blocks prog_array updates while
> the entry is patched.
> 
> Fixes: d6083f040d5d ("bpf: Prevent tailcall infinite loop caused by freplace")

Your commit msg also says the issue about tr->extension_prog.

An extra Fixes tag for tr->extension_prog should be added.

Fixes: be8704ff07d2 ("bpf: Introduce dynamic program extensions")

The changes below lgtm:

Acked-by: Leon Hwang <leon.hwang@linux.dev>

Thanks,
Leon

> Suggested-by: Leon Hwang <leon.hwang@linux.dev>
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
> ---
>  kernel/bpf/trampoline.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index da85bd580ef0..bf4ab0ac264e 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -973,10 +973,17 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_node *node,
>  		err = bpf_freplace_link_tgt_prog(tgt_prog);
>  		if (err)
>  			return err;
> +		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
> +					 BPF_MOD_JUMP, NULL,
> +					 node->link->prog->bpf_func);
> +		if (err) {
> +			/* Undo the claim from bpf_freplace_link_tgt_prog(). */
> +			guard(mutex)(&tgt_prog->aux->ext_mutex);
> +			tgt_prog->aux->freplace_link_cnt--;
> +			return err;
> +		}
>  		tr->extension_prog = node->link->prog;
> -		return bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
> -					  BPF_MOD_JUMP, NULL,
> -					  node->link->prog->bpf_func);
> +		return 0;
>  	}
>  	err = bpf_trampoline_add_prog(tr, node, cnt);
>  	if (err)