bpf: Change value of MAX_TAIL_CALL_CNT from 32 to 33
authorTiezhu Yang <yangtiezhu@loongson.cn>
Fri, 5 Nov 2021 01:30:00 +0000 (09:30 +0800)
committerDaniel Borkmann <daniel@iogearbox.net>
Tue, 16 Nov 2021 13:03:15 +0000 (14:03 +0100)
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 and thus confusing at 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 would still be good
to change the value of MAX_TAIL_CALL_CNT from 32 to 33 to make the code
more readable.

The 32-bit x86 JIT was using a limit of 32, just fix the wrong comments and
limit to 33 tail calls as the constant MAX_TAIL_CALL_CNT updated. For the
mips64 JIT, use "ori" instead of "addiu" as suggested by Johan Almbladh.
For the riscv JIT, use RV_REG_TCC directly to save one register move as
suggested by Björn Töpel. For the other implementations, no function changes,
it does not change the current limit 33, the new value of MAX_TAIL_CALL_CNT
can reflect the actual max tail call count, the related tail call testcases
in test_bpf module 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@loongson.cn>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Tested-by: Johan Almbladh <johan.almbladh@anyfinetworks.com>
Tested-by: Ilya Leoshkevich <iii@linux.ibm.com>
Acked-by: Björn Töpel <bjorn@kernel.org>
Acked-by: Johan Almbladh <johan.almbladh@anyfinetworks.com>
Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
Link: https://lore.kernel.org/bpf/1636075800-3264-1-git-send-email-yangtiezhu@loongson.cn
17 files changed:
arch/arm/net/bpf_jit_32.c
arch/arm64/net/bpf_jit_comp.c
arch/mips/net/bpf_jit_comp32.c
arch/mips/net/bpf_jit_comp64.c
arch/powerpc/net/bpf_jit_comp32.c
arch/powerpc/net/bpf_jit_comp64.c
arch/riscv/net/bpf_jit_comp32.c
arch/riscv/net/bpf_jit_comp64.c
arch/s390/net/bpf_jit_comp.c
arch/sparc/net/bpf_jit_comp_64.c
arch/x86/net/bpf_jit_comp.c
arch/x86/net/bpf_jit_comp32.c
include/linux/bpf.h
include/uapi/linux/bpf.h
kernel/bpf/core.c
lib/test_bpf.c
tools/include/uapi/linux/bpf.h

index eeb6dc0ecf463960fa482c51f6178c04b33d1220..e59b41e9ab0c1c57691386b532fe7488fb3bee70 100644 (file)
@@ -1199,7 +1199,8 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
 
        /* tmp2[0] = array, tmp2[1] = index */
 
-       /* if (tail_call_cnt > MAX_TAIL_CALL_CNT)
+       /*
+        * if (tail_call_cnt >= MAX_TAIL_CALL_CNT)
         *      goto out;
         * tail_call_cnt++;
         */
@@ -1208,7 +1209,7 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
        tc = arm_bpf_get_reg64(tcc, tmp, ctx);
        emit(ARM_CMP_I(tc[0], hi), ctx);
        _emit(ARM_COND_EQ, ARM_CMP_I(tc[1], lo), ctx);
-       _emit(ARM_COND_HI, ARM_B(jmp_offset), ctx);
+       _emit(ARM_COND_CS, ARM_B(jmp_offset), ctx);
        emit(ARM_ADDS_I(tc[1], tc[1], 1), ctx);
        emit(ARM_ADC_I(tc[0], tc[0], 0), ctx);
        arm_bpf_put_reg64(tcc, tmp, ctx);
index 86c9dc0681cce2fda9abdb80f17e1f8b8f187712..07c12c42b7517a258959856e54f53cbd59dbe9a3 100644 (file)
@@ -287,13 +287,14 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
        emit(A64_CMP(0, r3, tmp), ctx);
        emit(A64_B_(A64_COND_CS, jmp_offset), ctx);
 
-       /* if (tail_call_cnt > MAX_TAIL_CALL_CNT)
+       /*
+        * if (tail_call_cnt >= MAX_TAIL_CALL_CNT)
         *     goto out;
         * tail_call_cnt++;
         */
        emit_a64_mov_i64(tmp, MAX_TAIL_CALL_CNT, ctx);
        emit(A64_CMP(1, tcc, tmp), ctx);
