[PATCH bpf-next v7 0/2] bpf: Fix tailcall infinite loop caused by freplace

[PATCH bpf-next v7 0/2] bpf: Fix tailcall infinite loop caused by freplace

[Leon Hwang]

Previously, I addressed a tailcall infinite loop issue related to
trampolines[0].

In this patchset, I resolve a similar issue where a tailcall infinite loop
can occur due to the combination of tailcalls and freplace programs. The
fix prevents adding extended programs to the prog_array map and blocks the
extension of a tail callee program with freplace.

Key changes:

1. If a program or its subprogram has been extended by an freplace program,
   it can no longer be updated to a prog_array map.
2. If a program has been added to a prog_array map, neither it nor its
   subprograms can be extended by an freplace program.

Changes:
v6 -> v7:
  * Address comments from Alexei:
    * Rewrite commit message more imperative and consice with AI.
    * Extend bpf_trampoline_link_prog() and bpf_trampoline_unlink_prog()
      to link and unlink target prog for freplace prog.
    * Use guard(mutex)(&tgt_prog->aux->ext_mutex) instead of
      mutex_lock()&mutex_unlock() pair.
  * Address comment from Eduard:
    * Remove misplaced "Reported-by" and "Closes" tags.

v5 -> v6:
  * Fix a build warning reported by kernel test robot.

v4 -> v5:
  * Move code of linking/unlinking target prog of freplace to trampoline.c.
  * Address comments from Alexei:
    * Change type of prog_array_member_cnt to u64.
    * Combine two patches to one.

v3 -> v4:
  * Address comments from Eduard:
    * Rename 'tail_callee_cnt' to 'prog_array_member_cnt'.
    * Add comment to 'prog_array_member_cnt'.
    * Use a mutex to protect 'is_extended' and 'prog_array_member_cnt'.

v2 -> v3:
  * Address comments from Alexei:
    * Stop hacking JIT.
    * Prevent the specific use case at attach/update time.

v1 -> v2:
  * Address comment from Eduard:
    * Explain why nop5 and xor/nop3 are swapped at prologue.
  * Address comment from Alexei:
    * Disallow attaching tail_call_reachable freplace prog to
      not-tail_call_reachable target in verifier.
  * Update "bpf, arm64: Fix tailcall infinite loop caused by freplace" with
    latest arm64 JIT code.

Links:
[0] https://lore.kernel.org/bpf/20230912150442.2009-1-hffilwlqm@gmail.com/

Leon Hwang (2):
  bpf: Prevent tailcall infinite loop caused by freplace
  selftests/bpf: Add test to verify tailcall and freplace restrictions

 include/linux/bpf.h                           | 17 +++-
 kernel/bpf/arraymap.c                         | 23 ++++-
 kernel/bpf/core.c                             |  1 +
 kernel/bpf/syscall.c                          |  7 +-
 kernel/bpf/trampoline.c                       | 37 +++++--
 .../selftests/bpf/prog_tests/tailcalls.c      | 98 ++++++++++++++++++-
 .../testing/selftests/bpf/progs/tc_bpf2bpf.c  |  5 +-
 7 files changed, 168 insertions(+), 20 deletions(-)

-- 
2.44.0

[PATCH bpf-next v7 1/2] bpf: Prevent tailcall infinite loop caused by freplace

[Leon Hwang]

There is a potential infinite loop issue that can occur when using a
combination of tail calls and freplace.

In an upcoming selftest, the attach target for entry_freplace of
tailcall_freplace.c is subprog_tc of tc_bpf2bpf.c, while the tail call in
entry_freplace leads to entry_tc. This results in an infinite loop:

entry_tc -> subprog_tc -> entry_freplace --tailcall-> entry_tc.

The problem arises because the tail_call_cnt in entry_freplace resets to
zero each time entry_freplace is executed, causing the tail call mechanism
to never terminate, eventually leading to a kernel panic.

To fix this issue, the solution is twofold:

1. Prevent updating a program extended by an freplace program to a
   prog_array map.
2. Prevent extending a program that is already part of a prog_array map
   with an freplace program.

This ensures that:

* If a program or its subprogram has been extended by an freplace program,
  it can no longer be updated to a prog_array map.
* If a program has been added to a prog_array map, neither it nor its
  subprograms can be extended by an freplace program.

