On Thu, Jul 29, 2021 at 3:29 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Thu, Jul 29, 2021 at 2:38 PM Johan Almbladh > <johan.almbladh@xxxxxxxxxxxxxxxxx> wrote: > > > > On Wed, Jul 28, 2021 at 9:13 PM Yonghong Song <yhs@xxxxxx> wrote: > > > I also checked arm/arm64 jit. I saw the following comments: > > > > > > /* if (tail_call_cnt > MAX_TAIL_CALL_CNT) > > > * goto out; > > > * tail_call_cnt++; > > > */ > > > > > > Maybe we have this MAX_TAIL_CALL_CNT + 1 issue > > > for arm/arm64 jit? > > > > That wouldn't be unreasonable. I don't have an arm or arm64 setup > > available right now, but I can try to test it in qemu. > > On a brief check, there seems to be quite a mess in terms of the code > and comments. > > E.g., in arch/x86/net/bpf_jit_comp32.c: > > /* > * if (tail_call_cnt > MAX_TAIL_CALL_CNT) > * goto out; > */ > > ^^^^ here comment is wrong > > [...] > > /* cmp edx,hi */ > EMIT3(0x83, add_1reg(0xF8, IA32_EBX), hi); > EMIT2(IA32_JNE, 3); > /* cmp ecx,lo */ > EMIT3(0x83, add_1reg(0xF8, IA32_ECX), lo); > > /* ja out */ > EMIT2(IA32_JAE, jmp_label(jmp_label1, 2)); > > ^^^ JAE is >=, right? But the comment says JA. > > > As for arch/x86/net/bpf_jit_comp.c, both comment and the code seem to > do > MAX_TAIL_CALL_CNT, but you are saying JIT is correct. What am I > missing? > > Can you please check all the places where MAX_TAIL_CALL_CNT is used > throughout the code? Let's clean this up in one go. > > Also, given it's so easy to do this off-by-one error, can you please > add a negative test validating that 33 tail calls are not allowed? I > assume we have a positive test that allows exactly MAX_TAIL_CALL_CNT, > but please double-check that as well. Ok, I see that you've added this in your bpf tests patch set. Please consider, additionally, implementing a similar test as part of selftests/bpf (specifically in test_progs). We run test_progs continuously in CI for every incoming patch/patchset, so it has much higher chances of capturing any regressions. I'm also thinking that this MAX_TAIL_CALL_CNT change should probably go into the bpf-next tree. First, this off-by-one behavior was around for a while and it doesn't cause serious issues, even if abused. But on the other hand, it will make your tail call tests fail, when applied into bpf-next without your change. So I think we should apply both into bpf-next. On a related topic, please don't forget to include the target kernel tree for your patches: [PATCH bpf] or [PATCH bpf-next]. > > I also wonder if it would make sense to convert these > internal-but-sort-of-advertised constants like MAX_TAIL_CALL_CNT and > BPF_COMPLEXITY_LIMIT_INSNS into enums so that they can be "discovered" > from BTF. This should be discussed/attempted outside of this fix, > though. Just bringing it up here.
