Re: [PATCH bpf-next v3 02/11] bpf: Move insn if/else into do_check_insn()

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-05-01 at 09:35 +0200, Luis Gerhorst wrote:
> This is required to catch the errors later and fall back to a nospec if
> on a speculative path.
> 
> Eliminate the regs variable as it is only used once and insn_idx is not
> modified in-between the definition and usage.
> 
> Still pass insn simply to match the other check_*() functions. As Eduard
> points out [1], insn is assumed to correspond to env->insn_idx in many
> places (e.g, __check_reg_arg()).
> 
> Move code into do_check_insn(), replace
> * "continue" with "return 0" after modifying insn_idx
> * "goto process_bpf_exit" with "return PROCESS_BPF_EXIT"
> * "do_print_state = " with "*do_print_state = "
> 
> [1] https://lore.kernel.org/all/293dbe3950a782b8eb3b87b71d7a967e120191fd.camel@gmail.com/
> 
> Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
> Acked-by: Henriette Herzog <henriette.herzog@rub.de>
> Cc: Maximilian Ott <ott@cs.fau.de>
> Cc: Milan Stephan <milan.stephan@fau.de>
> ---

Except two notes below, I think this patch looks good.
Thank you, this is a good refactoring.

[...]

> +static int do_check_insn(struct bpf_verifier_env *env, struct bpf_insn *insn,
> +			 bool *do_print_state)
> +{

[...]

> +	} else if (class == BPF_ST) {
> +		enum bpf_reg_type dst_reg_type;
> +
> +		if (BPF_MODE(insn->code) != BPF_MEM ||
> +		    insn->src_reg != BPF_REG_0) {
> +			verbose(env, "BPF_ST uses reserved fields\n");
> +			return -EINVAL;
> +		}
> +		/* check src operand */
> +		err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> +		if (err)
> +			return err;
> +
> +		dst_reg_type = cur_regs(env)[insn->dst_reg].type;

Implicitly relying on `insn == &env->prog->insnsi[env->cur_idx]`
is weird. Still think that `insn` parameter should be dropped and
computed inside this function instead.

> +
> +		/* check that memory (dst_reg + off) is writeable */
> +		err = check_mem_access(env, env->insn_idx, insn->dst_reg,
> +				       insn->off, BPF_SIZE(insn->code),
> +				       BPF_WRITE, -1, false, false);
> +		if (err)
> +			return err;
> +
> +		err = save_aux_ptr_type(env, dst_reg_type, false);
> +		if (err)
> +			return err;
> +	} else if (class == BPF_JMP || class == BPF_JMP32) {

[...]

> +		} else if (opcode == BPF_EXIT) {
> +			if (BPF_SRC(insn->code) != BPF_K ||
> +			    insn->imm != 0 ||
> +			    insn->src_reg != BPF_REG_0 ||
> +			    insn->dst_reg != BPF_REG_0 ||
> +			    class == BPF_JMP32) {
> +				verbose(env, "BPF_EXIT uses reserved fields\n");
> +				return -EINVAL;
> +			}
> +process_bpf_exit_full:

Nit: since we are refactoring I'd extract this as a function instead of goto.

> +			/* We must do check_reference_leak here before
> +			 * prepare_func_exit to handle the case when
> +			 * state->curframe > 0, it may be a callback function,
> +			 * for which reference_state must match caller reference
> +			 * state when it exits.
> +			 */
> +			err = check_resource_leak(env, exception_exit, !env->cur_state->curframe,
> +						  "BPF_EXIT instruction in main prog");
> +			if (err)
> +				return err;
> +
> +			/* The side effect of the prepare_func_exit which is
> +			 * being skipped is that it frees bpf_func_state.
> +			 * Typically, process_bpf_exit will only be hit with
> +			 * outermost exit. copy_verifier_state in pop_stack will
> +			 * handle freeing of any extra bpf_func_state left over
> +			 * from not processing all nested function exits. We
> +			 * also skip return code checks as they are not needed
> +			 * for exceptional exits.
> +			 */
> +			if (exception_exit)
> +				return PROCESS_BPF_EXIT;
> +
> +			if (env->cur_state->curframe) {
> +				/* exit from nested function */
> +				err = prepare_func_exit(env, &env->insn_idx);
> +				if (err)
> +					return err;
> +				*do_print_state = true;
> +				return 0;
> +			}
> +
> +			err = check_return_code(env, BPF_REG_0, "R0");
> +			if (err)
> +				return err;
> +			return PROCESS_BPF_EXIT;

[...]