-       emit(A64_B_(A64_COND_HI, jmp_offset), ctx);
+       emit(A64_B_(A64_COND_CS, jmp_offset), ctx);
        emit(A64_ADD_I(1, tcc, tcc, 1), ctx);
 
        /* prog = array->ptrs[index];
index bd996ede12f8e1cd8ccfe83aa924b7d9ec838b1e..044b11b65bcac8a076187873a7542ff64a59a037 100644 (file)
@@ -1381,8 +1381,7 @@ void build_prologue(struct jit_context *ctx)
         * 16-byte area in the parent's stack frame. On a tail call, the
         * calling function jumps into the prologue after these instructions.
         */
-       emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO,
-            min(MAX_TAIL_CALL_CNT + 1, 0xffff));
+       emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));
        emit(ctx, sw, MIPS_R_T9, 0, MIPS_R_SP);
 
        /*
index 815ade7242278a62abc6f75f567d1cd4e3d89622..6475828ffb36d5544e3457ed670a35b8e83734b9 100644 (file)
@@ -552,7 +552,7 @@ void build_prologue(struct jit_context *ctx)
         * On a tail call, the calling function jumps into the prologue
         * after this instruction.
         */
-       emit(ctx, addiu, tc, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT + 1, 0xffff));
+       emit(ctx, ori, tc, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));
 
        /* === Entry-point for tail calls === */
 
index 0da31d41d4131068fa5543cbe0dbdd30051f13fb..8a4faa05f9e41e73de0430797c30051a5eff2eb8 100644 (file)
@@ -221,13 +221,13 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
        PPC_BCC(COND_GE, out);
 
        /*
-        * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
+        * if (tail_call_cnt >= MAX_TAIL_CALL_CNT)
         *   goto out;
         */
        EMIT(PPC_RAW_CMPLWI(_R0, MAX_TAIL_CALL_CNT));
        /* tail_call_cnt++; */
        EMIT(PPC_RAW_ADDIC(_R0, _R0, 1));
-       PPC_BCC(COND_GT, out);
+       PPC_BCC(COND_GE, out);
 
        /* prog = array->ptrs[index]; */
        EMIT(PPC_RAW_RLWINM(_R3, b2p_index, 2, 0, 29));
index 8b5157ccfebae55e9a6e0e2e42a05497d661be38..8571aafcc9e1ea9ef12da0bea64cad7be54ebf7d 100644 (file)
@@ -228,12 +228,12 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
        PPC_BCC(COND_GE, out);
 
        /*
-        * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
+        * if (tail_call_cnt >= MAX_TAIL_CALL_CNT)
         *   goto out;
         */
        PPC_BPF_LL(b2p[TMP_REG_1], 1, bpf_jit_stack_tailcallcnt(ctx));
        EMIT(PPC_RAW_CMPLWI(b2p[TMP_REG_1], MAX_TAIL_CALL_CNT));
-       PPC_BCC(COND_GT, out);
+       PPC_BCC(COND_GE, out);
 
        /*
         * tail_call_cnt++;
index e6497424cbf60b07e9d844d66bc2ac0e46453eb7..529a83b85c1c934791ed8d18e79b9527c73b83fc 100644 (file)
@@ -799,11 +799,10 @@ static int emit_bpf_tail_call(int insn, struct rv_jit_context *ctx)
        emit_bcc(BPF_JGE, lo(idx_reg), RV_REG_T1, off, ctx);
 
        /*
-        * temp_tcc = tcc - 1;
-        * if (tcc < 0)
+        * if (--tcc < 0)
         *   goto out;
         */
-       emit(rv_addi(RV_REG_T1, RV_REG_TCC, -1), ctx);
+       emit(rv_addi(RV_REG_TCC, RV_REG_TCC, -1), ctx);
        off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
        emit_bcc(BPF_JSLT, RV_REG_TCC, RV_REG_ZERO, off, ctx);
 
