[RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop

[RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop

[Leon Hwang]

This patch series fixes a tailcall infinite loop on x64.

From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and tailcall
handling in JIT"), the tailcall on x64 works better than before.

From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF subprograms
for x64 JIT"), tailcall is able to run in BPF subprograms on x64.

From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF program
to other BPF programs"), BPF program is able to trace other BPF programs.

How about combining them all together?

1. FENTRY/FEXIT on a BPF subprogram.
2. A tailcall runs in the BPF subprogram.
3. The tailcall calls the subprogram's caller.

As a result, a tailcall infinite loop comes up. And the loop would halt
the machine.

As we know, in tail call context, the tail_call_cnt propagates by stack
and rax register between BPF subprograms. So do in trampolines.

How did I discover the bug?

From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth 256 for
subprogs with tailcalls"), the total stack size limits to around 8KiB.
Then, I write some bpf progs to validate the stack consuming, that are
tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on bpf2bpf[1].

At that time, accidently, I made a tailcall loop. And then the loop halted
my VM. Without the loop, the bpf progs would consume over 8KiB stack size.
But the _stack-overflow_ did not halt my VM.

With bpf_printk(), I confirmed that the tailcall count limit did not work
expectedly. Next, read the code and fix it.

Finally, unfortunately, I only fix it on x64 but other arches. As a
result, CI tests failed because this bug hasn't been fixed on s390x.

Some helps on s390x are requested.

v3 -> v4:
  * Suggestions from Maciej:
    * Separate tail_call_cnt initialisation to a single patch.
    * Unnecessary to check subprog.

v2 -> v3:
  * Suggestions from Alexei:
    * Fix comment style.
    * Remove FIXME comment.
    * Remove arch_prepare_bpf_trampoline() function comment update.
    * Remove the unnecessary 'tgt_prog->aux->func[subprog]->is_func' check.

[1]: https://github.com/Asphaltt/learn-by-example/tree/main/ebpf/tailcall-stackoverflow

Leon Hwang (4):
  bpf, x64: Comment tail_call_cnt initialisation
  bpf, x64: Fix tailcall infinite loop
  selftests/bpf: Correct map_fd to data_fd in tailcalls
  selftests/bpf: Add testcases for tailcall infinite loop fixing

 arch/x86/net/bpf_jit_comp.c                   |  32 +-
 include/linux/bpf.h                           |   5 +
 kernel/bpf/trampoline.c                       |   4 +-
 kernel/bpf/verifier.c                         |   3 +
 .../selftests/bpf/prog_tests/tailcalls.c      | 311 +++++++++++++++++-
 .../bpf/progs/tailcall_bpf2bpf_fentry.c       |  18 +
 .../bpf/progs/tailcall_bpf2bpf_fexit.c        |  18 +
 7 files changed, 377 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c


base-commit: 9930e4af4b509bcf6f060b09b16884f26102d110
-- 
2.41.0

[RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation

[Leon Hwang]

Without understanding emit_prologue(), it is really hard to figure out
where does tail_call_cnt come from, even though searching tail_call_cnt
in the whole kernel repo.

By adding these comments, it is a little bit easy to understand
tail_call_cnt initialisation.

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 arch/x86/net/bpf_jit_comp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a5930042139d3..bcca1c9b9a027 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
 	prog += X86_PATCH_SIZE;
 	if (!ebpf_from_cbpf) {
 		if (tail_call_reachable && !is_subprog)
+			/* When it's the entry of the whole tailcall context,
+			 * zeroing rax means initialising tail_call_cnt.
+			 */
 			EMIT2(0x31, 0xC0); /* xor eax, eax */
 		else
+			/* Keep the same instruction layout. */
 			EMIT2(0x66, 0x90); /* nop2 */
 	}
 	EMIT1(0x55);             /* push rbp */
-- 
2.41.0

Re: [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation

[Maciej Fijalkowski]

On Sun, Sep 03, 2023 at 11:14:45PM +0800, Leon Hwang wrote:
> Without understanding emit_prologue(), it is really hard to figure out
> where does tail_call_cnt come from, even though searching tail_call_cnt
> in the whole kernel repo.
> 
> By adding these comments, it is a little bit easy to understand

s/easy/easier

> tail_call_cnt initialisation.
> 
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>

I was rather thinking about dropping these comments altogether. We should
be trying to provide fixes as small as possible IMHO.

Having this as a separate commit also breaks logistics of this set as
the fix + selftest should be routed towards bpf tree, whereas this
particular commit is rather a bpf-next candidate.

PTAL at https://www.kernel.org/doc/html/latest/bpf/bpf_devel_QA.html

> ---
>  arch/x86/net/bpf_jit_comp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index a5930042139d3..bcca1c9b9a027 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
>  	prog += X86_PATCH_SIZE;
>  	if (!ebpf_from_cbpf) {
>  		if (tail_call_reachable && !is_subprog)
> +			/* When it's the entry of the whole tailcall context,
> +			 * zeroing rax means initialising tail_call_cnt.
> +			 */
>  			EMIT2(0x31, 0xC0); /* xor eax, eax */
>  		else
> +			/* Keep the same instruction layout. */
>  			EMIT2(0x66, 0x90); /* nop2 */
>  	}
>  	EMIT1(0x55);             /* push rbp */
> -- 
> 2.41.0
> 

Re: [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation

[Leon Hwang]

On 6/9/23 03:26, Maciej Fijalkowski wrote:
> On Sun, Sep 03, 2023 at 11:14:45PM +0800, Leon Hwang wrote:
>> Without understanding emit_prologue(), it is really hard to figure out
>> where does tail_call_cnt come from, even though searching tail_call_cnt
>> in the whole kernel repo.
>>
>> By adding these comments, it is a little bit easy to understand
> 
> s/easy/easier

Got it.

> 
>> tail_call_cnt initialisation.
>>
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
> 
> I was rather thinking about dropping these comments altogether. We should
> be trying to provide fixes as small as possible IMHO.
> 
> Having this as a separate commit also breaks logistics of this set as
> the fix + selftest should be routed towards bpf tree, whereas this
> particular commit is rather a bpf-next candidate.
> 
> PTAL at https://www.kernel.org/doc/html/latest/bpf/bpf_devel_QA.html

Thanks for your review.

I'll keep this commit as one of this set.

Thanks,
Leon

> 
>> ---
>>  arch/x86/net/bpf_jit_comp.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index a5930042139d3..bcca1c9b9a027 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
>>  	prog += X86_PATCH_SIZE;
>>  	if (!ebpf_from_cbpf) {
>>  		if (tail_call_reachable && !is_subprog)
>> +			/* When it's the entry of the whole tailcall context,
>> +			 * zeroing rax means initialising tail_call_cnt.
>> +			 */
>>  			EMIT2(0x31, 0xC0); /* xor eax, eax */
>>  		else
>> +			/* Keep the same instruction layout. */
>>  			EMIT2(0x66, 0x90); /* nop2 */
>>  	}
>>  	EMIT1(0x55);             /* push rbp */
>> -- 
>> 2.41.0
>>

[RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop

[Leon Hwang]

From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and tailcall
handling in JIT"), the tailcall on x64 works better than before.

From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF subprograms
for x64 JIT"), tailcall is able to run in BPF subprograms on x64.

From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF program
to other BPF programs"), BPF program is able to trace other BPF programs.

How about combining them all together?

1. FENTRY/FEXIT on a BPF subprogram.
2. A tailcall runs in the BPF subprogram.
3. The tailcall calls the subprogram's caller.

As a result, a tailcall infinite loop comes up. And the loop would halt
the machine.

As we know, in tail call context, the tail_call_cnt propagates by stack
and rax register between BPF subprograms. So do in trampolines.

Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
Fixes: e411901c0b77 ("bpf: allow for tailcalls in BPF subprograms for x64 JIT")
Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 arch/x86/net/bpf_jit_comp.c | 28 ++++++++++++++++++++++------
 include/linux/bpf.h         |  5 +++++
 kernel/bpf/trampoline.c     |  4 ++--
 kernel/bpf/verifier.c       |  3 +++
 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index bcca1c9b9a027..2846c21d75bfa 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1022,6 +1022,10 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
 
 #define INSN_SZ_DIFF (((addrs[i] - addrs[i - 1]) - (prog - temp)))
 
+/* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
+#define RESTORE_TAIL_CALL_CNT(stack)				\
+	EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
+
 static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
 		  int oldproglen, struct jit_context *ctx, bool jmp_padding)
 {
@@ -1627,9 +1631,7 @@ st:			if (is_imm8(insn->off))
 
 			func = (u8 *) __bpf_call_base + imm32;
 			if (tail_call_reachable) {
-				/* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
-				EMIT3_off32(0x48, 0x8B, 0x85,
-					    -round_up(bpf_prog->aux->stack_depth, 8) - 8);
+				RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
 				if (!imm32)
 					return -EINVAL;
 				offs = 7 + x86_call_depth_emit_accounting(&prog, func);
@@ -2404,6 +2406,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	 *                     [ ...        ]
 	 *                     [ stack_arg2 ]
 	 * RBP - arg_stack_off [ stack_arg1 ]
+	 * RSP                 [ tail_call_cnt ] BPF_TRAMP_F_TAIL_CALL_CTX
 	 */
 
 	/* room for return value of orig_call or fentry prog */
@@ -2468,6 +2471,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	else
 		/* sub rsp, stack_size */
 		EMIT4(0x48, 0x83, 0xEC, stack_size);
+	if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
+		EMIT1(0x50);		/* push rax */
 	/* mov QWORD PTR [rbp - rbx_off], rbx */
 	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_6, -rbx_off);
 
@@ -2520,9 +2525,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		restore_regs(m, &prog, regs_off);
 		save_args(m, &prog, arg_stack_off, true);
 
+		if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
+			/* Before calling the original function, restore the
+			 * tail_call_cnt from stack to rax.
+			 */
+			RESTORE_TAIL_CALL_CNT(stack_size);
+
 		if (flags & BPF_TRAMP_F_ORIG_STACK) {
-			emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
-			EMIT2(0xff, 0xd0); /* call *rax */
+			emit_ldx(&prog, BPF_DW, BPF_REG_6, BPF_REG_FP, 8);
+			EMIT2(0xff, 0xd3); /* call *rbx */
 		} else {
 			/* call original function */
 			if (emit_rsb_call(&prog, orig_call, prog)) {
@@ -2573,7 +2584,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 			ret = -EINVAL;
 			goto cleanup;
 		}
-	}
+	} else if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
+		/* Before running the original function, restore the
+		 * tail_call_cnt from stack to rax.
+		 */
+		RESTORE_TAIL_CALL_CNT(stack_size);
+
 	/* restore return value of orig_call or fentry prog back into RAX */
 	if (save_ret)
 		emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cfabbcf47bdb8..c8df257ea435d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1028,6 +1028,11 @@ struct btf_func_model {
  */
 #define BPF_TRAMP_F_SHARE_IPMODIFY	BIT(6)
 
+/* Indicate that current trampoline is in a tail call context. Then, it has to
+ * cache and restore tail_call_cnt to avoid infinite tail call loop.
+ */
+#define BPF_TRAMP_F_TAIL_CALL_CTX	BIT(7)
+
 /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
  * bytes on x86.
  */
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 78acf28d48732..16ab5da7161f2 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -415,8 +415,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 		goto out;
 	}
 
-	/* clear all bits except SHARE_IPMODIFY */
-	tr->flags &= BPF_TRAMP_F_SHARE_IPMODIFY;
+	/* clear all bits except SHARE_IPMODIFY and TAIL_CALL_CTX */
+	tr->flags &= (BPF_TRAMP_F_SHARE_IPMODIFY | BPF_TRAMP_F_TAIL_CALL_CTX);
 
 	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
 	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4ccca1f6c9981..765da3007106a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19629,6 +19629,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	if (!tr)
 		return -ENOMEM;
 
+	if (tgt_prog && tgt_prog->aux->tail_call_reachable)
+		tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX;
+
 	prog->aux->dst_trampoline = tr;
 	return 0;
 }
-- 
2.41.0

Re: [RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop

[Maciej Fijalkowski]

On Sun, Sep 03, 2023 at 11:14:46PM +0800, Leon Hwang wrote:
> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and tailcall
> handling in JIT"), the tailcall on x64 works better than before.
> 
> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF subprograms
> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
> 
> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF program
> to other BPF programs"), BPF program is able to trace other BPF programs.
> 
> How about combining them all together?
> 
> 1. FENTRY/FEXIT on a BPF subprogram.
> 2. A tailcall runs in the BPF subprogram.
> 3. The tailcall calls the subprogram's caller.
> 
> As a result, a tailcall infinite loop comes up. And the loop would halt
> the machine.
> 
> As we know, in tail call context, the tail_call_cnt propagates by stack
> and rax register between BPF subprograms. So do in trampolines.
> 
> Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
> Fixes: e411901c0b77 ("bpf: allow for tailcalls in BPF subprograms for x64 JIT")
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  arch/x86/net/bpf_jit_comp.c | 28 ++++++++++++++++++++++------
>  include/linux/bpf.h         |  5 +++++
>  kernel/bpf/trampoline.c     |  4 ++--
>  kernel/bpf/verifier.c       |  3 +++
>  4 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index bcca1c9b9a027..2846c21d75bfa 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1022,6 +1022,10 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
>  
>  #define INSN_SZ_DIFF (((addrs[i] - addrs[i - 1]) - (prog - temp)))
>  
> +/* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
> +#define RESTORE_TAIL_CALL_CNT(stack)				\
> +	EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
> +
>  static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
>  		  int oldproglen, struct jit_context *ctx, bool jmp_padding)
>  {
> @@ -1627,9 +1631,7 @@ st:			if (is_imm8(insn->off))
>  
>  			func = (u8 *) __bpf_call_base + imm32;
>  			if (tail_call_reachable) {
> -				/* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
> -				EMIT3_off32(0x48, 0x8B, 0x85,
> -					    -round_up(bpf_prog->aux->stack_depth, 8) - 8);
> +				RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
>  				if (!imm32)
>  					return -EINVAL;
>  				offs = 7 + x86_call_depth_emit_accounting(&prog, func);
> @@ -2404,6 +2406,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>  	 *                     [ ...        ]
>  	 *                     [ stack_arg2 ]
>  	 * RBP - arg_stack_off [ stack_arg1 ]
> +	 * RSP                 [ tail_call_cnt ] BPF_TRAMP_F_TAIL_CALL_CTX
>  	 */
>  
>  	/* room for return value of orig_call or fentry prog */
> @@ -2468,6 +2471,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>  	else
>  		/* sub rsp, stack_size */
>  		EMIT4(0x48, 0x83, 0xEC, stack_size);
> +	if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
> +		EMIT1(0x50);		/* push rax */
>  	/* mov QWORD PTR [rbp - rbx_off], rbx */
>  	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_6, -rbx_off);
>  
> @@ -2520,9 +2525,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>  		restore_regs(m, &prog, regs_off);
>  		save_args(m, &prog, arg_stack_off, true);
>  
> +		if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
> +			/* Before calling the original function, restore the
> +			 * tail_call_cnt from stack to rax.
> +			 */
> +			RESTORE_TAIL_CALL_CNT(stack_size);
> +
>  		if (flags & BPF_TRAMP_F_ORIG_STACK) {
> -			emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
> -			EMIT2(0xff, 0xd0); /* call *rax */
> +			emit_ldx(&prog, BPF_DW, BPF_REG_6, BPF_REG_FP, 8);
> +			EMIT2(0xff, 0xd3); /* call *rbx */
>  		} else {
>  			/* call original function */
>  			if (emit_rsb_call(&prog, orig_call, prog)) {
> @@ -2573,7 +2584,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>  			ret = -EINVAL;
>  			goto cleanup;
>  		}
> -	}
> +	} else if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
> +		/* Before running the original function, restore the
> +		 * tail_call_cnt from stack to rax.
> +		 */
> +		RESTORE_TAIL_CALL_CNT(stack_size);
> +
>  	/* restore return value of orig_call or fentry prog back into RAX */
>  	if (save_ret)
>  		emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cfabbcf47bdb8..c8df257ea435d 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1028,6 +1028,11 @@ struct btf_func_model {
>   */
>  #define BPF_TRAMP_F_SHARE_IPMODIFY	BIT(6)
>  
> +/* Indicate that current trampoline is in a tail call context. Then, it has to
> + * cache and restore tail_call_cnt to avoid infinite tail call loop.
> + */
> +#define BPF_TRAMP_F_TAIL_CALL_CTX	BIT(7)
> +
>  /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
>   * bytes on x86.
>   */
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 78acf28d48732..16ab5da7161f2 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -415,8 +415,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>  		goto out;
>  	}
>  
> -	/* clear all bits except SHARE_IPMODIFY */
> -	tr->flags &= BPF_TRAMP_F_SHARE_IPMODIFY;
> +	/* clear all bits except SHARE_IPMODIFY and TAIL_CALL_CTX */
> +	tr->flags &= (BPF_TRAMP_F_SHARE_IPMODIFY | BPF_TRAMP_F_TAIL_CALL_CTX);
>  
>  	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
>  	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links) {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4ccca1f6c9981..765da3007106a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19629,6 +19629,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>  	if (!tr)
>  		return -ENOMEM;
>  
> +	if (tgt_prog && tgt_prog->aux->tail_call_reachable)
> +		tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX;
> +
>  	prog->aux->dst_trampoline = tr;
>  	return 0;
>  }
> -- 
> 2.41.0
> 

[RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls

[Leon Hwang]

Get and check data_fd. It should not to check map_fd again.

Fixes: 79d49ba048ec ("bpf, testing: Add various tail call test cases")
Fixes: 3b0379111197 ("selftests/bpf: Add tailcall_bpf2bpf tests")
Fixes: 5e0b0a4c52d3 ("selftests/bpf: Test tail call counting with bpf2bpf and data on stack")
Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/tailcalls.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index 58fe2c586ed76..b20d7f77a5bce 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -274,7 +274,7 @@ static void test_tailcall_count(const char *which)
 		return;
 
 	data_fd = bpf_map__fd(data_map);
-	if (CHECK_FAIL(map_fd < 0))
+	if (CHECK_FAIL(data_fd < 0))
 		return;
 
 	i = 0;
@@ -355,7 +355,7 @@ static void test_tailcall_4(void)
 		return;
 
 	data_fd = bpf_map__fd(data_map);
-	if (CHECK_FAIL(map_fd < 0))
+	if (CHECK_FAIL(data_fd < 0))
 		return;
 
 	for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
@@ -445,7 +445,7 @@ static void test_tailcall_5(void)
 		return;
 
 	data_fd = bpf_map__fd(data_map);
-	if (CHECK_FAIL(map_fd < 0))
+	if (CHECK_FAIL(data_fd < 0))
 		return;
 
 	for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
@@ -634,7 +634,7 @@ static void test_tailcall_bpf2bpf_2(void)
 		return;
 
 	data_fd = bpf_map__fd(data_map);
-	if (CHECK_FAIL(map_fd < 0))
+	if (CHECK_FAIL(data_fd < 0))
 		return;
 
 	i = 0;
@@ -808,7 +808,7 @@ static void test_tailcall_bpf2bpf_4(bool noise)
 		return;
 
 	data_fd = bpf_map__fd(data_map);
-	if (CHECK_FAIL(map_fd < 0))
+	if (CHECK_FAIL(data_fd < 0))
 		return;
 
 	i = 0;
@@ -872,7 +872,7 @@ static void test_tailcall_bpf2bpf_6(void)
 	ASSERT_EQ(topts.retval, 0, "tailcall retval");
 
 	data_fd = bpf_map__fd(obj->maps.bss);
-	if (!ASSERT_GE(map_fd, 0, "bss map fd"))
+	if (!ASSERT_GE(data_fd, 0, "bss map fd"))
 		goto out;
 
 	i = 0;
-- 
2.41.0

Re: [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls

[Maciej Fijalkowski]

On Sun, Sep 03, 2023 at 11:14:47PM +0800, Leon Hwang wrote:
> Get and check data_fd. It should not to check map_fd again.
> 
> Fixes: 79d49ba048ec ("bpf, testing: Add various tail call test cases")
> Fixes: 3b0379111197 ("selftests/bpf: Add tailcall_bpf2bpf tests")
> Fixes: 5e0b0a4c52d3 ("selftests/bpf: Test tail call counting with bpf2bpf and data on stack")
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>

This could be pulled out of this RFC set and sent separately to bpf tree,
given that Ilya is taking a look at addressing s390 jit.

> ---
>  tools/testing/selftests/bpf/prog_tests/tailcalls.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index 58fe2c586ed76..b20d7f77a5bce 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -274,7 +274,7 @@ static void test_tailcall_count(const char *which)
>  		return;
>  
>  	data_fd = bpf_map__fd(data_map);
> -	if (CHECK_FAIL(map_fd < 0))
> +	if (CHECK_FAIL(data_fd < 0))
>  		return;
>  
>  	i = 0;
> @@ -355,7 +355,7 @@ static void test_tailcall_4(void)
>  		return;
>  
>  	data_fd = bpf_map__fd(data_map);
> -	if (CHECK_FAIL(map_fd < 0))
> +	if (CHECK_FAIL(data_fd < 0))
>  		return;
>  
>  	for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
> @@ -445,7 +445,7 @@ static void test_tailcall_5(void)
>  		return;
>  
>  	data_fd = bpf_map__fd(data_map);
> -	if (CHECK_FAIL(map_fd < 0))
> +	if (CHECK_FAIL(data_fd < 0))
>  		return;

shouldn't this be 'goto out' ? applies to the rest of the code i believe.

>  
>  	for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
> @@ -634,7 +634,7 @@ static void test_tailcall_bpf2bpf_2(void)
>  		return;
>  
>  	data_fd = bpf_map__fd(data_map);
> -	if (CHECK_FAIL(map_fd < 0))
> +	if (CHECK_FAIL(data_fd < 0))
>  		return;
>  
>  	i = 0;
> @@ -808,7 +808,7 @@ static void test_tailcall_bpf2bpf_4(bool noise)
>  		return;
>  
>  	data_fd = bpf_map__fd(data_map);
> -	if (CHECK_FAIL(map_fd < 0))
> +	if (CHECK_FAIL(data_fd < 0))
>  		return;
>  
>  	i = 0;
> @@ -872,7 +872,7 @@ static void test_tailcall_bpf2bpf_6(void)
>  	ASSERT_EQ(topts.retval, 0, "tailcall retval");
>  
>  	data_fd = bpf_map__fd(obj->maps.bss);
> -	if (!ASSERT_GE(map_fd, 0, "bss map fd"))
> +	if (!ASSERT_GE(data_fd, 0, "bss map fd"))
>  		goto out;
>  
>  	i = 0;
> -- 
> 2.41.0
> 

Re: [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls

[Leon Hwang]

On 6/9/23 03:22, Maciej Fijalkowski wrote:
> On Sun, Sep 03, 2023 at 11:14:47PM +0800, Leon Hwang wrote:
>> Get and check data_fd. It should not to check map_fd again.
>>
>> Fixes: 79d49ba048ec ("bpf, testing: Add various tail call test cases")
>> Fixes: 3b0379111197 ("selftests/bpf: Add tailcall_bpf2bpf tests")
>> Fixes: 5e0b0a4c52d3 ("selftests/bpf: Test tail call counting with bpf2bpf and data on stack")
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
> 
> This could be pulled out of this RFC set and sent separately to bpf tree,
> given that Ilya is taking a look at addressing s390 jit.

Yeah, I'll do it.

> 
>> ---
>>  tools/testing/selftests/bpf/prog_tests/tailcalls.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> index 58fe2c586ed76..b20d7f77a5bce 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> @@ -274,7 +274,7 @@ static void test_tailcall_count(const char *which)
>>  		return;
>>  
>>  	data_fd = bpf_map__fd(data_map);
>> -	if (CHECK_FAIL(map_fd < 0))
>> +	if (CHECK_FAIL(data_fd < 0))
>>  		return;
>>  
>>  	i = 0;
>> @@ -355,7 +355,7 @@ static void test_tailcall_4(void)
>>  		return;
>>  
>>  	data_fd = bpf_map__fd(data_map);
>> -	if (CHECK_FAIL(map_fd < 0))
>> +	if (CHECK_FAIL(data_fd < 0))
>>  		return;
>>  
>>  	for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
>> @@ -445,7 +445,7 @@ static void test_tailcall_5(void)
>>  		return;
>>  
>>  	data_fd = bpf_map__fd(data_map);
>> -	if (CHECK_FAIL(map_fd < 0))
>> +	if (CHECK_FAIL(data_fd < 0))
>>  		return;
> 
> shouldn't this be 'goto out' ? applies to the rest of the code i believe.

Good point. I'll correct some other 'return' to 'goto out' meanwhile.

Thanks,
Leon

> 
>>  
>>  	for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
>> @@ -634,7 +634,7 @@ static void test_tailcall_bpf2bpf_2(void)
>>  		return;
>>  
>>  	data_fd = bpf_map__fd(data_map);
>> -	if (CHECK_FAIL(map_fd < 0))
>> +	if (CHECK_FAIL(data_fd < 0))
>>  		return;
>>  
>>  	i = 0;
>> @@ -808,7 +808,7 @@ static void test_tailcall_bpf2bpf_4(bool noise)
>>  		return;
>>  
>>  	data_fd = bpf_map__fd(data_map);
>> -	if (CHECK_FAIL(map_fd < 0))
>> +	if (CHECK_FAIL(data_fd < 0))
>>  		return;
>>  
>>  	i = 0;
>> @@ -872,7 +872,7 @@ static void test_tailcall_bpf2bpf_6(void)
>>  	ASSERT_EQ(topts.retval, 0, "tailcall retval");
>>  
>>  	data_fd = bpf_map__fd(obj->maps.bss);
>> -	if (!ASSERT_GE(map_fd, 0, "bss map fd"))
>> +	if (!ASSERT_GE(data_fd, 0, "bss map fd"))
>>  		goto out;
>>  
>>  	i = 0;
>> -- 
>> 2.41.0
>>

[RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing

[Leon Hwang]

Add 4 test cases to confirm the tailcall infinite loop bug has been fixed.

Like tailcall_bpf2bpf cases, do fentry/fexit on the bpf2bpf, and then
check the final count result.

tools/testing/selftests/bpf/test_progs -t tailcalls
226/13  tailcalls/tailcall_bpf2bpf_fentry:OK
226/14  tailcalls/tailcall_bpf2bpf_fexit:OK
226/15  tailcalls/tailcall_bpf2bpf_fentry_fexit:OK
226/16  tailcalls/tailcall_bpf2bpf_fentry_entry:OK
226     tailcalls:OK
Summary: 1/16 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 .../selftests/bpf/prog_tests/tailcalls.c      | 299 ++++++++++++++++++
 .../bpf/progs/tailcall_bpf2bpf_fentry.c       |  18 ++
 .../bpf/progs/tailcall_bpf2bpf_fexit.c        |  18 ++
 3 files changed, 335 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c

diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index b20d7f77a5bce..331b4e455ad06 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -884,6 +884,297 @@ static void test_tailcall_bpf2bpf_6(void)
 	tailcall_bpf2bpf6__destroy(obj);
 }
 
+static void tailcall_bpf2bpf_fentry_fexit(bool test_fentry, bool test_fexit)
+{
+	struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL, *fexit_obj = NULL;
+	struct bpf_link *fentry_link = NULL, *fexit_link = NULL;
+	int err, map_fd, prog_fd, main_fd, data_fd, i, val;
+	struct bpf_map *prog_array, *data_map;
+	struct bpf_program *prog;
+	char buff[128] = {};
+
+	LIBBPF_OPTS(bpf_test_run_opts, topts,
+		.data_in = buff,
+		.data_size_in = sizeof(buff),
+		.repeat = 1,
+	);
+
+	err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
+				 BPF_PROG_TYPE_SCHED_CLS,
+				 &tgt_obj, &prog_fd);
+	if (!ASSERT_OK(err, "load tgt_obj"))
+		return;
+
+	prog = bpf_object__find_program_by_name(tgt_obj, "entry");
+	if (!ASSERT_OK_PTR(prog, "find entry prog"))
+		goto out;
+
+	main_fd = bpf_program__fd(prog);
+	if (!ASSERT_FALSE(main_fd < 0, "find entry prog fd"))
+		goto out;
+
+	prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
+	if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
+		goto out;
+
+	map_fd = bpf_map__fd(prog_array);
+	if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
+		goto out;
+
+	prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
+	if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
+		goto out;
+
+	prog_fd = bpf_program__fd(prog);
+	if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
+		goto out;
+
+	i = 0;
+	err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
+	if (!ASSERT_OK(err, "update jmp_table"))
+		goto out;
+
+	if (test_fentry) {
+		fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
+						   NULL);
+		if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
+			goto out;
+
+		prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
+		if (!ASSERT_OK_PTR(prog, "find fentry prog"))
+			goto out;
+
+		err = bpf_program__set_attach_target(prog, prog_fd,
+						     "subprog_tail");
+		if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
+			goto out;
+
+		err = bpf_object__load(fentry_obj);
+		if (!ASSERT_OK(err, "load fentry_obj"))
+			goto out;
+
+		fentry_link = bpf_program__attach_trace(prog);
+		if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
+			goto out;
+	}
+
+	if (test_fexit) {
+		fexit_obj = bpf_object__open_file("tailcall_bpf2bpf_fexit.bpf.o",
+						  NULL);
+		if (!ASSERT_OK_PTR(fexit_obj, "open fexit_obj file"))
+			goto out;
+
+		prog = bpf_object__find_program_by_name(fexit_obj, "fexit");
+		if (!ASSERT_OK_PTR(prog, "find fexit prog"))
+			goto out;
+
+		err = bpf_program__set_attach_target(prog, prog_fd,
+						     "subprog_tail");
+		if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
+			goto out;
+
+		err = bpf_object__load(fexit_obj);
+		if (!ASSERT_OK(err, "load fexit_obj"))
+			goto out;
+
+		fexit_link = bpf_program__attach_trace(prog);
+		if (!ASSERT_OK_PTR(fexit_link, "attach_trace"))
+			goto out;
+	}
+
+	err = bpf_prog_test_run_opts(main_fd, &topts);
+	ASSERT_OK(err, "tailcall");
+	ASSERT_EQ(topts.retval, 1, "tailcall retval");
+
+	data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
+	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+			  "find tailcall.bss map"))
+		goto out;
+
+	data_fd = bpf_map__fd(data_map);
+	if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
+		goto out;
+
+	i = 0;
+	err = bpf_map_lookup_elem(data_fd, &i, &val);
+	ASSERT_OK(err, "tailcall count");
+	ASSERT_EQ(val, 33, "tailcall count");
+
+	if (test_fentry) {
+		data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
+		if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+				  "find tailcall_bpf2bpf_fentry.bss map"))
+			goto out;
+
+		data_fd = bpf_map__fd(data_map);
+		if (!ASSERT_FALSE(data_fd < 0,
+				  "find tailcall_bpf2bpf_fentry.bss map fd"))
+			goto out;
+
+		i = 0;
+		err = bpf_map_lookup_elem(data_fd, &i, &val);
+		ASSERT_OK(err, "fentry count");
+		ASSERT_EQ(val, 33, "fentry count");
+	}
+
+	if (test_fexit) {
+		data_map = bpf_object__find_map_by_name(fexit_obj, ".bss");
+		if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+				  "find tailcall_bpf2bpf_fexit.bss map"))
+			goto out;
+
+		data_fd = bpf_map__fd(data_map);
+		if (!ASSERT_FALSE(data_fd < 0,
+				  "find tailcall_bpf2bpf_fexit.bss map fd"))
+			goto out;
+
+		i = 0;
+		err = bpf_map_lookup_elem(data_fd, &i, &val);
+		ASSERT_OK(err, "fexit count");
+		ASSERT_EQ(val, 33, "fexit count");
+	}
+
+out:
+	bpf_link__destroy(fentry_link);
+	bpf_link__destroy(fexit_link);
+	bpf_object__close(fentry_obj);
+	bpf_object__close(fexit_obj);
+	bpf_object__close(tgt_obj);
+}
+
+/* test_tailcall_bpf2bpf_fentry checks that the count value of the tail call
+ * limit enforcement matches with expectations when tailcall is preceded with
+ * bpf2bpf call, and the bpf2bpf call is traced by fentry.
+ */
+static void test_tailcall_bpf2bpf_fentry(void)
+{
+	tailcall_bpf2bpf_fentry_fexit(true, false);
+}
+
+/* test_tailcall_bpf2bpf_fexit checks that the count value of the tail call
+ * limit enforcement matches with expectations when tailcall is preceded with
+ * bpf2bpf call, and the bpf2bpf call is traced by fexit.
+ */
+static void test_tailcall_bpf2bpf_fexit(void)
+{
+	tailcall_bpf2bpf_fentry_fexit(false, true);
+}
+
+/* test_tailcall_bpf2bpf_fentry_fexit checks that the count value of the tail
+ * call limit enforcement matches with expectations when tailcall is preceded
+ * with bpf2bpf call, and the bpf2bpf call is traced by both fentry and fexit.
+ */
+static void test_tailcall_bpf2bpf_fentry_fexit(void)
+{
+	tailcall_bpf2bpf_fentry_fexit(true, true);
+}
+
+/* test_tailcall_bpf2bpf_fentry_entry checks that the count value of the tail
+ * call limit enforcement matches with expectations when tailcall is preceded
+ * with bpf2bpf call, and the bpf2bpf caller is traced by fentry.
+ */
+static void test_tailcall_bpf2bpf_fentry_entry(void)
+{
+	struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL;
+	int err, map_fd, prog_fd, data_fd, i, val;
+	struct bpf_map *prog_array, *data_map;
+	struct bpf_link *fentry_link = NULL;
+	struct bpf_program *prog;
+	char buff[128] = {};
+
+	LIBBPF_OPTS(bpf_test_run_opts, topts,
+		.data_in = buff,
+		.data_size_in = sizeof(buff),
+		.repeat = 1,
+	);
+
+	err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
+				 BPF_PROG_TYPE_SCHED_CLS,
+				 &tgt_obj, &prog_fd);
+	if (!ASSERT_OK(err, "load tgt_obj"))
+		return;
+
+	prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
+	if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
+		goto out;
+
+	map_fd = bpf_map__fd(prog_array);
+	if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
+		goto out;
+
+	prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
+	if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
+		goto out;
+
+	prog_fd = bpf_program__fd(prog);
+	if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
+		goto out;
+
+	i = 0;
+	err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
+	if (!ASSERT_OK(err, "update jmp_table"))
+		goto out;
+
+	fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
+					   NULL);
+	if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
+		goto out;
+
+	prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
+	if (!ASSERT_OK_PTR(prog, "find fentry prog"))
+		goto out;
+
+	err = bpf_program__set_attach_target(prog, prog_fd, "classifier_0");
+	if (!ASSERT_OK(err, "set_attach_target classifier_0"))
+		goto out;
+
+	err = bpf_object__load(fentry_obj);
+	if (!ASSERT_OK(err, "load fentry_obj"))
+		goto out;
+
+	fentry_link = bpf_program__attach_trace(prog);
+	if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
+		goto out;
+
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "tailcall");
+	ASSERT_EQ(topts.retval, 1, "tailcall retval");
+
+	data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
+	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+			  "find tailcall.bss map"))
+		goto out;
+
+	data_fd = bpf_map__fd(data_map);
+	if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
+		goto out;
+
+	i = 0;
+	err = bpf_map_lookup_elem(data_fd, &i, &val);
+	ASSERT_OK(err, "tailcall count");
+	ASSERT_EQ(val, 34, "tailcall count");
+
+	data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
+	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+			  "find tailcall_bpf2bpf_fentry.bss map"))
+		goto out;
+
+	data_fd = bpf_map__fd(data_map);
+	if (!ASSERT_FALSE(data_fd < 0,
+			  "find tailcall_bpf2bpf_fentry.bss map fd"))
+		goto out;
+
+	i = 0;
+	err = bpf_map_lookup_elem(data_fd, &i, &val);
+	ASSERT_OK(err, "fentry count");
+	ASSERT_EQ(val, 1, "fentry count");
+
+out:
+	bpf_link__destroy(fentry_link);
+	bpf_object__close(fentry_obj);
+	bpf_object__close(tgt_obj);
+}
+
 void test_tailcalls(void)
 {
 	if (test__start_subtest("tailcall_1"))
@@ -910,4 +1201,12 @@ void test_tailcalls(void)
 		test_tailcall_bpf2bpf_4(true);
 	if (test__start_subtest("tailcall_bpf2bpf_6"))
 		test_tailcall_bpf2bpf_6();
+	if (test__start_subtest("tailcall_bpf2bpf_fentry"))
+		test_tailcall_bpf2bpf_fentry();
+	if (test__start_subtest("tailcall_bpf2bpf_fexit"))
+		test_tailcall_bpf2bpf_fexit();
+	if (test__start_subtest("tailcall_bpf2bpf_fentry_fexit"))
+		test_tailcall_bpf2bpf_fentry_fexit();
+	if (test__start_subtest("tailcall_bpf2bpf_fentry_entry"))
+		test_tailcall_bpf2bpf_fentry_entry();
 }
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
new file mode 100644
index 0000000000000..8436c6729167c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Leon Hwang */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+int count = 0;
+
+SEC("fentry/subprog_tail")
+int BPF_PROG(fentry, struct sk_buff *skb)
+{
+	count++;
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
new file mode 100644
index 0000000000000..fe16412c6e6e9
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Leon Hwang */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+int count = 0;
+
+SEC("fexit/subprog_tail")
+int BPF_PROG(fexit, struct sk_buff *skb)
+{
+	count++;
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.41.0

Re: [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing

[Maciej Fijalkowski]

On Sun, Sep 03, 2023 at 11:14:48PM +0800, Leon Hwang wrote:
> Add 4 test cases to confirm the tailcall infinite loop bug has been fixed.
> 
> Like tailcall_bpf2bpf cases, do fentry/fexit on the bpf2bpf, and then
> check the final count result.
> 
> tools/testing/selftests/bpf/test_progs -t tailcalls
> 226/13  tailcalls/tailcall_bpf2bpf_fentry:OK
> 226/14  tailcalls/tailcall_bpf2bpf_fexit:OK
> 226/15  tailcalls/tailcall_bpf2bpf_fentry_fexit:OK
> 226/16  tailcalls/tailcall_bpf2bpf_fentry_entry:OK
> 226     tailcalls:OK
> Summary: 1/16 PASSED, 0 SKIPPED, 0 FAILED
> 
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/tailcalls.c      | 299 ++++++++++++++++++
>  .../bpf/progs/tailcall_bpf2bpf_fentry.c       |  18 ++
>  .../bpf/progs/tailcall_bpf2bpf_fexit.c        |  18 ++
>  3 files changed, 335 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
>  create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index b20d7f77a5bce..331b4e455ad06 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -884,6 +884,297 @@ static void test_tailcall_bpf2bpf_6(void)
>  	tailcall_bpf2bpf6__destroy(obj);
>  }
>  
> +static void tailcall_bpf2bpf_fentry_fexit(bool test_fentry, bool test_fexit)
> +{
> +	struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL, *fexit_obj = NULL;
> +	struct bpf_link *fentry_link = NULL, *fexit_link = NULL;
> +	int err, map_fd, prog_fd, main_fd, data_fd, i, val;
> +	struct bpf_map *prog_array, *data_map;
> +	struct bpf_program *prog;
> +	char buff[128] = {};
> +
> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
> +		.data_in = buff,
> +		.data_size_in = sizeof(buff),
> +		.repeat = 1,
> +	);
> +
> +	err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
> +				 BPF_PROG_TYPE_SCHED_CLS,
> +				 &tgt_obj, &prog_fd);
> +	if (!ASSERT_OK(err, "load tgt_obj"))
> +		return;
> +
> +	prog = bpf_object__find_program_by_name(tgt_obj, "entry");
> +	if (!ASSERT_OK_PTR(prog, "find entry prog"))
> +		goto out;
> +
> +	main_fd = bpf_program__fd(prog);
> +	if (!ASSERT_FALSE(main_fd < 0, "find entry prog fd"))
> +		goto out;
> +
> +	prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
> +	if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
> +		goto out;
> +
> +	map_fd = bpf_map__fd(prog_array);
> +	if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
> +		goto out;
> +
> +	prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
> +	if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
> +		goto out;
> +
> +	prog_fd = bpf_program__fd(prog);
> +	if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
> +		goto out;
> +
> +	i = 0;
> +	err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
> +	if (!ASSERT_OK(err, "update jmp_table"))
> +		goto out;
> +
> +	if (test_fentry) {
> +		fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
> +						   NULL);
> +		if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
> +			goto out;
> +
> +		prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
> +		if (!ASSERT_OK_PTR(prog, "find fentry prog"))
> +			goto out;
> +
> +		err = bpf_program__set_attach_target(prog, prog_fd,
> +						     "subprog_tail");
> +		if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
> +			goto out;
> +
> +		err = bpf_object__load(fentry_obj);
> +		if (!ASSERT_OK(err, "load fentry_obj"))
> +			goto out;
> +
> +		fentry_link = bpf_program__attach_trace(prog);
> +		if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
> +			goto out;
> +	}
> +
> +	if (test_fexit) {
> +		fexit_obj = bpf_object__open_file("tailcall_bpf2bpf_fexit.bpf.o",
> +						  NULL);
> +		if (!ASSERT_OK_PTR(fexit_obj, "open fexit_obj file"))
> +			goto out;
> +
> +		prog = bpf_object__find_program_by_name(fexit_obj, "fexit");
> +		if (!ASSERT_OK_PTR(prog, "find fexit prog"))
> +			goto out;
> +
> +		err = bpf_program__set_attach_target(prog, prog_fd,
> +						     "subprog_tail");
> +		if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
> +			goto out;
> +
> +		err = bpf_object__load(fexit_obj);
> +		if (!ASSERT_OK(err, "load fexit_obj"))
> +			goto out;
> +
> +		fexit_link = bpf_program__attach_trace(prog);
> +		if (!ASSERT_OK_PTR(fexit_link, "attach_trace"))
> +			goto out;
> +	}
> +
> +	err = bpf_prog_test_run_opts(main_fd, &topts);
> +	ASSERT_OK(err, "tailcall");
> +	ASSERT_EQ(topts.retval, 1, "tailcall retval");
> +
> +	data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
> +	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> +			  "find tailcall.bss map"))
> +		goto out;
> +
> +	data_fd = bpf_map__fd(data_map);
> +	if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
> +		goto out;
> +
> +	i = 0;
> +	err = bpf_map_lookup_elem(data_fd, &i, &val);
> +	ASSERT_OK(err, "tailcall count");
> +	ASSERT_EQ(val, 33, "tailcall count");
> +
> +	if (test_fentry) {
> +		data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
> +		if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> +				  "find tailcall_bpf2bpf_fentry.bss map"))
> +			goto out;
> +
> +		data_fd = bpf_map__fd(data_map);
> +		if (!ASSERT_FALSE(data_fd < 0,
> +				  "find tailcall_bpf2bpf_fentry.bss map fd"))
> +			goto out;
> +
> +		i = 0;
> +		err = bpf_map_lookup_elem(data_fd, &i, &val);
> +		ASSERT_OK(err, "fentry count");
> +		ASSERT_EQ(val, 33, "fentry count");
> +	}
> +
> +	if (test_fexit) {
> +		data_map = bpf_object__find_map_by_name(fexit_obj, ".bss");
> +		if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> +				  "find tailcall_bpf2bpf_fexit.bss map"))
> +			goto out;
> +
> +		data_fd = bpf_map__fd(data_map);
> +		if (!ASSERT_FALSE(data_fd < 0,
> +				  "find tailcall_bpf2bpf_fexit.bss map fd"))
> +			goto out;
> +
> +		i = 0;
> +		err = bpf_map_lookup_elem(data_fd, &i, &val);
> +		ASSERT_OK(err, "fexit count");
> +		ASSERT_EQ(val, 33, "fexit count");
> +	}
> +
> +out:
> +	bpf_link__destroy(fentry_link);
> +	bpf_link__destroy(fexit_link);
> +	bpf_object__close(fentry_obj);
> +	bpf_object__close(fexit_obj);
> +	bpf_object__close(tgt_obj);
> +}
> +
> +/* test_tailcall_bpf2bpf_fentry checks that the count value of the tail call
> + * limit enforcement matches with expectations when tailcall is preceded with
> + * bpf2bpf call, and the bpf2bpf call is traced by fentry.
> + */
> +static void test_tailcall_bpf2bpf_fentry(void)
> +{
> +	tailcall_bpf2bpf_fentry_fexit(true, false);
> +}
> +
> +/* test_tailcall_bpf2bpf_fexit checks that the count value of the tail call
> + * limit enforcement matches with expectations when tailcall is preceded with
> + * bpf2bpf call, and the bpf2bpf call is traced by fexit.
> + */
> +static void test_tailcall_bpf2bpf_fexit(void)
> +{
> +	tailcall_bpf2bpf_fentry_fexit(false, true);
> +}
> +
> +/* test_tailcall_bpf2bpf_fentry_fexit checks that the count value of the tail
> + * call limit enforcement matches with expectations when tailcall is preceded
> + * with bpf2bpf call, and the bpf2bpf call is traced by both fentry and fexit.
> + */
> +static void test_tailcall_bpf2bpf_fentry_fexit(void)
> +{
> +	tailcall_bpf2bpf_fentry_fexit(true, true);

Would it be possible to modify existing test_tailcall_count() to have
fentry/fexit testing within? __tailcall_bpf2bpf_fentry_fexit() basically
repeats the logic of test_tailcall_count(), right?

How about something like:

static void test_tailcall_bpf2bpf_fentry(void)
{
	test_tailcall_count("tailcall_bpf2bpf2.bpf.o", true, false);
}

// rest of your test cases

and existing tailcall count tests:

static void test_tailcall_3(void)
{
	test_tailcall_count("tailcall3.bpf.o", false, false);
}

static void test_tailcall_6(void)
{
	test_tailcall_count("tailcall6.bpf.o", false, false);
}

> +}
> +
> +/* test_tailcall_bpf2bpf_fentry_entry checks that the count value of the tail
> + * call limit enforcement matches with expectations when tailcall is preceded
> + * with bpf2bpf call, and the bpf2bpf caller is traced by fentry.
> + */
> +static void test_tailcall_bpf2bpf_fentry_entry(void)
> +{
> +	struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL;
> +	int err, map_fd, prog_fd, data_fd, i, val;
> +	struct bpf_map *prog_array, *data_map;
> +	struct bpf_link *fentry_link = NULL;
> +	struct bpf_program *prog;
> +	char buff[128] = {};
> +
> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
> +		.data_in = buff,
> +		.data_size_in = sizeof(buff),
> +		.repeat = 1,
> +	);
> +
> +	err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
> +				 BPF_PROG_TYPE_SCHED_CLS,
> +				 &tgt_obj, &prog_fd);
> +	if (!ASSERT_OK(err, "load tgt_obj"))
> +		return;
> +
> +	prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
> +	if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
> +		goto out;
> +
> +	map_fd = bpf_map__fd(prog_array);
> +	if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
> +		goto out;
> +
> +	prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
> +	if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
> +		goto out;
> +
> +	prog_fd = bpf_program__fd(prog);
> +	if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
> +		goto out;
> +
> +	i = 0;
> +	err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
> +	if (!ASSERT_OK(err, "update jmp_table"))
> +		goto out;
> +
> +	fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
> +					   NULL);
> +	if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
> +		goto out;
> +
> +	prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
> +	if (!ASSERT_OK_PTR(prog, "find fentry prog"))
> +		goto out;
> +
> +	err = bpf_program__set_attach_target(prog, prog_fd, "classifier_0");
> +	if (!ASSERT_OK(err, "set_attach_target classifier_0"))
> +		goto out;
> +
> +	err = bpf_object__load(fentry_obj);
> +	if (!ASSERT_OK(err, "load fentry_obj"))
> +		goto out;
> +
> +	fentry_link = bpf_program__attach_trace(prog);
> +	if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
> +		goto out;
> +
> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
> +	ASSERT_OK(err, "tailcall");
> +	ASSERT_EQ(topts.retval, 1, "tailcall retval");
> +
> +	data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
> +	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> +			  "find tailcall.bss map"))
> +		goto out;
> +
> +	data_fd = bpf_map__fd(data_map);
> +	if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
> +		goto out;
> +
> +	i = 0;
> +	err = bpf_map_lookup_elem(data_fd, &i, &val);
> +	ASSERT_OK(err, "tailcall count");
> +	ASSERT_EQ(val, 34, "tailcall count");
> +
> +	data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
> +	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> +			  "find tailcall_bpf2bpf_fentry.bss map"))
> +		goto out;
> +
> +	data_fd = bpf_map__fd(data_map);
> +	if (!ASSERT_FALSE(data_fd < 0,
> +			  "find tailcall_bpf2bpf_fentry.bss map fd"))
> +		goto out;
> +
> +	i = 0;
> +	err = bpf_map_lookup_elem(data_fd, &i, &val);
> +	ASSERT_OK(err, "fentry count");
> +	ASSERT_EQ(val, 1, "fentry count");
> +
> +out:
> +	bpf_link__destroy(fentry_link);
> +	bpf_object__close(fentry_obj);
> +	bpf_object__close(tgt_obj);
> +}
> +
>  void test_tailcalls(void)
>  {
>  	if (test__start_subtest("tailcall_1"))
> @@ -910,4 +1201,12 @@ void test_tailcalls(void)
>  		test_tailcall_bpf2bpf_4(true);
>  	if (test__start_subtest("tailcall_bpf2bpf_6"))
>  		test_tailcall_bpf2bpf_6();
> +	if (test__start_subtest("tailcall_bpf2bpf_fentry"))
> +		test_tailcall_bpf2bpf_fentry();
> +	if (test__start_subtest("tailcall_bpf2bpf_fexit"))
> +		test_tailcall_bpf2bpf_fexit();
> +	if (test__start_subtest("tailcall_bpf2bpf_fentry_fexit"))
> +		test_tailcall_bpf2bpf_fentry_fexit();
> +	if (test__start_subtest("tailcall_bpf2bpf_fentry_entry"))
> +		test_tailcall_bpf2bpf_fentry_entry();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
> new file mode 100644
> index 0000000000000..8436c6729167c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright Leon Hwang */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +int count = 0;
> +
> +SEC("fentry/subprog_tail")
> +int BPF_PROG(fentry, struct sk_buff *skb)
> +{
> +	count++;
> +
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
> new file mode 100644
> index 0000000000000..fe16412c6e6e9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright Leon Hwang */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +int count = 0;
> +
> +SEC("fexit/subprog_tail")
> +int BPF_PROG(fexit, struct sk_buff *skb)
> +{
> +	count++;
> +
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> -- 
> 2.41.0
> 

Re: [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing

[Leon Hwang]

On 7/9/23 05:18, Maciej Fijalkowski wrote:
> On Sun, Sep 03, 2023 at 11:14:48PM +0800, Leon Hwang wrote:
>> Add 4 test cases to confirm the tailcall infinite loop bug has been fixed.
>>
>> Like tailcall_bpf2bpf cases, do fentry/fexit on the bpf2bpf, and then
>> check the final count result.
>>
>> tools/testing/selftests/bpf/test_progs -t tailcalls
>> 226/13  tailcalls/tailcall_bpf2bpf_fentry:OK
>> 226/14  tailcalls/tailcall_bpf2bpf_fexit:OK
>> 226/15  tailcalls/tailcall_bpf2bpf_fentry_fexit:OK
>> 226/16  tailcalls/tailcall_bpf2bpf_fentry_entry:OK
>> 226     tailcalls:OK
>> Summary: 1/16 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
>> ---
>>  .../selftests/bpf/prog_tests/tailcalls.c      | 299 ++++++++++++++++++
>>  .../bpf/progs/tailcall_bpf2bpf_fentry.c       |  18 ++
>>  .../bpf/progs/tailcall_bpf2bpf_fexit.c        |  18 ++
>>  3 files changed, 335 insertions(+)
>>  create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
>>  create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> index b20d7f77a5bce..331b4e455ad06 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> @@ -884,6 +884,297 @@ static void test_tailcall_bpf2bpf_6(void)
>>  	tailcall_bpf2bpf6__destroy(obj);
>>  }
>>  
>> +static void tailcall_bpf2bpf_fentry_fexit(bool test_fentry, bool test_fexit)
>> +{
>> +	struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL, *fexit_obj = NULL;
>> +	struct bpf_link *fentry_link = NULL, *fexit_link = NULL;
>> +	int err, map_fd, prog_fd, main_fd, data_fd, i, val;
>> +	struct bpf_map *prog_array, *data_map;
>> +	struct bpf_program *prog;
>> +	char buff[128] = {};
>> +
>> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
>> +		.data_in = buff,
>> +		.data_size_in = sizeof(buff),
>> +		.repeat = 1,
>> +	);
>> +
>> +	err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
>> +				 BPF_PROG_TYPE_SCHED_CLS,
>> +				 &tgt_obj, &prog_fd);
>> +	if (!ASSERT_OK(err, "load tgt_obj"))
>> +		return;
>> +
>> +	prog = bpf_object__find_program_by_name(tgt_obj, "entry");
>> +	if (!ASSERT_OK_PTR(prog, "find entry prog"))
>> +		goto out;
>> +
>> +	main_fd = bpf_program__fd(prog);
>> +	if (!ASSERT_FALSE(main_fd < 0, "find entry prog fd"))
>> +		goto out;
>> +
>> +	prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
>> +	if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
>> +		goto out;
>> +
>> +	map_fd = bpf_map__fd(prog_array);
>> +	if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
>> +		goto out;
>> +
>> +	prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
>> +	if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
>> +		goto out;
>> +
>> +	prog_fd = bpf_program__fd(prog);
>> +	if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
>> +		goto out;
>> +
>> +	i = 0;
>> +	err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
>> +	if (!ASSERT_OK(err, "update jmp_table"))
>> +		goto out;
>> +
>> +	if (test_fentry) {
>> +		fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
>> +						   NULL);
>> +		if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
>> +			goto out;
>> +
>> +		prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
>> +		if (!ASSERT_OK_PTR(prog, "find fentry prog"))
>> +			goto out;
>> +
>> +		err = bpf_program__set_attach_target(prog, prog_fd,
>> +						     "subprog_tail");
>> +		if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
>> +			goto out;
>> +
>> +		err = bpf_object__load(fentry_obj);
>> +		if (!ASSERT_OK(err, "load fentry_obj"))
>> +			goto out;
>> +
>> +		fentry_link = bpf_program__attach_trace(prog);
>> +		if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
>> +			goto out;
>> +	}
>> +
>> +	if (test_fexit) {
>> +		fexit_obj = bpf_object__open_file("tailcall_bpf2bpf_fexit.bpf.o",
>> +						  NULL);
>> +		if (!ASSERT_OK_PTR(fexit_obj, "open fexit_obj file"))
>> +			goto out;
>> +
>> +		prog = bpf_object__find_program_by_name(fexit_obj, "fexit");
>> +		if (!ASSERT_OK_PTR(prog, "find fexit prog"))
>> +			goto out;
>> +
>> +		err = bpf_program__set_attach_target(prog, prog_fd,
>> +						     "subprog_tail");
>> +		if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
>> +			goto out;
>> +
>> +		err = bpf_object__load(fexit_obj);
>> +		if (!ASSERT_OK(err, "load fexit_obj"))
>> +			goto out;
>> +
>> +		fexit_link = bpf_program__attach_trace(prog);
>> +		if (!ASSERT_OK_PTR(fexit_link, "attach_trace"))
>> +			goto out;
>> +	}
>> +
>> +	err = bpf_prog_test_run_opts(main_fd, &topts);
>> +	ASSERT_OK(err, "tailcall");
>> +	ASSERT_EQ(topts.retval, 1, "tailcall retval");
>> +
>> +	data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
>> +	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> +			  "find tailcall.bss map"))
>> +		goto out;
>> +
>> +	data_fd = bpf_map__fd(data_map);
>> +	if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
>> +		goto out;
>> +
>> +	i = 0;
>> +	err = bpf_map_lookup_elem(data_fd, &i, &val);
>> +	ASSERT_OK(err, "tailcall count");
>> +	ASSERT_EQ(val, 33, "tailcall count");
>> +
>> +	if (test_fentry) {
>> +		data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
>> +		if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> +				  "find tailcall_bpf2bpf_fentry.bss map"))
>> +			goto out;
>> +
>> +		data_fd = bpf_map__fd(data_map);
>> +		if (!ASSERT_FALSE(data_fd < 0,
>> +				  "find tailcall_bpf2bpf_fentry.bss map fd"))
>> +			goto out;
>> +
>> +		i = 0;
>> +		err = bpf_map_lookup_elem(data_fd, &i, &val);
>> +		ASSERT_OK(err, "fentry count");
>> +		ASSERT_EQ(val, 33, "fentry count");
>> +	}
>> +
>> +	if (test_fexit) {
>> +		data_map = bpf_object__find_map_by_name(fexit_obj, ".bss");
>> +		if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> +				  "find tailcall_bpf2bpf_fexit.bss map"))
>> +			goto out;
>> +
>> +		data_fd = bpf_map__fd(data_map);
>> +		if (!ASSERT_FALSE(data_fd < 0,
>> +				  "find tailcall_bpf2bpf_fexit.bss map fd"))
>> +			goto out;
>> +
>> +		i = 0;
>> +		err = bpf_map_lookup_elem(data_fd, &i, &val);
>> +		ASSERT_OK(err, "fexit count");
>> +		ASSERT_EQ(val, 33, "fexit count");
>> +	}
>> +
>> +out:
>> +	bpf_link__destroy(fentry_link);
>> +	bpf_link__destroy(fexit_link);
>> +	bpf_object__close(fentry_obj);
>> +	bpf_object__close(fexit_obj);
>> +	bpf_object__close(tgt_obj);
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fentry checks that the count value of the tail call
>> + * limit enforcement matches with expectations when tailcall is preceded with
>> + * bpf2bpf call, and the bpf2bpf call is traced by fentry.
>> + */
>> +static void test_tailcall_bpf2bpf_fentry(void)
>> +{
>> +	tailcall_bpf2bpf_fentry_fexit(true, false);
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fexit checks that the count value of the tail call
>> + * limit enforcement matches with expectations when tailcall is preceded with
>> + * bpf2bpf call, and the bpf2bpf call is traced by fexit.
>> + */
>> +static void test_tailcall_bpf2bpf_fexit(void)
>> +{
>> +	tailcall_bpf2bpf_fentry_fexit(false, true);
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fentry_fexit checks that the count value of the tail
>> + * call limit enforcement matches with expectations when tailcall is preceded
>> + * with bpf2bpf call, and the bpf2bpf call is traced by both fentry and fexit.
>> + */
>> +static void test_tailcall_bpf2bpf_fentry_fexit(void)
>> +{
>> +	tailcall_bpf2bpf_fentry_fexit(true, true);
> 
> Would it be possible to modify existing test_tailcall_count() to have
> fentry/fexit testing within? __tailcall_bpf2bpf_fentry_fexit() basically
> repeats the logic of test_tailcall_count(), right?
> 
> How about something like:
> 
> static void test_tailcall_bpf2bpf_fentry(void)
> {
> 	test_tailcall_count("tailcall_bpf2bpf2.bpf.o", true, false);
> }
> 
> // rest of your test cases
> 
> and existing tailcall count tests:
> 
> static void test_tailcall_3(void)
> {
> 	test_tailcall_count("tailcall3.bpf.o", false, false);
> }
> 
> static void test_tailcall_6(void)
> {
> 	test_tailcall_count("tailcall6.bpf.o", false, false);
> }

LGTM, I'll try it.

Thanks,
Leon

> 
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fentry_entry checks that the count value of the tail
>> + * call limit enforcement matches with expectations when tailcall is preceded
>> + * with bpf2bpf call, and the bpf2bpf caller is traced by fentry.
>> + */
>> +static void test_tailcall_bpf2bpf_fentry_entry(void)
>> +{
>> +	struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL;
>> +	int err, map_fd, prog_fd, data_fd, i, val;
>> +	struct bpf_map *prog_array, *data_map;
>> +	struct bpf_link *fentry_link = NULL;
>> +	struct bpf_program *prog;
>> +	char buff[128] = {};
>> +
>> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
>> +		.data_in = buff,
>> +		.data_size_in = sizeof(buff),
>> +		.repeat = 1,
>> +	);
>> +
>> +	err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
>> +				 BPF_PROG_TYPE_SCHED_CLS,
>> +				 &tgt_obj, &prog_fd);
>> +	if (!ASSERT_OK(err, "load tgt_obj"))
>> +		return;
>> +
>> +	prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
>> +	if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
>> +		goto out;
>> +
>> +	map_fd = bpf_map__fd(prog_array);
>> +	if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
>> +		goto out;
>> +
>> +	prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
>> +	if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
>> +		goto out;
>> +
>> +	prog_fd = bpf_program__fd(prog);
>> +	if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
>> +		goto out;
>> +
>> +	i = 0;
>> +	err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
>> +	if (!ASSERT_OK(err, "update jmp_table"))
>> +		goto out;
>> +
>> +	fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
>> +					   NULL);
>> +	if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
>> +		goto out;
>> +
>> +	prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
>> +	if (!ASSERT_OK_PTR(prog, "find fentry prog"))
>> +		goto out;
>> +
>> +	err = bpf_program__set_attach_target(prog, prog_fd, "classifier_0");
>> +	if (!ASSERT_OK(err, "set_attach_target classifier_0"))
>> +		goto out;
>> +
>> +	err = bpf_object__load(fentry_obj);
>> +	if (!ASSERT_OK(err, "load fentry_obj"))
>> +		goto out;
>> +
>> +	fentry_link = bpf_program__attach_trace(prog);
>> +	if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
>> +		goto out;
>> +
>> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
>> +	ASSERT_OK(err, "tailcall");
>> +	ASSERT_EQ(topts.retval, 1, "tailcall retval");
>> +
>> +	data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
>> +	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> +			  "find tailcall.bss map"))
>> +		goto out;
>> +
>> +	data_fd = bpf_map__fd(data_map);
>> +	if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
>> +		goto out;
>> +
>> +	i = 0;
>> +	err = bpf_map_lookup_elem(data_fd, &i, &val);
>> +	ASSERT_OK(err, "tailcall count");
>> +	ASSERT_EQ(val, 34, "tailcall count");
>> +
>> +	data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
>> +	if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> +			  "find tailcall_bpf2bpf_fentry.bss map"))
>> +		goto out;
>> +
>> +	data_fd = bpf_map__fd(data_map);
>> +	if (!ASSERT_FALSE(data_fd < 0,
>> +			  "find tailcall_bpf2bpf_fentry.bss map fd"))
>> +		goto out;
>> +
>> +	i = 0;
>> +	err = bpf_map_lookup_elem(data_fd, &i, &val);
>> +	ASSERT_OK(err, "fentry count");
>> +	ASSERT_EQ(val, 1, "fentry count");
>> +
>> +out:
>> +	bpf_link__destroy(fentry_link);
>> +	bpf_object__close(fentry_obj);
>> +	bpf_object__close(tgt_obj);
>> +}
>> +
>>  void test_tailcalls(void)
>>  {
>>  	if (test__start_subtest("tailcall_1"))
>> @@ -910,4 +1201,12 @@ void test_tailcalls(void)
>>  		test_tailcall_bpf2bpf_4(true);
>>  	if (test__start_subtest("tailcall_bpf2bpf_6"))
>>  		test_tailcall_bpf2bpf_6();
>> +	if (test__start_subtest("tailcall_bpf2bpf_fentry"))
>> +		test_tailcall_bpf2bpf_fentry();
>> +	if (test__start_subtest("tailcall_bpf2bpf_fexit"))
>> +		test_tailcall_bpf2bpf_fexit();
>> +	if (test__start_subtest("tailcall_bpf2bpf_fentry_fexit"))
>> +		test_tailcall_bpf2bpf_fentry_fexit();
>> +	if (test__start_subtest("tailcall_bpf2bpf_fentry_entry"))
>> +		test_tailcall_bpf2bpf_fentry_entry();
>>  }
>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
>> new file mode 100644
>> index 0000000000000..8436c6729167c
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
>> @@ -0,0 +1,18 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright Leon Hwang */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +int count = 0;
>> +
>> +SEC("fentry/subprog_tail")
>> +int BPF_PROG(fentry, struct sk_buff *skb)
>> +{
>> +	count++;
>> +
>> +	return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
>> new file mode 100644
>> index 0000000000000..fe16412c6e6e9
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
>> @@ -0,0 +1,18 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright Leon Hwang */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +int count = 0;
>> +
>> +SEC("fexit/subprog_tail")
>> +int BPF_PROG(fexit, struct sk_buff *skb)
>> +{
>> +	count++;
>> +
>> +	return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>> -- 
>> 2.41.0
>>

Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop

[Ilya Leoshkevich]

On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
> This patch series fixes a tailcall infinite loop on x64.
> 
> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
> tailcall
> handling in JIT"), the tailcall on x64 works better than before.
> 
> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
> subprograms
> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
> 
> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
> program
> to other BPF programs"), BPF program is able to trace other BPF
> programs.
> 
> How about combining them all together?
> 
> 1. FENTRY/FEXIT on a BPF subprogram.
> 2. A tailcall runs in the BPF subprogram.
> 3. The tailcall calls the subprogram's caller.
> 
> As a result, a tailcall infinite loop comes up. And the loop would
> halt
> the machine.
> 
> As we know, in tail call context, the tail_call_cnt propagates by
> stack
> and rax register between BPF subprograms. So do in trampolines.
> 
> How did I discover the bug?
> 
> From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth 256
> for
> subprogs with tailcalls"), the total stack size limits to around
> 8KiB.
> Then, I write some bpf progs to validate the stack consuming, that
> are
> tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on bpf2bpf[1].
> 
> At that time, accidently, I made a tailcall loop. And then the loop
> halted
> my VM. Without the loop, the bpf progs would consume over 8KiB stack
> size.
> But the _stack-overflow_ did not halt my VM.
> 
> With bpf_printk(), I confirmed that the tailcall count limit did not
> work
> expectedly. Next, read the code and fix it.
> 
> Finally, unfortunately, I only fix it on x64 but other arches. As a
> result, CI tests failed because this bug hasn't been fixed on s390x.
> 
> Some helps on s390x are requested.

I will take a look, thanks for letting me know.
I noticed there was something peculiar in this area when implementing
the trampoline:

	 * Note 1: The callee can increment the tail call counter, but
	 * we do not load it back, since the x86 JIT does not do this
	 * either.

but I thought that this was intentional.


[...]

Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop

[Leon Hwang]

On 2023/9/4 21:10, Ilya Leoshkevich wrote:
> On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
>> This patch series fixes a tailcall infinite loop on x64.
>>
>> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
>> tailcall
>> handling in JIT"), the tailcall on x64 works better than before.
>>
>> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
>> subprograms
>> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
>>
>> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
>> program
>> to other BPF programs"), BPF program is able to trace other BPF
>> programs.
>>
>> How about combining them all together?
>>
>> 1. FENTRY/FEXIT on a BPF subprogram.
>> 2. A tailcall runs in the BPF subprogram.
>> 3. The tailcall calls the subprogram's caller.
>>
>> As a result, a tailcall infinite loop comes up. And the loop would
>> halt
>> the machine.
>>
>> As we know, in tail call context, the tail_call_cnt propagates by
>> stack
>> and rax register between BPF subprograms. So do in trampolines.
>>
>> How did I discover the bug?
>>
>> From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth 256
>> for
>> subprogs with tailcalls"), the total stack size limits to around
>> 8KiB.
>> Then, I write some bpf progs to validate the stack consuming, that
>> are
>> tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on bpf2bpf[1].
>>
>> At that time, accidently, I made a tailcall loop. And then the loop
>> halted
>> my VM. Without the loop, the bpf progs would consume over 8KiB stack
>> size.
>> But the _stack-overflow_ did not halt my VM.
>>
>> With bpf_printk(), I confirmed that the tailcall count limit did not
>> work
>> expectedly. Next, read the code and fix it.
>>
>> Finally, unfortunately, I only fix it on x64 but other arches. As a
>> result, CI tests failed because this bug hasn't been fixed on s390x.
>>
>> Some helps on s390x are requested.
> 
> I will take a look, thanks for letting me know.

Great.

> I noticed there was something peculiar in this area when implementing
> the trampoline:
> 
> 	 * Note 1: The callee can increment the tail call counter, but
> 	 * we do not load it back, since the x86 JIT does not do this
> 	 * either.>
> but I thought that this was intentional.

I do think so.

But wait, should we load it back?

Let me show a demo:

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

static __noinline
int subprog_tail_01(struct __sk_buff *skb)
{
	if (load_byte(skb, 0))
		bpf_tail_call_static(skb, &jmp_table, 1);
	else
		bpf_tail_call_static(skb, &jmp_table, 0);
	return 1;
}

static __noinline
int subprog_tail_23(struct __sk_buff *skb)
{
	if (load_byte(skb, 0))
		bpf_tail_call_static(skb, &jmp_table, 3);
	else
		bpf_tail_call_static(skb, &jmp_table, 2);
	return 1;
}

int count0 = 0;

SEC("tc")
int classifier_01(struct __sk_buff *skb)
{
	count0++;
	return subprog_tail_01(skb);
}

int count1 = 0;

SEC("tc")
int classifier_23(struct __sk_buff *skb)
{
	count1++;
	return subprog_tail_23(skb);
}

static __noinline
int subprog_tailcall(struct __sk_buff *skb, int index)
{
	volatile int retval = 0;
	bpf_tail_call(skb, &jmp_table, index);
	return retval;
}

SEC("tc")
int entry(struct __sk_buff *skb)
{
	subprog_tailcall(skb, 0);
	subprog_tailcall(skb, 2);

	return 0;
}

Finally, count0 is 33. And count1 is 33, too. It breaks the
MAX_TAIL_CALL_CNT limit by the way tailcall in subprog.

From 04fd61ab36ec065e ("bpf: allow bpf programs to tail-call other bpf
programs"):
The chain of tail calls can form unpredictable dynamic loops therefore
tail_call_cnt is used to limit the number of calls and currently is set
to 32.

It seems like that MAX_TAIL_CALL_CNT limits the max tailcall hierarchy
layers.

So, should we load it back?

Thanks,
Leon

Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop

[Ilya Leoshkevich]

On Mon, 2023-09-04 at 23:15 +0800, Leon Hwang wrote:
> 
> 
> On 2023/9/4 21:10, Ilya Leoshkevich wrote:
> > On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
> > > This patch series fixes a tailcall infinite loop on x64.
> > > 
> > > From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
> > > tailcall
> > > handling in JIT"), the tailcall on x64 works better than before.
> > > 
> > > From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
> > > subprograms
> > > for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
> > > 
> > > From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
> > > program
> > > to other BPF programs"), BPF program is able to trace other BPF
> > > programs.
> > > 
> > > How about combining them all together?
> > > 
> > > 1. FENTRY/FEXIT on a BPF subprogram.
> > > 2. A tailcall runs in the BPF subprogram.
> > > 3. The tailcall calls the subprogram's caller.
> > > 
> > > As a result, a tailcall infinite loop comes up. And the loop
> > > would
> > > halt
> > > the machine.
> > > 
> > > As we know, in tail call context, the tail_call_cnt propagates by
> > > stack
> > > and rax register between BPF subprograms. So do in trampolines.
> > > 
> > > How did I discover the bug?
> > > 
> > > From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth
> > > 256
> > > for
> > > subprogs with tailcalls"), the total stack size limits to around
> > > 8KiB.
> > > Then, I write some bpf progs to validate the stack consuming,
> > > that
> > > are
> > > tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on
> > > bpf2bpf[1].
> > > 
> > > At that time, accidently, I made a tailcall loop. And then the
> > > loop
> > > halted
> > > my VM. Without the loop, the bpf progs would consume over 8KiB
> > > stack
> > > size.
> > > But the _stack-overflow_ did not halt my VM.
> > > 
> > > With bpf_printk(), I confirmed that the tailcall count limit did
> > > not
> > > work
> > > expectedly. Next, read the code and fix it.
> > > 
> > > Finally, unfortunately, I only fix it on x64 but other arches. As
> > > a
> > > result, CI tests failed because this bug hasn't been fixed on
> > > s390x.
> > > 
> > > Some helps on s390x are requested.
> > 
> > I will take a look, thanks for letting me know.
> 
> Great.
> 
> > I noticed there was something peculiar in this area when
> > implementing
> > the trampoline:
> > 
> >          * Note 1: The callee can increment the tail call counter,
> > but
> >          * we do not load it back, since the x86 JIT does not do
> > this
> >          * either.>
> > but I thought that this was intentional.
> 
> I do think so.
> 
> But wait, should we load it back?
> 
> Let me show a demo:
> 
> struct {
>         __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>         __uint(max_entries, 4);
>         __uint(key_size, sizeof(__u32));
>         __uint(value_size, sizeof(__u32));
> } jmp_table SEC(".maps");
> 
> static __noinline
> int subprog_tail_01(struct __sk_buff *skb)
> {
>         if (load_byte(skb, 0))
>                 bpf_tail_call_static(skb, &jmp_table, 1);
>         else
>                 bpf_tail_call_static(skb, &jmp_table, 0);
>         return 1;
> }
> 
> static __noinline
> int subprog_tail_23(struct __sk_buff *skb)
> {
>         if (load_byte(skb, 0))
>                 bpf_tail_call_static(skb, &jmp_table, 3);
>         else
>                 bpf_tail_call_static(skb, &jmp_table, 2);
>         return 1;
> }
> 
> int count0 = 0;
> 
> SEC("tc")
> int classifier_01(struct __sk_buff *skb)
> {
>         count0++;
>         return subprog_tail_01(skb);
> }
> 
> int count1 = 0;
> 
> SEC("tc")
> int classifier_23(struct __sk_buff *skb)
> {
>         count1++;
>         return subprog_tail_23(skb);
> }
> 
> static __noinline
> int subprog_tailcall(struct __sk_buff *skb, int index)
> {
>         volatile int retval = 0;
>         bpf_tail_call(skb, &jmp_table, index);
>         return retval;
> }
> 
> SEC("tc")
> int entry(struct __sk_buff *skb)
> {
>         subprog_tailcall(skb, 0);
>         subprog_tailcall(skb, 2);
> 
>         return 0;
> }
> 
> Finally, count0 is 33. And count1 is 33, too. It breaks the
> MAX_TAIL_CALL_CNT limit by the way tailcall in subprog.

Thanks for coming up with an example, I could not create something like
this back then and just left a note for the future.

> From 04fd61ab36ec065e ("bpf: allow bpf programs to tail-call other
> bpf
> programs"):
> The chain of tail calls can form unpredictable dynamic loops
> therefore
> tail_call_cnt is used to limit the number of calls and currently is
> set
> to 32.
> 
> It seems like that MAX_TAIL_CALL_CNT limits the max tailcall
> hierarchy
> layers.
> 
> So, should we load it back?

I wonder if the current behavior can lead to high system load in some
cases. The current limit on the number of instructions is 1M; with tail
calls it goes up to MAX_TAIL_CALL_CNT * 1M. If one uses the technique
above to the fullest extent, i.e., on each tail call hierarchy level,
can't one execute (2 ** MAX_TAIL_CALL_CNT) * 1M instructions as a
result?

> Thanks,
> Leon

Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop

[Leon Hwang]

On 6/9/23 08:57, Ilya Leoshkevich wrote:
> On Mon, 2023-09-04 at 23:15 +0800, Leon Hwang wrote:
>>
>>
>> On 2023/9/4 21:10, Ilya Leoshkevich wrote:
>>> On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
>>>> This patch series fixes a tailcall infinite loop on x64.
>>>>
>>>> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
>>>> tailcall
>>>> handling in JIT"), the tailcall on x64 works better than before.
>>>>
>>>> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
>>>> subprograms
>>>> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
>>>>
>>>> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
>>>> program
>>>> to other BPF programs"), BPF program is able to trace other BPF
>>>> programs.
>>>>
>>>> How about combining them all together?
>>>>
>>>> 1. FENTRY/FEXIT on a BPF subprogram.
>>>> 2. A tailcall runs in the BPF subprogram.
>>>> 3. The tailcall calls the subprogram's caller.
>>>>
>>>> As a result, a tailcall infinite loop comes up. And the loop
>>>> would
>>>> halt
>>>> the machine.
>>>>
>>>> As we know, in tail call context, the tail_call_cnt propagates by
>>>> stack
>>>> and rax register between BPF subprograms. So do in trampolines.
>>>>
>>>> How did I discover the bug?
>>>>
>>>> From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth
>>>> 256
>>>> for
>>>> subprogs with tailcalls"), the total stack size limits to around
>>>> 8KiB.
>>>> Then, I write some bpf progs to validate the stack consuming,
>>>> that
>>>> are
>>>> tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on
>>>> bpf2bpf[1].
>>>>
>>>> At that time, accidently, I made a tailcall loop. And then the
>>>> loop
>>>> halted
>>>> my VM. Without the loop, the bpf progs would consume over 8KiB
>>>> stack
>>>> size.
>>>> But the _stack-overflow_ did not halt my VM.
>>>>
>>>> With bpf_printk(), I confirmed that the tailcall count limit did
>>>> not
>>>> work
>>>> expectedly. Next, read the code and fix it.
>>>>
>>>> Finally, unfortunately, I only fix it on x64 but other arches. As
>>>> a
>>>> result, CI tests failed because this bug hasn't been fixed on
>>>> s390x.
>>>>
>>>> Some helps on s390x are requested.
>>>
>>> I will take a look, thanks for letting me know.
>>
>> Great.
>>
>>> I noticed there was something peculiar in this area when
>>> implementing
>>> the trampoline:
>>>
>>>          * Note 1: The callee can increment the tail call counter,
>>> but
>>>          * we do not load it back, since the x86 JIT does not do
>>> this
>>>          * either.>
>>> but I thought that this was intentional.
>>
>> I do think so.
>>
>> But wait, should we load it back?
>>
>> Let me show a demo:
>>
>> struct {
>>         __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>>         __uint(max_entries, 4);
>>         __uint(key_size, sizeof(__u32));
>>         __uint(value_size, sizeof(__u32));
>> } jmp_table SEC(".maps");
>>
>> static __noinline
>> int subprog_tail_01(struct __sk_buff *skb)
>> {
>>         if (load_byte(skb, 0))
>>                 bpf_tail_call_static(skb, &jmp_table, 1);
>>         else
>>                 bpf_tail_call_static(skb, &jmp_table, 0);
>>         return 1;
>> }
>>
>> static __noinline
>> int subprog_tail_23(struct __sk_buff *skb)
>> {
>>         if (load_byte(skb, 0))
>>                 bpf_tail_call_static(skb, &jmp_table, 3);
>>         else
>>                 bpf_tail_call_static(skb, &jmp_table, 2);
>>         return 1;
>> }
>>
>> int count0 = 0;
>>
>> SEC("tc")
>> int classifier_01(struct __sk_buff *skb)
>> {
>>         count0++;
>>         return subprog_tail_01(skb);
>> }
>>
>> int count1 = 0;
>>
>> SEC("tc")
>> int classifier_23(struct __sk_buff *skb)
>> {
>>         count1++;
>>         return subprog_tail_23(skb);
>> }
>>
>> static __noinline
>> int subprog_tailcall(struct __sk_buff *skb, int index)
>> {
>>         volatile int retval = 0;
>>         bpf_tail_call(skb, &jmp_table, index);
>>         return retval;
>> }
>>
>> SEC("tc")
>> int entry(struct __sk_buff *skb)
>> {
>>         subprog_tailcall(skb, 0);
>>         subprog_tailcall(skb, 2);
>>
>>         return 0;
>> }
>>
>> Finally, count0 is 33. And count1 is 33, too. It breaks the
>> MAX_TAIL_CALL_CNT limit by the way tailcall in subprog.
> 
> Thanks for coming up with an example, I could not create something like
> this back then and just left a note for the future.
> 
>> From 04fd61ab36ec065e ("bpf: allow bpf programs to tail-call other
>> bpf
>> programs"):
>> The chain of tail calls can form unpredictable dynamic loops
>> therefore
>> tail_call_cnt is used to limit the number of calls and currently is
>> set
>> to 32.
>>
>> It seems like that MAX_TAIL_CALL_CNT limits the max tailcall
>> hierarchy
>> layers.
>>
>> So, should we load it back?
> 
> I wonder if the current behavior can lead to high system load in some
> cases. The current limit on the number of instructions is 1M; with tail
> calls it goes up to MAX_TAIL_CALL_CNT * 1M. If one uses the technique
> above to the fullest extent, i.e., on each tail call hierarchy level,
> can't one execute (2 ** MAX_TAIL_CALL_CNT) * 1M instructions as a
> result?

Executing (2 ** MAX_TAIL_CALL_CNT) * 1M instructions is really harmful.

So, we should load it back.

I'll try to fix it with another patchset.

Thanks,
Leon