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 20:02:33 +0200
From:   Johan Almbladh <johan.almbladh@...finetworks.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        illusionist.neo@...il.com, zlim.lnx@...il.com,
        Paul Burton <paulburton@...nel.org>,
        naveen.n.rao@...ux.ibm.com, sandipan@...ux.ibm.com,
        Luke Nelson <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 Mon, Aug 9, 2021 at 5:42 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> 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?

I tested both the 64-bit and the 32-bit JITs with QEMU. Both passed,
meaning that the tail call recursion stopped after 32 tail calls.
However, the comments in the code indicated that it would allow one
more call, and also said JA when it actually emitted JAE. This patch
merely fixes the comments in the 32-bit JIT to match the code.

> 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? ;)

Yes, the x86-64 JIT is different. It also pass the test, but I do find
the code and comments a bit confusing too. Since it pass the test, and
the top-level comment correctly states the stop condition as
++tail_call_cnt > MAX_TAIL_CALL_CNT, I left it at that.

On a side note, I see that the x86-64 JIT also has a direct tail call
code path which the other JITs don't seem to have. The tail call test
only checks the indirect tail call code path.

>
> >       /* add eax,0x1 */
> >
>