On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> This patch refactors the core logic of "btf_check_func_arg_match()"
> into a new function "do_btf_check_func_arg_match()".
> "do_btf_check_func_arg_match()" will be reused later to check
> the kernel function call.
>
> The "if (!btf_type_is_ptr(t))" is checked first to improve the indentation
> which will be useful for a later patch.
>
> Some of the "btf_kind_str[]" usages is replaced with the shortcut
> "btf_type_str(t)".
>
> Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
> ---
> include/linux/btf.h | 5 ++
> kernel/bpf/btf.c | 159 ++++++++++++++++++++++++--------------------
> 2 files changed, 91 insertions(+), 73 deletions(-)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 7fabf1428093..93bf2e5225f5 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -140,6 +140,11 @@ static inline bool btf_type_is_enum(const struct btf_type *t)
> return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM;
> }
>
> +static inline bool btf_type_is_scalar(const struct btf_type *t)
> +{
> + return btf_type_is_int(t) || btf_type_is_enum(t);
> +}
> +
> static inline bool btf_type_is_typedef(const struct btf_type *t)
> {
> return BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF;
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 96cd24020a38..529b94b601c6 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4381,7 +4381,7 @@ static u8 bpf_ctx_convert_map[] = {
> #undef BPF_LINK_TYPE
>
> static const struct btf_member *
> -btf_get_prog_ctx_type(struct bpf_verifier_log *log, struct btf *btf,
> +btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
> const struct btf_type *t, enum bpf_prog_type prog_type,
> int arg)
> {
> @@ -5366,122 +5366,135 @@ int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
> return btf_check_func_type_match(log, btf1, t1, btf2, t2);
> }
>
> -/* Compare BTF of a function with given bpf_reg_state.
> - * Returns:
> - * EFAULT - there is a verifier bug. Abort verification.
> - * EINVAL - there is a type mismatch or BTF is not available.
> - * 0 - BTF matches with what bpf_reg_state expects.
> - * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
> - */
> -int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> - struct bpf_reg_state *regs)
> +static int do_btf_check_func_arg_match(struct bpf_verifier_env *env,
do_btf_check_func_arg_match vs btf_check_func_arg_match distinction is
not clear at all. How about something like
btf_check_func_arg_match vs btf_check_subprog_arg_match (or btf_func
vs bpf_subprog). I think that highlights the main distinction better,
no?
> + const struct btf *btf, u32 func_id,
> + struct bpf_reg_state *regs,
> + bool ptr_to_mem_ok)
> {
> struct bpf_verifier_log *log = &env->log;
> - struct bpf_prog *prog = env->prog;
> - struct btf *btf = prog->aux->btf;
> - const struct btf_param *args;
> + const char *func_name, *ref_tname;
> const struct btf_type *t, *ref_t;
> - u32 i, nargs, btf_id, type_size;
> - const char *tname;
> - bool is_global;
> -
> - if (!prog->aux->func_info)
> - return -EINVAL;
> -
> - btf_id = prog->aux->func_info[subprog].type_id;
> - if (!btf_id)
> - return -EFAULT;
> -
> - if (prog->aux->func_info_aux[subprog].unreliable)
> - return -EINVAL;
> + const struct btf_param *args;
> + u32 i, nargs;
>
> - t = btf_type_by_id(btf, btf_id);
> + t = btf_type_by_id(btf, func_id);
> if (!t || !btf_type_is_func(t)) {
> /* These checks were already done by the verifier while loading
> * struct bpf_func_info
> */
> - bpf_log(log, "BTF of func#%d doesn't point to KIND_FUNC\n",
> - subprog);
> + bpf_log(log, "BTF of func_id %u doesn't point to KIND_FUNC\n",
> + func_id);
> return -EFAULT;
> }
> - tname = btf_name_by_offset(btf, t->name_off);
> + func_name = btf_name_by_offset(btf, t->name_off);
>
> t = btf_type_by_id(btf, t->type);
> if (!t || !btf_type_is_func_proto(t)) {
> - bpf_log(log, "Invalid BTF of func %s\n", tname);
> + bpf_log(log, "Invalid BTF of func %s\n", func_name);
> return -EFAULT;
> }
> args = (const struct btf_param *)(t + 1);
> nargs = btf_type_vlen(t);
> if (nargs > MAX_BPF_FUNC_REG_ARGS) {
> - bpf_log(log, "Function %s has %d > %d args\n", tname, nargs,
> + bpf_log(log, "Function %s has %d > %d args\n", func_name, nargs,
> MAX_BPF_FUNC_REG_ARGS);
> - goto out;
> + return -EINVAL;
> }
>
> - is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
> /* check that BTF function arguments match actual types that the
> * verifier sees.
> */
> for (i = 0; i < nargs; i++) {
> - struct bpf_reg_state *reg = ®s[i + 1];
> + u32 regno = i + 1;
> + struct bpf_reg_state *reg = ®s[regno];
>
> - t = btf_type_by_id(btf, args[i].type);
> - while (btf_type_is_modifier(t))
> - t = btf_type_by_id(btf, t->type);
> - if (btf_type_is_int(t) || btf_type_is_enum(t)) {
> + t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> + if (btf_type_is_scalar(t)) {
> if (reg->type == SCALAR_VALUE)
> continue;
> - bpf_log(log, "R%d is not a scalar\n", i + 1);
> - goto out;
> + bpf_log(log, "R%d is not a scalar\n", regno);
> + return -EINVAL;
> }
> - if (btf_type_is_ptr(t)) {
> +
> + if (!btf_type_is_ptr(t)) {
> + bpf_log(log, "Unrecognized arg#%d type %s\n",
> + i, btf_type_str(t));
> + return -EINVAL;
> + }
> +
> + ref_t = btf_type_skip_modifiers(btf, t->type, NULL);
> + ref_tname = btf_name_by_offset(btf, ref_t->name_off);
these two seem to be used only inside else `if (ptr_to_mem_ok)`, let's
move the code and variables inside that branch?
> + if (btf_get_prog_ctx_type(log, btf, t, env->prog->type, i)) {
> /* If function expects ctx type in BTF check that caller
> * is passing PTR_TO_CTX.
> */
> - if (btf_get_prog_ctx_type(log, btf, t, prog->type, i)) {
> - if (reg->type != PTR_TO_CTX) {
> - bpf_log(log,
> - "arg#%d expected pointer to ctx, but got %s\n",
> - i, btf_kind_str[BTF_INFO_KIND(t->info)]);
> - goto out;
> - }
> - if (check_ctx_reg(env, reg, i + 1))
> - goto out;
> - continue;
> + if (reg->type != PTR_TO_CTX) {
> + bpf_log(log,
> + "arg#%d expected pointer to ctx, but got %s\n",
> + i, btf_type_str(t));
> + return -EINVAL;
> }
> + if (check_ctx_reg(env, reg, regno))
> + return -EINVAL;
original code had `continue` here allowing to stop tracking if/else
logic. Any specific reason you removed it? It keeps logic simpler to
follow, imo.
> + } else if (ptr_to_mem_ok) {
similarly to how you did reduction of nestedness with btf_type_is_ptr, I'd do
if (!ptr_to_mem_ok)
return -EINVAL;
and let brain forget about another if/else branch tracking
> + const struct btf_type *resolve_ret;
> + u32 type_size;
>
> - if (!is_global)
> - goto out;
> -
> - t = btf_type_skip_modifiers(btf, t->type, NULL);
> -
> - ref_t = btf_resolve_size(btf, t, &type_size);
> - if (IS_ERR(ref_t)) {
> + resolve_ret = btf_resolve_size(btf, ref_t, &type_size);
> + if (IS_ERR(resolve_ret)) {
> bpf_log(log,
> - "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
> - i, btf_type_str(t), btf_name_by_offset(btf, t->name_off),
> - PTR_ERR(ref_t));
> - goto out;
> + "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
> + i, btf_type_str(ref_t), ref_tname,
> + PTR_ERR(resolve_ret));
> + return -EINVAL;
> }
>
> - if (check_mem_reg(env, reg, i + 1, type_size))
> - goto out;
> -
> - continue;
> + if (check_mem_reg(env, reg, regno, type_size))
> + return -EINVAL;
> + } else {
> + return -EINVAL;
> }
> - bpf_log(log, "Unrecognized arg#%d type %s\n",
> - i, btf_kind_str[BTF_INFO_KIND(t->info)]);
> - goto out;
> }
> +
> return 0;
> -out:
> +}
> +
> +/* Compare BTF of a function with given bpf_reg_state.
> + * Returns:
> + * EFAULT - there is a verifier bug. Abort verification.
> + * EINVAL - there is a type mismatch or BTF is not available.
> + * 0 - BTF matches with what bpf_reg_state expects.
> + * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
> + */
> +int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> + struct bpf_reg_state *regs)
> +{
> + struct bpf_prog *prog = env->prog;
> + struct btf *btf = prog->aux->btf;
> + bool is_global;
> + u32 btf_id;
> + int err;
> +
> + if (!prog->aux->func_info)
> + return -EINVAL;
> +
> + btf_id = prog->aux->func_info[subprog].type_id;
> + if (!btf_id)
> + return -EFAULT;
> +
> + if (prog->aux->func_info_aux[subprog].unreliable)
> + return -EINVAL;
> +
> + is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
> + err = do_btf_check_func_arg_match(env, btf, btf_id, regs, is_global);
> +
> /* Compiler optimizations can remove arguments from static functions
> * or mismatched type can be passed into a global function.
> * In such cases mark the function as unreliable from BTF point of view.
> */
> - prog->aux->func_info_aux[subprog].unreliable = true;
> - return -EINVAL;
> + if (err == -EINVAL)
> + prog->aux->func_info_aux[subprog].unreliable = true;
is there any harm marking it unreliable for any error? this makes it
look like -EINVAL is super-special. If it's EFAULT, it won't matter,
right?
> + return err;
> }
>
> /* Convert BTF of a function into bpf_reg_state if possible
> --
> 2.30.2
>
