Messages in this thread | | | From | Tiezhu Yang <> | Subject | Re: [PATCH bpf-next v2] bpf: Change value of MAX_TAIL_CALL_CNT from 32 to 33 | Date | Tue, 14 Sep 2021 20:36:20 +0800 |
| |
On 09/14/2021 03:30 PM, Daniel Borkmann wrote: > On 9/11/21 3:56 AM, Tiezhu Yang wrote: >> [...] >> With this patch, it does not change the current limit 33, >> MAX_TAIL_CALL_CNT >> can reflect the actual max tail call count, the tailcall selftests >> can work >> well, and also the above failed testcase in test_bpf can be fixed for >> the >> interpreter (all archs) and the JIT (all archs except for x86). >> >> # uname -m >> x86_64 >> # echo 1 > /proc/sys/net/core/bpf_jit_enable >> # modprobe test_bpf >> # dmesg | grep -w FAIL >> Tail call error path, max count reached jited:1 ret 33 != 34 FAIL > > Could you also state in here which archs you have tested with this > change? I > presume /every/ arch which has a JIT?
OK, will do it in v3. I have tested on x86 and mips.
> >> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >> --- >> >> v2: >> -- fix the typos in the commit message and update the commit message. >> -- fix the failed tailcall selftests for x86 jit. >> I am not quite sure the change on x86 is proper, with this change, >> tailcall selftests passed, but tailcall limit test in test_bpf.ko >> failed, I do not know the reason now, I think this is another >> issue, >> maybe someone more versed in x86 jit could take a look. > > There should be a series from Johan coming today with regards to > test_bpf.ko > that will fix the "tail call error path, max count reached" test which > had an > assumption in that R0 would always be valid for the fall-through and > could be > passed to the bpf_exit insn whereas it is not guaranteed and verifier, > for > example, forbids a subsequent access to R0 w/o reinit. For your > testing, I > would suggested to recheck once this series is out.
I will test the following patch on x86 and mips:
[PATCH bpf v4 13/14] bpf/tests: Fix error in tail call limit tests
[...]
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c >> index 0fe6aac..74a9e61 100644 >> --- a/arch/x86/net/bpf_jit_comp.c >> +++ b/arch/x86/net/bpf_jit_comp.c >> @@ -402,7 +402,7 @@ static int get_pop_bytes(bool *callee_regs_used) >> * ... 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) > > Why such inconsistency to e.g. above with arm64 case but also compared to > x86 32 bit which uses JAE? If so, we should cleanly follow the reference > implementation (== interpreter) _everywhere_ and _not_ introduce > additional > variants/implementations across JITs.
In order tokeep consistencyand make as few changes as possible, <javascript:void(0);>I will modify the check condition as follows:
#define MAX_TAIL_CALL_CNT 33 (1) for x86, arm64, ... (0 ~ 32) tcc = 0; if (tcc == MAX_TAIL_CALL_CNT) goto out; tcc++;
(2) for mips, riscv (33 ~ 1) tcc = MAX_TAIL_CALL_CNT; if (tcc == 0) goto out; tcc--;
[...]
| |