Re: [PATCH v2 bpf-next 2/6] bpf: Use bpf_verifier_env buffers for reg_set_min_max

Mykyta Yatsenko
2026-03-23T15:33:42Z

Paul Chaignon <paul.chaignon@gmail.com> writes:

> In a subsequent patch, the regs_refine_cond_op and reg_bounds_sync
> functions will be called in is_branch_taken instead of reg_set_min_max,
> to simulate each branch's outcome. Since they will run before we branch
> out, these two functions will need to work on temporary registers for
> the two branches.
>
> This refactoring patch prepares for that change, by introducing the
> temporary registers on bpf_verifier_env and using them in
> reg_set_min_max.
>
> This change also allows us to save one fake_reg slot as we don't need to
> allocate an additional temporary buffer in case of a BPF_K condition.
>
> Finally, you may notice that this patch removes the check for
> "false_reg1 == false_reg2" in reg_set_min_max. That check was introduced
> in commit d43ad9da8052 ("bpf: Skip bounds adjustment for conditional
> jumps on same scalar register") to avoid an invariant violation. Given
> that "env->false_reg1 == env->false_reg2" doesn't make sense and
> invariant violations are addressed in a subsequent commit, this patch
> just removes the check.
>
> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
> Co-developed-by: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
> Signed-off-by: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
> Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> ---
> include/linux/bpf_verifier.h | 4 ++-
> kernel/bpf/verifier.c | 64 +++++++++++++-----------------------
> 2 files changed, 26 insertions(+), 42 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 090aa26d1c98..b129e0aaee20 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -837,7 +837,9 @@ struct bpf_verifier_env {
> u64 scratched_stack_slots;
> u64 prev_log_pos, prev_insn_print_pos;
> /* buffer used to temporary hold constants as scalar registers */
> - struct bpf_reg_state fake_reg[2];
> + struct bpf_reg_state fake_reg[1];
> + /* buffers used to save updated reg states while simulating branches */
> + struct bpf_reg_state true_reg1, true_reg2, false_reg1, false_reg2;
I can see Eduard suggested to store this in env to reduce stack usage by
the verifier. The rest of the refactoring looks like a correct
alignment. The only difference with the base version is removal of the
if (false_reg1 == false_reg2) condition, which is explained in the
commit message.
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
> /* buffer used to generate temporary string representations,
> * e.g., in reg_type_str() to generate reg_type string
> */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b638ab841c10..fbc29fb96a60 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17184,10 +17184,6 @@ static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state
> * but we don't support that right now.
> */
> static int reg_set_min_max(struct bpf_verifier_env *env,
> - struct bpf_reg_state *true_reg1,
> - struct bpf_reg_state *true_reg2,
> - struct bpf_reg_state *false_reg1,
> - struct bpf_reg_state *false_reg2,
> u8 opcode, bool is_jmp32)
> {
> int err;
> @@ -17196,30 +17192,23 @@ static int reg_set_min_max(struct bpf_verifier_env *env,
> * variable offset from the compare (unless they were a pointer into
> * the same object, but we don't bother with that).
> */
> - if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE)
> - return 0;
> -
> - /* We compute branch direction for same SCALAR_VALUE registers in
> - * is_scalar_branch_taken(). For unknown branch directions (e.g., BPF_JSET)
> - * on the same registers, we don't need to adjust the min/max values.
> - */
> - if (false_reg1 == false_reg2)
> + if (env->false_reg1.type != SCALAR_VALUE || env->false_reg2.type != SCALAR_VALUE)
> return 0;
>
> /* fallthrough (FALSE) branch */
> - regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32);
> - reg_bounds_sync(false_reg1);
> - reg_bounds_sync(false_reg2);
> + regs_refine_cond_op(&env->false_reg1, &env->false_reg2, rev_opcode(opcode), is_jmp32);
> + reg_bounds_sync(&env->false_reg1);
> + reg_bounds_sync(&env->false_reg2);
>
> /* jump (TRUE) branch */
> - regs_refine_cond_op(true_reg1, true_reg2, opcode, is_jmp32);
> - reg_bounds_sync(true_reg1);
> - reg_bounds_sync(true_reg2);
> -
> - err = reg_bounds_sanity_check(env, true_reg1, "true_reg1");
> - err = err ?: reg_bounds_sanity_check(env, true_reg2, "true_reg2");
> - err = err ?: reg_bounds_sanity_check(env, false_reg1, "false_reg1");
> - err = err ?: reg_bounds_sanity_check(env, false_reg2, "false_reg2");
> + regs_refine_cond_op(&env->true_reg1, &env->true_reg2, opcode, is_jmp32);
> + reg_bounds_sync(&env->true_reg1);
> + reg_bounds_sync(&env->true_reg2);
> +
> + err = reg_bounds_sanity_check(env, &env->true_reg1, "true_reg1");
> + err = err ?: reg_bounds_sanity_check(env, &env->true_reg2, "true_reg2");
> + err = err ?: reg_bounds_sanity_check(env, &env->false_reg1, "false_reg1");
> + err = err ?: reg_bounds_sanity_check(env, &env->false_reg2, "false_reg2");
> return err;
> }
>
> @@ -17597,6 +17586,10 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
> }
>
> is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32;
> + copy_register_state(&env->false_reg1, dst_reg);
> + copy_register_state(&env->false_reg2, src_reg);
> + copy_register_state(&env->true_reg1, dst_reg);
> + copy_register_state(&env->true_reg2, src_reg);
> pred = is_branch_taken(dst_reg, src_reg, opcode, is_jmp32);
> if (pred >= 0) {
> /* If we get here with a dst_reg pointer type it is because
> @@ -17661,27 +17654,16 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
> return PTR_ERR(other_branch);
> other_branch_regs = other_branch->frame[other_branch->curframe]->regs;
>
> - if (BPF_SRC(insn->code) == BPF_X) {
> - err = reg_set_min_max(env,
> - &other_branch_regs[insn->dst_reg],
> - &other_branch_regs[insn->src_reg],
> - dst_reg, src_reg, opcode, is_jmp32);
> - } else /* BPF_SRC(insn->code) == BPF_K */ {
> - /* reg_set_min_max() can mangle the fake_reg. Make a copy
> - * so that these are two different memory locations. The
> - * src_reg is not used beyond here in context of K.
> - */
> - memcpy(&env->fake_reg[1], &env->fake_reg[0],
> - sizeof(env->fake_reg[0]));
> - err = reg_set_min_max(env,
> - &other_branch_regs[insn->dst_reg],
> - &env->fake_reg[0],
> - dst_reg, &env->fake_reg[1],
> - opcode, is_jmp32);
> - }
> + err = reg_set_min_max(env, opcode, is_jmp32);
> if (err)
> return err;
>
> + copy_register_state(dst_reg, &env->false_reg1);
> + copy_register_state(src_reg, &env->false_reg2);
> + copy_register_state(&other_branch_regs[insn->dst_reg], &env->true_reg1);
> + if (BPF_SRC(insn->code) == BPF_X)
> + copy_register_state(&other_branch_regs[insn->src_reg], &env->true_reg2);
> +
> if (BPF_SRC(insn->code) == BPF_X &&
> src_reg->type == SCALAR_VALUE && src_reg->id &&
> !WARN_ON_ONCE(src_reg->id != other_branch_regs[insn->src_reg].id)) {
> --
> 2.43.0