Re: [PATCH v4 bpf-next 1/2] bpf: properly verify tail call behavior

Eduard Zingerman
2025-11-10T20:28:29Z

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Mon, 2025-11-10 at 16:18 +0100, Martin Teichmann wrote:

[...]

> diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
> index a7240013fd9d..54f4772d990c 100644
> --- a/kernel/bpf/liveness.c
> +++ b/kernel/bpf/liveness.c
> @@ -500,6 +500,10 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx)
> if (opcode_info->can_jump)
> succ->items[succ->cnt++] = idx + bpf_jmp_offset(insn) + 1;
>
> + if (unlikely(insn->code == (BPF_JMP | BPF_CALL) && insn->src_reg == 0
> + && insn->imm == BPF_FUNC_tail_call))
> + succ->items[succ->cnt++] = idx;
> +
> return succ;
> }
>

Hi Martin,

This is a clever hack and I like it, but let's not do that.
It is going to be a footgun if e.g. someone would use
bpf_insn_successors() to build intra-procedural CFG.
Instead, please allocate a jt object for tail calls as in the diff
attached (on top of your patch-set). Please also extend
compute_live_registers.c to cover this logic.

Other than that, I think that patch logic and tests are fine.

Thanks,
Eduard

[...]

[-- Attachment #2: tail-call-jt.patch --]
[-- Type: text/x-patch, Size: 4484 bytes --]

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 5441341f1ab9..8d0b60fa5f2b 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -527,7 +527,6 @@ struct bpf_insn_aux_data {
struct {
u32 map_index; /* index into used_maps[] */
u32 map_off; /* offset from value base address */
- struct bpf_iarray *jt; /* jump table for gotox instruction */
};
struct {
enum bpf_reg_type reg_type; /* type of pseudo_btf_id */
@@ -550,6 +549,7 @@ struct bpf_insn_aux_data {
/* remember the offset of node field within type to rewrite */
u64 insert_off;
};
+ struct bpf_iarray *jt; /* jump table for gotox or bpf_tailcall call instruction */
struct btf_struct_meta *kptr_struct_meta;
u64 map_key_state; /* constant (32 bit) key tracking for maps */
int ctx_field_size; /* the ctx field size for load insn, maybe 0 */
@@ -652,6 +652,7 @@ struct bpf_subprog_info {
u32 start; /* insn idx of function entry point */
u32 linfo_idx; /* The idx to the main_prog->aux->linfo */
u32 postorder_start; /* The idx to the env->cfg.insn_postorder */
+ u32 exit_idx; /* Index of one of the BPF_EXIT instructions in this subprogram */
u16 stack_depth; /* max. stack depth used by this function */
u16 stack_extra;
/* offsets in range [stack_depth .. fastcall_stack_off)
@@ -669,9 +670,9 @@ struct bpf_subprog_info {
bool keep_fastcall_stack: 1;
bool changes_pkt_data: 1;
bool might_sleep: 1;
+ u8 arg_cnt:3;

enum priv_stack_mode priv_stack_mode;
- u8 arg_cnt;
struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
};

diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
index 54f4772d990c..60db5d655495 100644
--- a/kernel/bpf/liveness.c
+++ b/kernel/bpf/liveness.c
@@ -482,11 +482,12 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx)
struct bpf_prog *prog = env->prog;
struct bpf_insn *insn = &prog->insnsi[idx];
const struct opcode_info *opcode_info;
- struct bpf_iarray *succ;
+ struct bpf_iarray *succ, *jt;
int insn_sz;

- if (unlikely(insn_is_gotox(insn)))
- return env->insn_aux_data[idx].jt;
+ jt = env->insn_aux_data[idx].jt;
+ if (unlikely(jt))
+ return jt;

/* pre-allocated array of size up to 2; reset cnt, as it may have been used already */
succ = env->succ;
@@ -500,10 +501,6 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx)
if (opcode_info->can_jump)
succ->items[succ->cnt++] = idx + bpf_jmp_offset(insn) + 1;

- if (unlikely(insn->code == (BPF_JMP | BPF_CALL) && insn->src_reg == 0
- && insn->imm == BPF_FUNC_tail_call))
- succ->items[succ->cnt++] = idx;
-
return succ;
}

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7a817777fbb3..dc129a59718b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3528,8 +3528,12 @@ static int check_subprogs(struct bpf_verifier_env *env)
subprog[cur_subprog].has_ld_abs = true;
if (BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32)
goto next;
- if (BPF_OP(code) == BPF_EXIT || BPF_OP(code) == BPF_CALL)
+ if (BPF_OP(code) == BPF_CALL)
goto next;
+ if (BPF_OP(code) == BPF_EXIT) {
+ subprog[cur_subprog].exit_idx = i;
+ goto next;
+ }
off = i + bpf_jmp_offset(&insn[i]) + 1;
if (off < subprog_start || off >= subprog_end) {
verbose(env, "jump out of range from insn %d to %d\n", i, off);
@@ -18120,6 +18124,25 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env)
return keep_exploring ? KEEP_EXPLORING : DONE_EXPLORING;
}

+static int visit_tailcall_insn(struct bpf_verifier_env *env, int t)
+{
+ static struct bpf_subprog_info *subprog;
+ struct bpf_iarray *jt;
+
+ if (env->insn_aux_data[t].jt)
+ return 0;
+
+ jt = iarray_realloc(NULL, 2);
+ if (!jt)
+ return -ENOMEM;
+
+ subprog = bpf_find_containing_subprog(env, t);
+ jt->items[0] = t + 1;
+ jt->items[1] = subprog->exit_idx;
+ env->insn_aux_data[t].jt = jt;
+ return 0;
+}
+
/* Visits the instruction at index t and returns one of the following:
* < 0 - an error occurred
* DONE_EXPLORING - the instruction was fully explored
@@ -18180,6 +18203,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
mark_subprog_might_sleep(env, t);
if (bpf_helper_changes_pkt_data(insn->imm))
mark_subprog_changes_pkt_data(env, t);
+ if (insn->imm == BPF_FUNC_tail_call)
+ visit_tailcall_insn(env, t);
} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
struct bpf_kfunc_call_arg_meta meta;