Re: [PATCH bpf-next v5] bpf: Change value of MAX_TAIL_CALL_CNT from 32 to 33

[Ilya Leoshkevich]

On Thu, 2021-11-04 at 09:50 +0800, Tiezhu Yang wrote:
> In the current code, the actual max tail call count is 33 which is
> greater
> than MAX_TAIL_CALL_CNT (defined as 32), the actual limit is not
> consistent
> with the meaning of MAX_TAIL_CALL_CNT, there is some confusion and
> need to
> spend some time to think about the reason at the first glance.
> 
> We can see the historical evolution from commit 04fd61ab36ec ("bpf:
> allow
> bpf programs to tail-call other bpf programs") and commit
> f9dabe016b63
> ("bpf: Undo off-by-one in interpreter tail call count limit").
> 
> In order to avoid changing existing behavior, the actual limit is 33
> now,
> this is reasonable.
> 
> After commit 874be05f525e ("bpf, tests: Add tail call test suite"),
> we can
> see there exists failed testcase.
> 
> On all archs when CONFIG_BPF_JIT_ALWAYS_ON is not set:
>  # echo 0 > /proc/sys/net/core/bpf_jit_enable
>  # modprobe test_bpf
>  # dmesg | grep -w FAIL
>  Tail call error path, max count reached jited:0 ret 34 != 33 FAIL
> 
> On some archs:
>  # 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 34 != 33 FAIL
> 
> Although the above failed testcase has been fixed in commit
> 18935a72eb25
> ("bpf/tests: Fix error in tail call limit tests"), it is still
> necessary
> to change the value of MAX_TAIL_CALL_CNT from 32 to 33 to make the
> code
> more readable, then do some small changes of the related code.
> 
> With this patch, it does not change the current limit 33,
> MAX_TAIL_CALL_CNT
> can reflect the actual max tail call count, the related tailcall
> testcases
> in test_bpf and selftests can work well for the interpreter and the
> JIT.
> 
> Here are the test results on x86_64:
> 
>  # uname -m
>  x86_64
>  # echo 0 > /proc/sys/net/core/bpf_jit_enable
>  # modprobe test_bpf test_suite=test_tail_calls
>  # dmesg | tail -1
>  test_bpf: test_tail_calls: Summary: 8 PASSED, 0 FAILED, [0/8 JIT'ed]
>  # rmmod test_bpf
>  # echo 1 > /proc/sys/net/core/bpf_jit_enable
>  # modprobe test_bpf test_suite=test_tail_calls
>  # dmesg | tail -1
>  test_bpf: test_tail_calls: Summary: 8 PASSED, 0 FAILED, [8/8 JIT'ed]
>  # rmmod test_bpf
>  # ./test_progs -t tailcalls
>  #142 tailcalls:OK
>  Summary: 1/11 PASSED, 0 SKIPPED, 0 FAILED
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>
> Acked-by: Björn Töpel <bjorn@xxxxxxxxxx>
> ---
> 
> v5: Use RV_REG_TCC directly to save one move for RISC-V,
>     suggested by Björn Töpel, thank you.
> 
> v4: Use >= as check condition,
>     suggested by Daniel Borkmann, thank you.
> 
>  arch/arm/net/bpf_jit_32.c         |  5 +++--
>  arch/arm64/net/bpf_jit_comp.c     |  5 +++--
>  arch/mips/net/bpf_jit_comp32.c    |  2 +-
>  arch/mips/net/bpf_jit_comp64.c    |  2 +-
>  arch/powerpc/net/bpf_jit_comp32.c |  4 ++--
>  arch/powerpc/net/bpf_jit_comp64.c |  4 ++--
>  arch/riscv/net/bpf_jit_comp32.c   |  6 ++----
>  arch/riscv/net/bpf_jit_comp64.c   |  7 +++----
>  arch/s390/net/bpf_jit_comp.c      |  6 +++---
>  arch/sparc/net/bpf_jit_comp_64.c  |  2 +-
>  arch/x86/net/bpf_jit_comp.c       | 10 +++++-----
>  arch/x86/net/bpf_jit_comp32.c     |  4 ++--
>  include/linux/bpf.h               |  2 +-
>  include/uapi/linux/bpf.h          |  2 +-
>  kernel/bpf/core.c                 |  3 ++-
>  lib/test_bpf.c                    |  4 ++--
>  tools/include/uapi/linux/bpf.h    |  2 +-
>  17 files changed, 35 insertions(+), 35 deletions(-)

[...]

> diff --git a/arch/s390/net/bpf_jit_comp.c
> b/arch/s390/net/bpf_jit_comp.c
> index 1a374d0..3553cfc 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -1369,7 +1369,7 @@ static noinline int bpf_jit_insn(struct bpf_jit
> *jit, struct bpf_prog *fp,
>                                  jit->prg);
>  
>                 /*
> -                * if (tail_call_cnt++ > MAX_TAIL_CALL_CNT)
> +                * if (tail_call_cnt++ >= MAX_TAIL_CALL_CNT)
>                  *         goto out;
>                  */
>  
> @@ -1381,9 +1381,9 @@ static noinline int bpf_jit_insn(struct bpf_jit
> *jit, struct bpf_prog *fp,
>                 EMIT4_IMM(0xa7080000, REG_W0, 1);
>                 /* laal %w1,%w0,off(%r15) */
>                 EMIT6_DISP_LH(0xeb000000, 0x00fa, REG_W1, REG_W0,
> REG_15, off);
> -               /* clij %w1,MAX_TAIL_CALL_CNT,0x2,out */
> +               /* clij %w1,MAX_TAIL_CALL_CNT-1,0x2,out */
>                 patch_2_clij = jit->prg;
> -               EMIT6_PCREL_RIEC(0xec000000, 0x007f, REG_W1,
> MAX_TAIL_CALL_CNT,
> +               EMIT6_PCREL_RIEC(0xec000000, 0x007f, REG_W1,
> MAX_TAIL_CALL_CNT - 1,
>                                  2, jit->prg);
>  
>                 /*

For the s390 part:

Tested-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
Acked-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>

[...]