@@ -829,7 +828,6 @@ static int emit_bpf_tail_call(int insn, struct rv_jit_context *ctx)
        if (is_12b_check(off, insn))
                return -1;
        emit(rv_lw(RV_REG_T0, off, RV_REG_T0), ctx);
-       emit(rv_addi(RV_REG_TCC, RV_REG_T1, 0), ctx);
        /* Epilogue jumps to *(t0 + 4). */
        __build_epilogue(true, ctx);
        return 0;
index f2a779c7e225deaefdc16a16575ec971b85ceb40..603630b6f3c5bd699a9a40095f1236ec40d189c6 100644 (file)
@@ -327,12 +327,12 @@ static int emit_bpf_tail_call(int insn, struct rv_jit_context *ctx)
        off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
        emit_branch(BPF_JGE, RV_REG_A2, RV_REG_T1, off, ctx);
 
-       /* if (TCC-- < 0)
+       /* if (--TCC < 0)
         *     goto out;
         */
-       emit_addi(RV_REG_T1, tcc, -1, ctx);
+       emit_addi(RV_REG_TCC, tcc, -1, ctx);
        off = ninsns_rvoff(tc_ninsn - (ctx->ninsns - start_insn));
-       emit_branch(BPF_JSLT, tcc, RV_REG_ZERO, off, ctx);
+       emit_branch(BPF_JSLT, RV_REG_TCC, RV_REG_ZERO, off, ctx);
 
        /* prog = array->ptrs[index];
         * if (!prog)
@@ -352,7 +352,6 @@ static int emit_bpf_tail_call(int insn, struct rv_jit_context *ctx)
        if (is_12b_check(off, insn))
                return -1;
        emit_ld(RV_REG_T3, off, RV_REG_T2, ctx);
-       emit_mv(RV_REG_TCC, RV_REG_T1, ctx);
        __build_epilogue(true, ctx);
        return 0;
 }
index 233cc9bcd6527b51b2bfa74494ed5dcae9085a41..9ff2bd83aad70c4cb79e063d734552c09bd35a9c 100644 (file)
@@ -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);
 
                /*
index 9a2f20cbd48b7c1c9ecf6c0e054c3e16bd1a7381..0bfe1c72a0c9e8b192ca82544fa779ae6025d244 100644 (file)
@@ -867,7 +867,7 @@ static void emit_tail_call(struct jit_ctx *ctx)
        emit(LD32 | IMMED | RS1(SP) | S13(off) | RD(tmp), ctx);
        emit_cmpi(tmp, MAX_TAIL_CALL_CNT, ctx);
 #define OFFSET2 13
-       emit_branch(BGU, ctx->idx, ctx->idx + OFFSET2, ctx);
+       emit_branch(BGEU, ctx->idx, ctx->idx + OFFSET2, ctx);
        emit_nop(ctx);
 
        emit_alu_K(ADD, tmp, 1, ctx);
index 726700fabca6d4f263b3b45f659a736e4c7013e4..6318479077867d44bf8459356817388305044bdc 100644 (file)
@@ -412,7 +412,7 @@ static void emit_indirect_jump(u8 **pprog, int reg, u8 *ip)
  * ... 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)
@@ -446,14 +446,14 @@ static void emit_bpf_tail_call_indirect(u8 **pprog, bool *callee_regs_used,
        EMIT2(X86_JBE, offset);                   /* jbe out */
 
        /*
-        * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
+        * if (tail_call_cnt++ >= MAX_TAIL_CALL_CNT)
         *      goto out;
         */
        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 */
 
        offset = ctx->tail_call_indirect_label - (prog + 2 - start);
-       EMIT2(X86_JA, offset);                    /* ja out */
+       EMIT2(X86_JAE, offset);                   /* jae out */
        EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
        EMIT2_off32(0x89, 0x85, tcc_off);         /* mov dword ptr [rbp - tcc_off], eax */
 