Additionally, fix a minor code style issue by replacing eight spaces with a
tab for proper formatting.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 include/linux/bpf.h     | 17 +++++++++++++----
 kernel/bpf/arraymap.c   | 23 ++++++++++++++++++++++-
 kernel/bpf/core.c       |  1 +
 kernel/bpf/syscall.c    |  7 ++++---
 kernel/bpf/trampoline.c | 37 +++++++++++++++++++++++++++++--------
 5 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 19d8ca8ac960f..0c216e71cec76 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1292,8 +1292,12 @@ void *__bpf_dynptr_data_rw(const struct bpf_dynptr_kern *ptr, u32 len);
 bool __bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr);
 
 #ifdef CONFIG_BPF_JIT
-int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr);
-int bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr);
+int bpf_trampoline_link_prog(struct bpf_tramp_link *link,
+			     struct bpf_trampoline *tr,
+			     struct bpf_prog *tgt_prog);
+int bpf_trampoline_unlink_prog(struct bpf_tramp_link *link,
+			       struct bpf_trampoline *tr,
+			       struct bpf_prog *tgt_prog);
 struct bpf_trampoline *bpf_trampoline_get(u64 key,
 					  struct bpf_attach_target_info *tgt_info);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
@@ -1374,12 +1378,14 @@ void bpf_jit_uncharge_modmem(u32 size);
 bool bpf_prog_has_trampoline(const struct bpf_prog *prog);
 #else
 static inline int bpf_trampoline_link_prog(struct bpf_tramp_link *link,
-					   struct bpf_trampoline *tr)
+					   struct bpf_trampoline *tr,
+					   struct bpf_prog *tgt_prog)
 {
 	return -ENOTSUPP;
 }
 static inline int bpf_trampoline_unlink_prog(struct bpf_tramp_link *link,
-					     struct bpf_trampoline *tr)
+					     struct bpf_trampoline *tr,
+					     struct bpf_prog *tgt_prog)
 {
 	return -ENOTSUPP;
 }
@@ -1483,6 +1489,9 @@ struct bpf_prog_aux {
 	bool xdp_has_frags;
 	bool exception_cb;
 	bool exception_boundary;
+	bool is_extended; /* true if extended by freplace program */
+	u64 prog_array_member_cnt; /* counts how many times as member of prog_array */
+	struct mutex ext_mutex; /* mutex for is_extended and prog_array_member_cnt */
 	struct bpf_arena *arena;
 	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
 	const struct btf_type *attach_func_proto;
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 79660e3fca4c1..f9bd63a74eee7 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -947,6 +947,7 @@ static void *prog_fd_array_get_ptr(struct bpf_map *map,
 				   struct file *map_file, int fd)
 {
 	struct bpf_prog *prog = bpf_prog_get(fd);
+	bool is_extended;
 
 	if (IS_ERR(prog))
 		return prog;
@@ -956,13 +957,33 @@ static void *prog_fd_array_get_ptr(struct bpf_map *map,
 		return ERR_PTR(-EINVAL);
 	}
 
+	mutex_lock(&prog->aux->ext_mutex);
+	is_extended = prog->aux->is_extended;
+	if (!is_extended)
+		prog->aux->prog_array_member_cnt++;
+	mutex_unlock(&prog->aux->ext_mutex);
+	if (is_extended) {
+		/* Extended prog can not be tail callee. It's to prevent a
+		 * potential infinite loop like:
+		 * tail callee prog entry -> tail callee prog subprog ->
+		 * freplace prog entry --tailcall-> tail callee prog entry.
+		 */
+		bpf_prog_put(prog);
+		return ERR_PTR(-EBUSY);
+	}
+
 	return prog;
 }
 
 static void prog_fd_array_put_ptr(struct bpf_map *map, void *ptr, bool need_defer)
 {
+	struct bpf_prog *prog = ptr;
+
+	mutex_lock(&prog->aux->ext_mutex);
+	prog->aux->prog_array_member_cnt--;
+	mutex_unlock(&prog->aux->ext_mutex);
 	/* bpf_prog is freed after one RCU or tasks trace grace period */
-	bpf_prog_put(ptr);
+	bpf_prog_put(prog);
 }
 
 static u32 prog_fd_array_sys_lookup_elem(void *ptr)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 5e77c58e06010..233ea78f8f1bd 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -131,6 +131,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag
 	INIT_LIST_HEAD_RCU(&fp->aux->ksym_prefix.lnode);
 #endif
 	mutex_init(&fp->aux->used_maps_mutex);
+	mutex_init(&fp->aux->ext_mutex);
 	mutex_init(&fp->aux->dst_mutex);
 
 	return fp;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca54..4d04d4d9c1f30 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3214,7 +3214,8 @@ static void bpf_tracing_link_release(struct bpf_link *link)
 		container_of(link, struct bpf_tracing_link, link.link);
 
 	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
-						tr_link->trampoline));
+						tr_link->trampoline,
+						tr_link->tgt_prog));
 
 	bpf_trampoline_put(tr_link->trampoline);
 
