Re: [PATCH bpf-next 03/15] bpf: Refactor btf_check_func_arg_match

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> 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@fb.com>
> ---
>  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 = &regs[i + 1];
> +               u32 regno = i + 1;
> +               struct bpf_reg_state *reg = &regs[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
>