Re: [PATCH bpf-next 7/7] x86: bpf: Fix comments on tail call count limiting

[Daniel Borkmann <daniel@iogearbox.net>]

On 8/9/21 11:34 AM, Johan Almbladh wrote:
> Before, the comments in the 32-bit eBPF JIT claimed that up to
> MAX_TAIL_CALL_CNT + 1 tail calls were allowed, when in fact the
> implementation was using the correct limit of MAX_TAIL_CALL_CNT.
> Now, the comments are in line with what the code actually does.
> 
> Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com>
> ---
>   arch/x86/net/bpf_jit_comp32.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index 3bfda5f502cb..8db9ab11abda 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c
> @@ -1272,7 +1272,7 @@ static void emit_epilogue(u8 **pprog, u32 stack_depth)
>    * ... bpf_tail_call(void *ctx, struct bpf_array *array, u64 index) ...
>    *   if (index >= array->map.max_entries)
>    *     goto out;
> - *   if (++tail_call_cnt > MAX_TAIL_CALL_CNT)
> + *   if (tail_call_cnt++ >= MAX_TAIL_CALL_CNT)
>    *     goto out;
>    *   prog = array->ptrs[index];
>    *   if (prog == NULL)
> @@ -1307,7 +1307,7 @@ static void emit_bpf_tail_call(u8 **pprog)
>   	EMIT2(IA32_JBE, jmp_label(jmp_label1, 2));
>   
>   	/*
> -	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
> +	 * if (tail_call_cnt >= MAX_TAIL_CALL_CNT)
>   	 *     goto out;
>   	 */
>   	lo = (u32)MAX_TAIL_CALL_CNT;
> @@ -1321,7 +1321,7 @@ static void emit_bpf_tail_call(u8 **pprog)
>   	/* cmp ecx,lo */
>   	EMIT3(0x83, add_1reg(0xF8, IA32_ECX), lo);
>   
> -	/* ja out */
> +	/* jae out */
>   	EMIT2(IA32_JAE, jmp_label(jmp_label1, 2));

You have me confused here ... b61a28cf11d6 ("bpf: Fix off-by-one in tail call count
limiting") from bpf-next says '[interpreter is now] in line with the behavior of the
x86 JITs'. From the latter I assumed you implicitly refer to x86-64. Which one did you
test specifically wrt the prior statement? It looks like x86-64 vs x86-32 differ:

   [...]
   EMIT2_off32(0x8B, 0x85, tcc_off);         /* mov eax, dword ptr [rbp - tcc_off] */
   EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
   EMIT2(X86_JA, OFFSET2);                   /* ja out */
   EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
   EMIT2_off32(0x89, 0x85, tcc_off);         /* mov dword ptr [rbp - tcc_off], eax */
   [...]

So it's ja vs jae ... unless I need more coffee? ;)

>   	/* add eax,0x1 */
>