@@ -3354,7 +3355,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	 *   in prog->aux
 	 *
 	 * - if prog->aux->dst_trampoline is NULL, the program has already been
-         *   attached to a target and its initial target was cleared (below)
+	 *   attached to a target and its initial target was cleared (below)
 	 *
 	 * - if tgt_prog != NULL, the caller specified tgt_prog_fd +
 	 *   target_btf_id using the link_create API.
@@ -3429,7 +3430,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	if (err)
 		goto out_unlock;
 
-	err = bpf_trampoline_link_prog(&link->link, tr);
+	err = bpf_trampoline_link_prog(&link->link, tr, tgt_prog);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
 		link = NULL;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index f8302a5ca400d..99b86e3b28a41 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -523,7 +523,9 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
 	}
 }
 
-static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
+static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
+				      struct bpf_trampoline *tr,
+				      struct bpf_prog *tgt_prog)
 {
 	enum bpf_tramp_prog_type kind;
 	struct bpf_tramp_link *link_exiting;
@@ -544,6 +546,17 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
 		/* Cannot attach extension if fentry/fexit are in use. */
 		if (cnt)
 			return -EBUSY;
+		guard(mutex)(&tgt_prog->aux->ext_mutex);
+		if (tgt_prog->aux->prog_array_member_cnt)
+			/* Program extensions can not extend target prog when
+			 * the target prog has been updated to any prog_array
+			 * map as tail callee. It's to prevent a potential
+			 * infinite loop like:
+			 * tgt prog entry -> tgt prog subprog -> freplace prog
+			 * entry --tailcall-> tgt prog entry.
+			 */
+			return -EBUSY;
+		tgt_prog->aux->is_extended = true;
 		tr->extension_prog = link->link.prog;
 		return bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
 					  link->link.prog->bpf_func);
@@ -570,17 +583,21 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
 	return err;
 }
 
-int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
+int bpf_trampoline_link_prog(struct bpf_tramp_link *link,
+			     struct bpf_trampoline *tr,
+			     struct bpf_prog *tgt_prog)
 {
 	int err;
 
 	mutex_lock(&tr->mutex);
-	err = __bpf_trampoline_link_prog(link, tr);
+	err = __bpf_trampoline_link_prog(link, tr, tgt_prog);
 	mutex_unlock(&tr->mutex);
 	return err;
 }
 
-static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
+static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link,
+					struct bpf_trampoline *tr,
+					struct bpf_prog *tgt_prog)
 {
 	enum bpf_tramp_prog_type kind;
 	int err;
@@ -588,9 +605,11 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_
 	kind = bpf_attach_type_to_tramp(link->link.prog);
 	if (kind == BPF_TRAMP_REPLACE) {
 		WARN_ON_ONCE(!tr->extension_prog);
+		guard(mutex)(&tgt_prog->aux->ext_mutex);
 		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP,
 					 tr->extension_prog->bpf_func, NULL);
 		tr->extension_prog = NULL;
+		tgt_prog->aux->is_extended = false;
 		return err;
 	}
 	hlist_del_init(&link->tramp_hlist);
@@ -599,12 +618,14 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_
 }
 
 /* bpf_trampoline_unlink_prog() should never fail. */
-int bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
+int bpf_trampoline_unlink_prog(struct bpf_tramp_link *link,
+			       struct bpf_trampoline *tr,
+			       struct bpf_prog *tgt_prog)
 {
 	int err;
 
 	mutex_lock(&tr->mutex);
-	err = __bpf_trampoline_unlink_prog(link, tr);
+	err = __bpf_trampoline_unlink_prog(link, tr, tgt_prog);
 	mutex_unlock(&tr->mutex);
 	return err;
 }
@@ -619,7 +640,7 @@ static void bpf_shim_tramp_link_release(struct bpf_link *link)
 	if (!shim_link->trampoline)
 		return;
 
