On 2/14/23 2:05 PM, Dave Marchevsky wrote: > Currently the ref_obj_id and OBJ_RELEASE searching is done in the code > that examines each individual arg (check_func_arg for helpers and > check_kfunc_args inner loop for kfuncs). This patch pulls out this > searching to occur before individual arg type handling, resulting in a > cleaner separation of logic and shared logic between kfuncs and helpers. > > The logic for this searching is already very similar between kfuncs and > helpers: > > Kfuncs: > * Function-level KF_RELEASE flag indicates that the kfunc releases > some previously-acquired arg > * Verifier searches through arg regs to find those with ref_obj_id set > * One such arg reg is selected. If multiple arg regs have ref_obj_id > set, the last one (by regno) is chosen to be released > > Helpers: > * OBJ_RELEASE is used in function proto to tag a particular arg as the > one that should be released > * There can only be one such tagged arg > * Verifier confirms that only one arg reg has ref_obj_id set and that > that reg matches expected OBJ_RELEASE arg > * If OBJ_RELEASE arg doesn't have a matching ref_obj_id arg reg, or > some other arg reg has ref_obj_id, it's an invalid state > > It's a long-term goal to merge as much kfunc and helper logic as > possible. Merging the similar functionality here is a small step in that > direction. > > Two new helper functions are added: > * args_find_ref_obj_id_regno > * For helpers and kfuncs. Searches through arg regs to find > ref_obj_id reg and returns its regno. > > * helper_proto_find_release_arg_regno > * For helpers only. Searches through fn proto args to find the > OBJ_RELEASE arg and returns the corresponding regno. > > The refactoring strives to keep failure logic and error messages > unchanged. However, because the release arg searching is now done before > any arg-specific type checking, verifier states that are invalid due to > both invalid release arg state _and_ some type- or helper-specific > checking logic might see the release arg-related error message first, > when previously verification would fail for the other reason. > > Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx> > --- > v2 -> v3: Whoops, didn't include v2 link. Here it is: https://lore.kernel.org/bpf/20230131171038.2648165-1-davemarchevsky@xxxxxx/ > * Edit patch summary for clarity > * Correct err_multi comment in args_find_ref_obj_id_regno doc string > * Rebase onto latest bpf-next: 'Revert "bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25"' > > v1 -> v2: https://lore.kernel.org/bpf/20230121002417.1684602-1-davemarchevsky@xxxxxx/ > * Fix uninitialized variable complaint (kernel test bot) > * Add err_multi param to args_find_ref_obj_id_regno - kfunc arg reg > checking wasn't erroring if multiple ref_obj_id arg regs were found, > retain this behavior > > v0 -> v1: https://lore.kernel.org/bpf/20221217082506.1570898-2-davemarchevsky@xxxxxx/ > * Remove allow_multi from args_find_ref_obj_id_regno, no need to > support multiple ref_obj_id arg regs > * No need to use temp variable 'i' to count nargs (David) > * Proper formatting of function-level comments on newly-added helpers (David) > > kernel/bpf/verifier.c | 220 +++++++++++++++++++++++++++++------------- > 1 file changed, 153 insertions(+), 67 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 21e08c111702..c0d01085f44f 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6735,48 +6735,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > return err; > > skip_type_check: > - if (arg_type_is_release(arg_type)) { > - if (arg_type_is_dynptr(arg_type)) { > - struct bpf_func_state *state = func(env, reg); > - int spi; > - > - /* Only dynptr created on stack can be released, thus > - * the get_spi and stack state checks for spilled_ptr > - * should only be done before process_dynptr_func for > - * PTR_TO_STACK. > - */ > - if (reg->type == PTR_TO_STACK) { > - spi = dynptr_get_spi(env, reg); > - if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) { > - verbose(env, "arg %d is an unacquired reference\n", regno); > - return -EINVAL; > - } > - } else { > - verbose(env, "cannot release unowned const bpf_dynptr\n"); > - return -EINVAL; > - } > - } else if (!reg->ref_obj_id && !register_is_null(reg)) { > - verbose(env, "R%d must be referenced when passed to release function\n", > - regno); > - return -EINVAL; > - } > - if (meta->release_regno) { > - verbose(env, "verifier internal error: more than one release argument\n"); > - return -EFAULT; > - } > - meta->release_regno = regno; > - } > - > - if (reg->ref_obj_id) { > - if (meta->ref_obj_id) { > - verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n", > - regno, reg->ref_obj_id, > - meta->ref_obj_id); > - return -EFAULT; > - } > - meta->ref_obj_id = reg->ref_obj_id; > - } > - > switch (base_type(arg_type)) { > case ARG_CONST_MAP_PTR: > /* bpf_map_xxx(map_ptr) call: remember that map_ptr */ > @@ -6891,6 +6849,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > err = check_mem_size_reg(env, reg, regno, true, meta); > break; > case ARG_PTR_TO_DYNPTR: > + if (meta->release_regno == regno) { > + struct bpf_func_state *state = func(env, reg); > + int spi; > + > + /* Only dynptr created on stack can be released, thus > + * the get_spi and stack state checks for spilled_ptr > + * should only be done before process_dynptr_func for > + * PTR_TO_STACK. > + */ > + if (reg->type == PTR_TO_STACK) { > + spi = dynptr_get_spi(env, reg); > + if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) { > + verbose(env, "arg %d is an unacquired reference\n", regno); > + return -EINVAL; > + } > + } else { > + verbose(env, "cannot release unowned const bpf_dynptr\n"); > + return -EINVAL; > + } > + } > err = process_dynptr_func(env, regno, arg_type, meta); > if (err) > return err; > @@ -8104,10 +8082,95 @@ static void update_loop_inline_state(struct bpf_verifier_env *env, u32 subprogno > state->callback_subprogno == subprogno); > } > > +/** > + * args_find_ref_obj_id_regno() - Find regno that should become meta->ref_obj_id > + * @env: Verifier env > + * @regs: Regs to search for ref_obj_id > + * @nargs: Number of arg regs to search > + * @err_multi: Should this function error if multiple ref_obj_id args found > + * > + * Call arg meta's ref_obj_id is used to either: > + * * For release funcs, keep track of ref that needs to be released > + * * For other funcs, keep track of ref that needs to be propagated to retval > + * > + * Find the arg regno with nonzero ref_obj_id > + * > + * If err_multi is false and multiple ref_obj_id arg regs are seen, regno of the > + * last one is returned > + * > + * Return: > + * * On success, regno that should become meta->ref_obj_id (regno > 0 since > + * BPF_REG_1 is first arg > + * * 0 if no arg had ref_obj_id set > + * * -err if some invalid arg reg state > + */ > +static int args_find_ref_obj_id_regno(struct bpf_verifier_env *env, struct bpf_reg_state *regs, > + u32 nargs, bool err_multi) > +{ > + struct bpf_reg_state *reg; > + u32 i, regno, found_regno = 0; > + > + for (i = 0; i < nargs; i++) { > + regno = i + BPF_REG_1; > + reg = ®s[regno]; > + > + if (!reg->ref_obj_id) > + continue; > + > + if (found_regno && err_multi) { > + verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n", > + regno, reg->ref_obj_id, regs[found_regno].ref_obj_id); > + return -EFAULT; > + } > + > + found_regno = regno; > + } > + > + return found_regno; > +} > + > +/** > + * helper_proto_find_release_arg_regno() - Find OBJ_RELEASE arg in func proto > + * @env: Verifier env > + * @fn: Func proto to search for OBJ_RELEASE > + * @nargs: Number of arg specs to search > + * > + * For helpers, to determine which arg reg should be released, loop through > + * func proto arg specification to find arg with OBJ_RELEASE > + * > + * Return: > + * * On success, regno of single OBJ_RELEASE arg > + * * 0 if no arg in the proto was OBJ_RELEASE > + * * -err if some invalid func proto state > + */ > +static int helper_proto_find_release_arg_regno(struct bpf_verifier_env *env, > + const struct bpf_func_proto *fn, u32 nargs) > +{ > + enum bpf_arg_type arg_type; > + int i, release_regno = 0; > + > + for (i = 0; i < nargs; i++) { > + arg_type = fn->arg_type[i]; > + > + if (!arg_type_is_release(arg_type)) > + continue; > + > + if (release_regno) { > + verbose(env, "verifier internal error: more than one release argument\n"); > + return -EFAULT; > + } > + > + release_regno = i + BPF_REG_1; > + } > + > + return release_regno; > +} > + > static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > int *insn_idx_p) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > + int i, err, func_id, nargs, release_regno, ref_regno; > const struct bpf_func_proto *fn = NULL; > enum bpf_return_type ret_type; > enum bpf_type_flag ret_flag; > @@ -8115,7 +8178,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > struct bpf_call_arg_meta meta; > int insn_idx = *insn_idx_p; > bool changes_data; > - int i, err, func_id; > > /* find function prototype */ > func_id = insn->imm; > @@ -8179,8 +8241,37 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > } > > meta.func_id = func_id; > + regs = cur_regs(env); > + > + /* find actual arg count */ > + for (nargs = 0; nargs < MAX_BPF_FUNC_REG_ARGS; nargs++) > + if (fn->arg_type[nargs] == ARG_DONTCARE) > + break; > + > + release_regno = helper_proto_find_release_arg_regno(env, fn, nargs); > + if (release_regno < 0) > + return release_regno; > + > + ref_regno = args_find_ref_obj_id_regno(env, regs, nargs, true); > + if (ref_regno < 0) > + return ref_regno; > + else if (ref_regno > 0) > + meta.ref_obj_id = regs[ref_regno].ref_obj_id; > + > + if (release_regno > 0) { > + if (!regs[release_regno].ref_obj_id && > + !register_is_null(®s[release_regno]) && > + !arg_type_is_dynptr(fn->arg_type[release_regno - BPF_REG_1])) { > + verbose(env, "R%d must be referenced when passed to release function\n", > + release_regno); > + return -EINVAL; > + } > + > + meta.release_regno = release_regno; > + } > + > /* check args */ > - for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) { > + for (i = 0; i < nargs; i++) { > err = check_func_arg(env, i, &meta, fn); > if (err) > return err; > @@ -8204,8 +8295,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > return err; > } > > - regs = cur_regs(env); > - > /* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot > * be reinitialized by any dynptr helper. Hence, mark_stack_slots_dynptr > * is safe to do directly. > @@ -9442,10 +9531,11 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, > static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta) > { > const char *func_name = meta->func_name, *ref_tname; > + struct bpf_reg_state *regs = cur_regs(env); > const struct btf *btf = meta->btf; > const struct btf_param *args; > + int ret, ref_regno; > u32 i, nargs; > - int ret; > > args = (const struct btf_param *)(meta->func_proto + 1); > nargs = btf_type_vlen(meta->func_proto); > @@ -9455,17 +9545,31 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > return -EINVAL; > } > > + ref_regno = args_find_ref_obj_id_regno(env, cur_regs(env), nargs, false); > + if (ref_regno < 0) { > + return ref_regno; > + } else if (!ref_regno && is_kfunc_release(meta)) { > + verbose(env, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n", > + func_name); > + return -EINVAL; > + } > + > + meta->ref_obj_id = regs[ref_regno].ref_obj_id; > + if (is_kfunc_release(meta)) > + meta->release_regno = ref_regno; > + > /* Check that BTF function arguments match actual types that the > * verifier sees. > */ > for (i = 0; i < nargs; i++) { > - struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[i + 1]; > const struct btf_type *t, *ref_t, *resolve_ret; > enum bpf_arg_type arg_type = ARG_DONTCARE; > u32 regno = i + 1, ref_id, type_size; > bool is_ret_buf_sz = false; > + struct bpf_reg_state *reg; > int kf_arg_type; > > + reg = ®s[regno]; > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > > if (is_kfunc_arg_ignore(btf, &args[i])) > @@ -9528,18 +9632,6 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > return -EACCES; > } > > - if (reg->ref_obj_id) { > - if (is_kfunc_release(meta) && meta->ref_obj_id) { > - verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n", > - regno, reg->ref_obj_id, > - meta->ref_obj_id); > - return -EFAULT; > - } > - meta->ref_obj_id = reg->ref_obj_id; > - if (is_kfunc_release(meta)) > - meta->release_regno = regno; > - } > - > ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id); > ref_tname = btf_name_by_offset(btf, ref_t->name_off); > > @@ -9585,7 +9677,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > return -EFAULT; > } > > - if (is_kfunc_release(meta) && reg->ref_obj_id) > + if (is_kfunc_release(meta) && regno == meta->release_regno) > arg_type |= OBJ_RELEASE; > ret = check_func_arg_reg_off(env, reg, regno, arg_type); > if (ret < 0) > @@ -9747,12 +9839,6 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > } > } > > - if (is_kfunc_release(meta) && !meta->release_regno) { > - verbose(env, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n", > - func_name); > - return -EINVAL; > - } > - > return 0; > } >
