Re: [PATCH bpf-next v3 02/11] bpf: Move insn if/else into do_check_insn()
Eduard Zingerman writes: > On Thu, 2025-05-01 at 09:35 +0200, Luis Gerhorst wrote: > >> +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. > >> +return -EINVAL; >> +} >> +process_bpf_exit_full: > > Nit: since we are refactoring I'd extract this as a function instead of goto. Both done, thanks again for the review and testing!
Re: [PATCH bpf-next v3 02/11] bpf: Move insn if/else into do_check_insn()
On Thu, 1 May 2025 at 09:43, 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/[email protected]/ > > Signed-off-by: Luis Gerhorst > Acked-by: Henriette Herzog > Cc: Maximilian Ott > Cc: Milan Stephan > --- Acked-by: Kumar Kartikeya Dwivedi
Re: [PATCH bpf-next v3 02/11] bpf: Move insn if/else into do_check_insn()
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/[email protected]/ > > Signed-off-by: Luis Gerhorst > Acked-by: Henriette Herzog > Cc: Maximilian Ott > Cc: Milan Stephan > --- 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; [...]
[PATCH bpf-next v3 02/11] bpf: Move insn if/else into do_check_insn()
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/[email protected]/ Signed-off-by: Luis Gerhorst Acked-by: Henriette Herzog Cc: Maximilian Ott Cc: Milan Stephan --- kernel/bpf/verifier.c | 426 ++ 1 file changed, 220 insertions(+), 206 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 54c6953a8b84..73475d7d7eca 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -19389,20 +19389,219 @@ static int save_aux_ptr_type(struct bpf_verifier_env *env, enum bpf_reg_type typ return 0; } +enum { + PROCESS_BPF_EXIT = 1 +}; + +static int do_check_insn(struct bpf_verifier_env *env, struct bpf_insn *insn, +bool *do_print_state) +{ + int err; + u8 class = BPF_CLASS(insn->code); + bool exception_exit = false; + + if (class == BPF_ALU || class == BPF_ALU64) { + err = check_alu_op(env, insn); + if (err) + return err; + + } else if (class == BPF_LDX) { + bool is_ldsx = BPF_MODE(insn->code) == BPF_MEMSX; + + /* Check for reserved fields is already done in +* resolve_pseudo_ldimm64(). +*/ + err = check_load_mem(env, insn, false, is_ldsx, true, "ldx"); + if (err) + return err; + } else if (class == BPF_STX) { + if (BPF_MODE(insn->code) == BPF_ATOMIC) { + err = check_atomic(env, insn); + if (err) + return err; + env->insn_idx++; + return 0; + } + + if (BPF_MODE(insn->code) != BPF_MEM || insn->imm != 0) { + verbose(env, "BPF_STX uses reserved fields\n"); + return -EINVAL; + } + + err = check_store_reg(env, insn, false); + if (err) + return err; + } 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; + + /* 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) { + u8 opcode = BPF_OP(insn->code); + + env->jmps_processed++; + if (opcode == BPF_CALL) { + if (BPF_SRC(insn->code) != BPF_K || + (insn->src_reg != BPF_PSEUDO_KFUNC_CALL && +insn->off != 0) || + (insn->src_reg != BPF_REG_0 && +insn->src_reg != BPF_PSEUDO_CALL && +insn->src_reg != BPF_PSEUDO_KFUNC_CALL) || + insn->dst_reg != BPF_REG_0 || class == BPF_JMP32) { + verbose(env, "BPF_CALL uses reserved fields\n"); + return -EINVAL; + } + + if (env->cur_state->active_locks) { + if ((insn->src_reg == BPF_REG_0 && +insn->imm != BPF_FUNC_spin_unlock) || + (insn->src_reg == BPF_PSEUDO_KFUNC_CALL && +(insn->off != 0 || !kfunc_spin_allowed(insn->imm { + verbose(env, + "function calls are not allowed