-	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&shim_link->link, shim_link->trampoline));
+	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&shim_link->link, shim_link->trampoline, NULL));
 	bpf_trampoline_put(shim_link->trampoline);
 }
 
@@ -733,7 +754,7 @@ int bpf_trampoline_link_cgroup_shim(struct bpf_prog *prog,
 		goto err;
 	}
 
-	err = __bpf_trampoline_link_prog(&shim_link->link, tr);
+	err = __bpf_trampoline_link_prog(&shim_link->link, tr, NULL);
 	if (err)
 		goto err;
 
-- 
2.44.0

Re: [PATCH bpf-next v7 1/2] bpf: Prevent tailcall infinite loop caused by freplace

[Alexei Starovoitov]

On Thu, Oct 10, 2024 at 8:39 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>
> -static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
> +static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
> +                                     struct bpf_trampoline *tr,
> +                                     struct bpf_prog *tgt_prog)
>  {
>         enum bpf_tramp_prog_type kind;
>         struct bpf_tramp_link *link_exiting;
> @@ -544,6 +546,17 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
>                 /* Cannot attach extension if fentry/fexit are in use. */
>                 if (cnt)
>                         return -EBUSY;
> +               guard(mutex)(&tgt_prog->aux->ext_mutex);
> +               if (tgt_prog->aux->prog_array_member_cnt)
> +                       /* Program extensions can not extend target prog when
> +                        * the target prog has been updated to any prog_array
> +                        * map as tail callee. It's to prevent a potential
> +                        * infinite loop like:
> +                        * tgt prog entry -> tgt prog subprog -> freplace prog
> +                        * entry --tailcall-> tgt prog entry.
> +                        */
> +                       return -EBUSY;
> +               tgt_prog->aux->is_extended = true;
>                 tr->extension_prog = link->link.prog;
>                 return bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
>                                           link->link.prog->bpf_func);

The suggestion to use guard(mutex) shouldn't be applied mindlessly.
Here you extend the mutex holding range all the way through
bpf_arch_text_poke().
This is wrong.

>         if (kind == BPF_TRAMP_REPLACE) {
>                 WARN_ON_ONCE(!tr->extension_prog);
> +               guard(mutex)(&tgt_prog->aux->ext_mutex);
>                 err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP,
>                                          tr->extension_prog->bpf_func, NULL);
>                 tr->extension_prog = NULL;
> +               tgt_prog->aux->is_extended = false;
>                 return err;

Same here. Clearly wrong to grab the mutex for the duration of poke.

Also Xu's suggestion makes sense to me.
"extension prog should not be tailcalled independently"

So I would disable such case as a part of this patch as well.

pw-bot: cr

Re: [PATCH bpf-next v7 1/2] bpf: Prevent tailcall infinite loop caused by freplace

[Leon Hwang]

On 11/10/24 01:09, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2024 at 8:39 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> -static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
>> +static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
>> +                                     struct bpf_trampoline *tr,
>> +                                     struct bpf_prog *tgt_prog)
>>  {
>>         enum bpf_tramp_prog_type kind;
>>         struct bpf_tramp_link *link_exiting;
>> @@ -544,6 +546,17 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
>>                 /* Cannot attach extension if fentry/fexit are in use. */
>>                 if (cnt)
>>                         return -EBUSY;
>> +               guard(mutex)(&tgt_prog->aux->ext_mutex);
>> +               if (tgt_prog->aux->prog_array_member_cnt)
>> +                       /* Program extensions can not extend target prog when
>> +                        * the target prog has been updated to any prog_array
>> +                        * map as tail callee. It's to prevent a potential
>> +                        * infinite loop like:
>> +                        * tgt prog entry -> tgt prog subprog -> freplace prog
>> +                        * entry --tailcall-> tgt prog entry.
>> +                        */
>> +                       return -EBUSY;
>> +               tgt_prog->aux->is_extended = true;
>>                 tr->extension_prog = link->link.prog;
>>                 return bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
>>                                           link->link.prog->bpf_func);
> 
> The suggestion to use guard(mutex) shouldn't be applied mindlessly.
> Here you extend the mutex holding range all the way through
> bpf_arch_text_poke().
> This is wrong.
> 

Understood. The guard(mutex) should indeed limit the mutex holding range
to as small as possible. I’ll adjust accordingly.

