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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Aug 2021 17:41:57 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Johan Almbladh <johan.almbladh@...finetworks.com>, ast@...nel.org,
        andrii@...nel.org
Cc:     kafai@...com, songliubraving@...com, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org,
        illusionist.neo@...il.com, zlim.lnx@...il.com,
        paulburton@...nel.org, naveen.n.rao@...ux.ibm.com,
        sandipan@...ux.ibm.com, luke.r.nels@...il.com, bjorn@...nel.org,
        iii@...ux.ibm.com, hca@...ux.ibm.com, gor@...ux.ibm.com,
        davem@...emloft.net, udknight@...il.com
Subject: Re: [PATCH bpf-next 7/7] x86: bpf: Fix comments on tail call count
 limiting

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@...finetworks.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 */
>