@@ -504,14 +504,14 @@ static void emit_bpf_tail_call_direct(struct bpf_jit_poke_descriptor *poke,
        int offset;
 
        /*
-        * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
+        * if (tail_call_cnt++ >= MAX_TAIL_CALL_CNT)
         *      goto out;
         */
        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 */
 
        offset = ctx->tail_call_direct_label - (prog + 2 - start);
-       EMIT2(X86_JA, offset);                        /* ja out */
+       EMIT2(X86_JAE, offset);                       /* jae out */
        EMIT3(0x83, 0xC0, 0x01);                      /* add eax, 1 */
        EMIT2_off32(0x89, 0x85, tcc_off);             /* mov dword ptr [rbp - tcc_off], eax */
 
index da9b7cfa46329773a0d095df4721a7860b98af37..429a89c5468b5748c01728193d7eeb82fadf2376 100644 (file)
@@ -1323,7 +1323,7 @@ static void emit_bpf_tail_call(u8 **pprog, u8 *ip)
        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;
@@ -1337,7 +1337,7 @@ static void emit_bpf_tail_call(u8 **pprog, u8 *ip)
        /* 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 */
index 56098c866704b7abd4374e28185218f42f2ce65e..cc7a0c36e7df18f1d826948e0d0c24134b117690 100644 (file)
@@ -1081,7 +1081,7 @@ struct bpf_array {
 };
 
 #define BPF_COMPLEXITY_LIMIT_INSNS      1000000 /* yes. 1M insns */
-#define MAX_TAIL_CALL_CNT 32
+#define MAX_TAIL_CALL_CNT 33
 
 #define BPF_F_ACCESS_MASK      (BPF_F_RDONLY |         \
                                 BPF_F_RDONLY_PROG |    \
index 6297eafdc40fa412e7f9e0a657cee24d5ba4c8e7..a69e4b04ffeb074f13c2751ff29702fcb5aae4f0 100644 (file)
@@ -1744,7 +1744,7 @@ union bpf_attr {
  *             if the maximum number of tail calls has been reached for this
  *             chain of programs. This limit is defined in the kernel by the
  *             macro **MAX_TAIL_CALL_CNT** (not accessible to user space),
- *             which is currently set to 32.
+ *             which is currently set to 33.
  *     Return
  *             0 on success, or a negative error in case of failure.
  *
index 2405e39d800fe5331e04750b665145d5dffb7120..b52dc845ecea35057fdbc230085bb3fe7d8cfcfd 100644 (file)
@@ -1574,7 +1574,8 @@ select_insn:
 
                if (unlikely(index >= array->map.max_entries))
                        goto out;
-               if (unlikely(tail_call_cnt > MAX_TAIL_CALL_CNT))
+
+               if (unlikely(tail_call_cnt >= MAX_TAIL_CALL_CNT))
                        goto out;
 
                tail_call_cnt++;
index adae39567264f27f431dae1d8863fd53c7255512..0c5cb2d6436a4772994e3cf75ee60e88e2d2a5ef 100644 (file)
@@ -14683,7 +14683,7 @@ static struct tail_call_test tail_call_tests[] = {
                        BPF_EXIT_INSN(),
                },
                .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE,
-               .result = (MAX_TAIL_CALL_CNT + 1 + 1) * MAX_TESTRUNS,
+               .result = (MAX_TAIL_CALL_CNT + 1) * MAX_TESTRUNS,
        },
        {
                "Tail call count preserved across function calls",
@@ -14705,7 +14705,7 @@ static struct tail_call_test tail_call_tests[] = {
                },
                .stack_depth = 8,
                .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE,
-               .result = (MAX_TAIL_CALL_CNT + 1 + 1) * MAX_TESTRUNS,
+               .result = (MAX_TAIL_CALL_CNT + 1) * MAX_TESTRUNS,
        },
        {
                "Tail call error path, NULL target",
index 6297eafdc40fa412e7f9e0a657cee24d5ba4c8e7..a69e4b04ffeb074f13c2751ff29702fcb5aae4f0 100644 (file)
@@ -1744,7 +1744,7 @@ union bpf_attr {
  *             if the maximum number of tail calls has been reached for this
  *             chain of programs. This limit is defined in the kernel by the
  *             macro **MAX_TAIL_CALL_CNT** (not accessible to user space),
- *             which is currently set to 32.
+ *             which is currently set to 33.
  *     Return
  *             0 on success, or a negative error in case of failure.
  *