>>         if (kind == BPF_TRAMP_REPLACE) {
>>                 WARN_ON_ONCE(!tr->extension_prog);
>> +               guard(mutex)(&tgt_prog->aux->ext_mutex);
>>                 err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP,
>>                                          tr->extension_prog->bpf_func, NULL);
>>                 tr->extension_prog = NULL;
>> +               tgt_prog->aux->is_extended = false;
>>                 return err;
> 
> Same here. Clearly wrong to grab the mutex for the duration of poke.
> 

Ack.

> Also Xu's suggestion makes sense to me.
> "extension prog should not be tailcalled independently"
> 
> So I would disable such case as a part of this patch as well.
> 

I’m fine with adding this restriction.

However, it will break a use case that works on the 5.15 kernel:

libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> freplace B.

With this limitation, the chain 'freplace A --tailcall-> freplace B'
will no longer work.

To comply with the new restriction, the use case would need to be
updated to:

libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> XDP B.

> pw-bot: cr

Thanks,
Leon

Re: [PATCH bpf-next v7 1/2] bpf: Prevent tailcall infinite loop caused by freplace

[Alexei Starovoitov]

On Thu, Oct 10, 2024 at 8:27 PM Leon Hwang <leon.hwang@linux.dev> wrote:
>
>
>
> On 11/10/24 01:09, Alexei Starovoitov wrote:
> > On Thu, Oct 10, 2024 at 8:39 AM Leon Hwang <leon.hwang@linux.dev> wrote:
> >>
> >> -static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
> >> +static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
> >> +                                     struct bpf_trampoline *tr,
> >> +                                     struct bpf_prog *tgt_prog)
> >>  {
> >>         enum bpf_tramp_prog_type kind;
> >>         struct bpf_tramp_link *link_exiting;
> >> @@ -544,6 +546,17 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
> >>                 /* Cannot attach extension if fentry/fexit are in use. */
> >>                 if (cnt)
> >>                         return -EBUSY;
> >> +               guard(mutex)(&tgt_prog->aux->ext_mutex);
> >> +               if (tgt_prog->aux->prog_array_member_cnt)
> >> +                       /* Program extensions can not extend target prog when
> >> +                        * the target prog has been updated to any prog_array
> >> +                        * map as tail callee. It's to prevent a potential
> >> +                        * infinite loop like:
> >> +                        * tgt prog entry -> tgt prog subprog -> freplace prog
> >> +                        * entry --tailcall-> tgt prog entry.
> >> +                        */
> >> +                       return -EBUSY;
> >> +               tgt_prog->aux->is_extended = true;
> >>                 tr->extension_prog = link->link.prog;
> >>                 return bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
> >>                                           link->link.prog->bpf_func);
> >
> > The suggestion to use guard(mutex) shouldn't be applied mindlessly.
> > Here you extend the mutex holding range all the way through
> > bpf_arch_text_poke().
> > This is wrong.
> >
>
> Understood. The guard(mutex) should indeed limit the mutex holding range
> to as small as possible. I’ll adjust accordingly.
>
> >>         if (kind == BPF_TRAMP_REPLACE) {
> >>                 WARN_ON_ONCE(!tr->extension_prog);
> >> +               guard(mutex)(&tgt_prog->aux->ext_mutex);
> >>                 err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP,
> >>                                          tr->extension_prog->bpf_func, NULL);
> >>                 tr->extension_prog = NULL;
> >> +               tgt_prog->aux->is_extended = false;
> >>                 return err;
> >
> > Same here. Clearly wrong to grab the mutex for the duration of poke.
> >
>
> Ack.
>
> > Also Xu's suggestion makes sense to me.
> > "extension prog should not be tailcalled independently"
> >
> > So I would disable such case as a part of this patch as well.
> >
>
> I’m fine with adding this restriction.
>
> However, it will break a use case that works on the 5.15 kernel:
>
> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> freplace B.
>
> With this limitation, the chain 'freplace A --tailcall-> freplace B'
> will no longer work.
>
> To comply with the new restriction, the use case would need to be
> updated to:
>
> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> XDP B.

I don't believe libxdp is doing anything like this.
It makes no sense to load PROG_TYPE_EXT that is supposed to freplace
another subprog and _not_ proceed with the actual replacement.

tail_call-ing into EXT prog directly is likely very broken.
EXT prog doesn't have to have ctx.
Its arguments match the target global subprog which may not have ctx at all.

So it's not about disabling, it's fixing the bug.

