Re: [PATCH v2 bpf-next 4/6] bpf: Simulate branches to prune based on range violations

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com> writes:

> On Mon, Mar 23, 2026 at 12:19 PM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>>
>> Paul Chaignon <paul.chaignon@gmail.com> writes:
>>
>> > From: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
>> >
> [...]
>> > +static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2,
>> > +                             u8 opcode, bool is_jmp32);
>> > +static u8 rev_opcode(u8 opcode);
>> > +
>> > +/* Learn more information about live branches by simulating both branches being
>> > + * taken using regs_refine_cond_op. Because regs_refine_cond_op is sound when
>> > + * the branch is taken, if it produces ill-formed register bounds, it must mean
>> > + * that the branch is dead.
>> > + */
>> Sorry for being nit-picky, could you please use kernel style comment style +
>
> Thanks for the suggestion. To clarify, by kernel-style did you mean
> keeping the beginning line empty?
> Or something else that I might have missed.
Yes, first line just /*, then text goes next line.
>
>> maybe reword it a little bit, instead of:
>>
>> /*
>>  * Because regs_refine_cond_op is sound when
>>  * the branch is taken, if it produces ill-formed register bounds, it must mean
>>  * that the branch is dead.
>>  */
>> Something like:
>> /*
>>  * regs_refine_cond_op() is sound, so producing ill-formed register
>>  * bounds for the branch means that branch is dead.
>>  */
>>
> This sounds good.
>
>> > +static int simulate_both_branches_taken(struct bpf_verifier_env *env, u8 opcode, bool is_jmp32)
>> > +{
>> > +     /* Fallthrough (FALSE) branch */
>> > +     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);
>> This is probably more related to patch 2: is it necessary to have both
>> true/false_reg1/2 pairs, it looks like we process false branch before
>> the true branch and they never intersect, don't they? So it worth
>> removing a pair of bpf_reg_states from env?
>
> We do process the false branch before the true branch in
> is_branch_taken->simulate_both_branches. But when both branches are possible
> (we return -1), we need all the four env buffers to hold the updated results
> of both branch simulations simultaneously.
>
> The new verifier state for other_branch hasn't been created at the time
> simulate_both_branches is called, so we cannot copy the results to their
> final destinations immediately. We must hold them in the buffers until
> after push_stack,
> after which we can copy them back into their corresponding register states:
>
> 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);
>
> [...]