| Message ID | 20210809093437.876558-8-johan.almbladh@anyfinetworks.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix MAX_TAIL_CALL_CNT handling in eBPF JITs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: mingo@redhat.com dsahern@kernel.org x86@kernel.org bp@alien8.de hpa@zytor.com tglx@linutronix.de yoshfuji@linux-ipv6.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 */ >
On Mon, Aug 9, 2021 at 5:42 PM Daniel Borkmann <daniel@iogearbox.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@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? 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 */ > > >
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)); /* add eax,0x1 */
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(-)