Re: [PATCH bpf-next v7 1/2] bpf: Prevent tailcall infinite loop caused by freplace

[Leon Hwang]

On 2024/10/11 23:50, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2024 at 8:27 PM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>>
>>
>> On 11/10/24 01:09, Alexei Starovoitov wrote:
>>> On Thu, Oct 10, 2024 at 8:39 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>>>
>>> Also Xu's suggestion makes sense to me.
>>> "extension prog should not be tailcalled independently"
>>>
>>> So I would disable such case as a part of this patch as well.
>>>
>>
>> I’m fine with adding this restriction.
>>
>> However, it will break a use case that works on the 5.15 kernel:
>>
>> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> freplace B.
>>
>> With this limitation, the chain 'freplace A --tailcall-> freplace B'
>> will no longer work.
>>
>> To comply with the new restriction, the use case would need to be
>> updated to:
>>
>> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> XDP B.
> 
> I don't believe libxdp is doing anything like this.
> It makes no sense to load PROG_TYPE_EXT that is supposed to freplace
> another subprog and _not_ proceed with the actual replacement.
> 

Without the new restriction, it’s difficult to prevent such a use case,
even if it’s not aligned with the intended design of freplace.

> tail_call-ing into EXT prog directly is likely very broken.
> EXT prog doesn't have to have ctx.
> Its arguments match the target global subprog which may not have ctx at all.
> 

Let me share a simple example of the use case in question:

In the XDP program:

__noinline int
int subprog_xdp(struct xdp_md *xdp)
{
 	return xdp ? XDP_PASS : XDP_ABORTED;
}

SEC("xdp")
int entry_xdp(struct xdp_md *xdp)
{
	return subprog_xdp(xdp);
}

In the freplace entry:

struct {
	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
	__uint(max_entries, 1);
	__uint(key_size, sizeof(__u32));
	__uint(value_size, sizeof(__u32));
} jmp_table SEC(".maps");

SEC("freplace")
int entry_freplace(struct xdp_md *xdp)
{
	int ret = XDP_PASS;

	bpf_tail_call_static(xdp, &jmp_table, 0);
	__sink(ret);
	return ret;
}

In the freplace tail callee:

SEC("freplace")
int entry_tailcallee(struct xdp_md *xdp)
{
	return XDP_PASS;
}

In this case, the attach target of entry_freplace is subprog_xdp, and
the tail call target of entry_freplace is entry_tailcallee. The attach
target of entry_tailcallee is entry_xdp, but it doesn't proceed with the
actual replacement. As a result, the call chain becomes:

entry_xdp -> subprog_xdp -> entry_freplace --tailcall-> entry_tailcallee.

> So it's not about disabling, it's fixing the bug.

Indeed, let us proceed with implementing the change.

Thanks,
Leon

Re: [PATCH bpf-next v7 1/2] bpf: Prevent tailcall infinite loop caused by freplace

[Alexei Starovoitov]

On Mon, Oct 14, 2024 at 6:17 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>
>
>
> On 2024/10/11 23:50, Alexei Starovoitov wrote:
> > On Thu, Oct 10, 2024 at 8:27 PM Leon Hwang <leon.hwang@linux.dev> wrote:
> >>
> >>
> >>
> >> On 11/10/24 01:09, Alexei Starovoitov wrote:
> >>> On Thu, Oct 10, 2024 at 8:39 AM Leon Hwang <leon.hwang@linux.dev> wrote:
> >>>>
> >>> Also Xu's suggestion makes sense to me.
> >>> "extension prog should not be tailcalled independently"
> >>>
> >>> So I would disable such case as a part of this patch as well.
> >>>
> >>
> >> I’m fine with adding this restriction.
> >>
> >> However, it will break a use case that works on the 5.15 kernel:
> >>
> >> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> freplace B.
> >>
> >> With this limitation, the chain 'freplace A --tailcall-> freplace B'
> >> will no longer work.
> >>
> >> To comply with the new restriction, the use case would need to be
> >> updated to:
> >>
> >> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> XDP B.
> >
> > I don't believe libxdp is doing anything like this.
> > It makes no sense to load PROG_TYPE_EXT that is supposed to freplace
> > another subprog and _not_ proceed with the actual replacement.
> >
>
> Without the new restriction, it’s difficult to prevent such a use case,
> even if it’s not aligned with the intended design of freplace.
>
> > tail_call-ing into EXT prog directly is likely very broken.
> > EXT prog doesn't have to have ctx.
> > Its arguments match the target global subprog which may not have ctx at all.
> >
>
> Let me share a simple example of the use case in question:
>
> In the XDP program:
>
> __noinline int
> int subprog_xdp(struct xdp_md *xdp)
> {
>         return xdp ? XDP_PASS : XDP_ABORTED;
> }
>
> SEC("xdp")
> int entry_xdp(struct xdp_md *xdp)
> {
>         return subprog_xdp(xdp);
> }
>
> In the freplace entry:
>
> struct {
>         __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>         __uint(max_entries, 1);
>         __uint(key_size, sizeof(__u32));
>         __uint(value_size, sizeof(__u32));
> } jmp_table SEC(".maps");
>
> SEC("freplace")
> int entry_freplace(struct xdp_md *xdp)
> {
>         int ret = XDP_PASS;
>
>         bpf_tail_call_static(xdp, &jmp_table, 0);
>         __sink(ret);
>         return ret;
> }
>
> In the freplace tail callee:
>
> SEC("freplace")
> int entry_tailcallee(struct xdp_md *xdp)
> {
>         return XDP_PASS;
> }
>
> In this case, the attach target of entry_freplace is subprog_xdp, and
> the tail call target of entry_freplace is entry_tailcallee. The attach
> target of entry_tailcallee is entry_xdp, but it doesn't proceed with the
> actual replacement. As a result, the call chain becomes:
>
> entry_xdp -> subprog_xdp -> entry_freplace --tailcall-> entry_tailcallee.

exactly. above makes no practical application.
It's only a headache for the verifier and source of obscure bugs.

>
> > So it's not about disabling, it's fixing the bug.
>
> Indeed, let us proceed with implementing the change.

yep.

[PATCH bpf-next v7 2/2] selftests/bpf: Add test to verify tailcall and freplace restrictions

[Leon Hwang]

Add a test case to ensure that attaching a tail callee program with an
freplace program fails, and that updating an extended program to a
prog_array map is also prohibited.

This test is designed to prevent the potential infinite loop issue caused
by the combination of tail calls and freplace, ensuring the correct
behavior and stability of the system.

cd tools/testing/selftests/bpf; ./test_progs -t tailcalls
335/26  tailcalls/tailcall_bpf2bpf_freplace:OK
335     tailcalls:OK
Summary: 1/26 PASSED, 0 SKIPPED, 0 FAILED

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 .../selftests/bpf/prog_tests/tailcalls.c      | 98 ++++++++++++++++++-
 .../testing/selftests/bpf/progs/tc_bpf2bpf.c  |  5 +-
 2 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index 21c5a37846ade..244d1959bbc7f 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -1525,7 +1525,7 @@ static void test_tailcall_freplace(void)
 
 	prog_fd = bpf_program__fd(tc_skel->progs.entry_tc);
 	freplace_prog = freplace_skel->progs.entry_freplace;
-	err = bpf_program__set_attach_target(freplace_prog, prog_fd, "subprog");
+	err = bpf_program__set_attach_target(freplace_prog, prog_fd, "subprog_tc");
 	if (!ASSERT_OK(err, "set_attach_target"))
 		goto out;
 
@@ -1534,7 +1534,7 @@ static void test_tailcall_freplace(void)
 		goto out;
 
 	freplace_link = bpf_program__attach_freplace(freplace_prog, prog_fd,
-						     "subprog");
+						     "subprog_tc");
 	if (!ASSERT_OK_PTR(freplace_link, "attach_freplace"))
 		goto out;
 
@@ -1556,6 +1556,98 @@ static void test_tailcall_freplace(void)
 	tailcall_freplace__destroy(freplace_skel);
 }
 
+/* test_tailcall_bpf2bpf_freplace checks the failure that fails to attach a tail
+ * callee prog with freplace prog or fails to update an extended prog to
+ * prog_array map.
+ */
+static void test_tailcall_bpf2bpf_freplace(void)
+{
+	struct tailcall_freplace *freplace_skel = NULL;
+	struct bpf_link *freplace_link = NULL;
+	struct tc_bpf2bpf *tc_skel = NULL;
+	char buff[128] = {};
+	int prog_fd, map_fd;
+	int err, key;
+
+	LIBBPF_OPTS(bpf_test_run_opts, topts,
+		    .data_in = buff,
+		    .data_size_in = sizeof(buff),
+		    .repeat = 1,
+	);
+
+	tc_skel = tc_bpf2bpf__open_and_load();
+	if (!ASSERT_OK_PTR(tc_skel, "tc_bpf2bpf__open_and_load"))
+		goto out;
+
+	prog_fd = bpf_program__fd(tc_skel->progs.entry_tc);
+	freplace_skel = tailcall_freplace__open();
+	if (!ASSERT_OK_PTR(freplace_skel, "tailcall_freplace__open"))
+		goto out;
+
+	err = bpf_program__set_attach_target(freplace_skel->progs.entry_freplace,
+					     prog_fd, "subprog_tc");
+	if (!ASSERT_OK(err, "set_attach_target"))
+		goto out;
+
+	err = tailcall_freplace__load(freplace_skel);
+	if (!ASSERT_OK(err, "tailcall_freplace__load"))
+		goto out;
+
+	/* OK to attach then detach freplace prog. */
+
+	freplace_link = bpf_program__attach_freplace(freplace_skel->progs.entry_freplace,
+						     prog_fd, "subprog_tc");
+	if (!ASSERT_OK_PTR(freplace_link, "attach_freplace"))
+		goto out;
+
+	err = bpf_link__destroy(freplace_link);
+	if (!ASSERT_OK(err, "destroy link"))
+		goto out;
+
+	/* OK to update prog_array map then delete element from the map. */
+
+	key = 0;
+	map_fd = bpf_map__fd(freplace_skel->maps.jmp_table);
+	err = bpf_map_update_elem(map_fd, &key, &prog_fd, BPF_ANY);
+	if (!ASSERT_OK(err, "update jmp_table"))
+		goto out;
+
+	err = bpf_map_delete_elem(map_fd, &key);
+	if (!ASSERT_OK(err, "delete_elem from jmp_table"))
+		goto out;
+
+	/* Fail to attach a tail callee prog with freplace prog. */
+
+	err = bpf_map_update_elem(map_fd, &key, &prog_fd, BPF_ANY);
+	if (!ASSERT_OK(err, "update jmp_table"))
+		goto out;
+
+	freplace_link = bpf_program__attach_freplace(freplace_skel->progs.entry_freplace,
+						     prog_fd, "subprog_tc");
+	if (!ASSERT_ERR_PTR(freplace_link, "attach_freplace failure"))
+		goto out;
+
+	err = bpf_map_delete_elem(map_fd, &key);
+	if (!ASSERT_OK(err, "delete_elem from jmp_table"))
+		goto out;
+
+	/* Fail to update an extended prog to prog_array map. */
+
+	freplace_link = bpf_program__attach_freplace(freplace_skel->progs.entry_freplace,
+						     prog_fd, "subprog_tc");
+	if (!ASSERT_OK_PTR(freplace_link, "attach_freplace"))
+		goto out;
+
+	err = bpf_map_update_elem(map_fd, &key, &prog_fd, BPF_ANY);
+	if (!ASSERT_ERR(err, "update jmp_table failure"))
+		goto out;
+
+out:
+	bpf_link__destroy(freplace_link);
+	tailcall_freplace__destroy(freplace_skel);
+	tc_bpf2bpf__destroy(tc_skel);
+}
+
 void test_tailcalls(void)
 {
 	if (test__start_subtest("tailcall_1"))
@@ -1606,4 +1698,6 @@ void test_tailcalls(void)
 	test_tailcall_bpf2bpf_hierarchy_3();
 	if (test__start_subtest("tailcall_freplace"))
 		test_tailcall_freplace();
+	if (test__start_subtest("tailcall_bpf2bpf_freplace"))
+		test_tailcall_bpf2bpf_freplace();
 }
diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
index 8a0632c37839a..d1a57f7d09bd8 100644
--- a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
@@ -5,10 +5,11 @@
 #include "bpf_misc.h"
 
 __noinline
-int subprog(struct __sk_buff *skb)
+int subprog_tc(struct __sk_buff *skb)
 {
 	int ret = 1;
 
+	__sink(skb);
 	__sink(ret);
 	return ret;
 }
@@ -16,7 +17,7 @@ int subprog(struct __sk_buff *skb)
 SEC("tc")
 int entry_tc(struct __sk_buff *skb)
 {
-	return subprog(skb);
+	return subprog_tc(skb);
 }
 
 char __license[] SEC("license") = "GPL";
-- 
2.44.0