Re: [PATCH bpf-next v3 1/6] bpf: Introduce pseudo_btf_id

[Andrii Nakryiko]

On Wed, Sep 16, 2020 at 3:39 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> Pseudo_btf_id is a type of ld_imm insn that associates a btf_id to a
> ksym so that further dereferences on the ksym can use the BTF info
> to validate accesses. Internally, when seeing a pseudo_btf_id ld insn,
> the verifier reads the btf_id stored in the insn[0]'s imm field and
> marks the dst_reg as PTR_TO_BTF_ID. The btf_id points to a VAR_KIND,
> which is encoded in btf_vminux by pahole. If the VAR is not of a struct
> type, the dst reg will be marked as PTR_TO_MEM instead of PTR_TO_BTF_ID
> and the mem_size is resolved to the size of the VAR's type.
>
> From the VAR btf_id, the verifier can also read the address of the
> ksym's corresponding kernel var from kallsyms and use that to fill
> dst_reg.
>
> Therefore, the proper functionality of pseudo_btf_id depends on (1)
> kallsyms and (2) the encoding of kernel global VARs in pahole, which
> should be available since pahole v1.18.
>
> Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> ---

Looks good, few minor nits if you are going to post another version
anyways. Assuming BPF offload change I mentioned is ok:

Acked-by: Andrii Nakryiko <andriin@xxxxxx>

>  include/linux/bpf_verifier.h   |   7 ++
>  include/linux/btf.h            |  15 ++++
>  include/uapi/linux/bpf.h       |  36 +++++++---
>  kernel/bpf/btf.c               |  15 ----
>  kernel/bpf/verifier.c          | 125 +++++++++++++++++++++++++++++----
>  tools/include/uapi/linux/bpf.h |  36 +++++++---
>  6 files changed, 188 insertions(+), 46 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 53c7bd568c5d..6a9dd0279ea4 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -308,6 +308,13 @@ struct bpf_insn_aux_data {
>                         u32 map_index;          /* index into used_maps[] */
>                         u32 map_off;            /* offset from value base address */
>                 };
> +               struct {
> +                       u32 reg_type;           /* type of pseudo_btf_id */

nit: there is an explicit enum for this: enum bpf_reg_type, which
matches struct bpf_reg_state's type field as well.

> +                       union {
> +                               u32 btf_id;     /* btf_id for struct typed var */
> +                               u32 mem_size;   /* mem_size for non-struct typed var */
> +                       };
> +               } btf_var;
>         };
>         u64 map_key_state; /* constant (32 bit) key tracking for maps */
>         int ctx_field_size; /* the ctx field size for load insn, maybe 0 */

[...]


> +/* replace pseudo btf_id with kernel symbol address */
> +static int check_pseudo_btf_id(struct bpf_verifier_env *env,
> +                              struct bpf_insn *insn,
> +                              struct bpf_insn_aux_data *aux)
> +{
> +       u32 type, id = insn->imm;
> +       const struct btf_type *t;
> +       const char *sym_name;
> +       u64 addr;
> +
> +       if (!btf_vmlinux) {
> +               verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
> +               return -EINVAL;
> +       }
> +
> +       t = btf_type_by_id(btf_vmlinux, id);
> +       if (!t) {
> +               verbose(env, "ldimm64 insn specifies invalid btf_id %d.\n", id);
> +               return -ENOENT;
> +       }
> +
> +       if (insn[1].imm != 0) {
> +               verbose(env, "reserved field (insn[1].imm) is used in pseudo_btf_id ldimm64 insn.\n");
> +               return -EINVAL;
> +       }

nit: I'd do this check first, before you look up type and check all
other type-related conditions

> +
> +       if (!btf_type_is_var(t)) {
> +               verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR.\n",
> +                       id);
> +               return -EINVAL;
> +       }
> +
> +       sym_name = btf_name_by_offset(btf_vmlinux, t->name_off);
> +       addr = kallsyms_lookup_name(sym_name);
> +       if (!addr) {
> +               verbose(env, "ldimm64 failed to find the address for kernel symbol '%s'.\n",
> +                       sym_name);
> +               return -ENOENT;
> +       }
> +
> +       insn[0].imm = (u32)addr;
> +       insn[1].imm = addr >> 32;
> +

[...]

> @@ -9442,6 +9533,14 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env)
>                                 /* valid generic load 64-bit imm */
>                                 goto next_insn;
>
> +                       if (insn[0].src_reg == BPF_PSEUDO_BTF_ID) {
> +                               aux = &env->insn_aux_data[i];
> +                               err = check_pseudo_btf_id(env, insn, aux);
> +                               if (err)
> +                                       return err;
> +                               goto next_insn;
> +                       }
> +
>                         /* In final convert_pseudo_ld_imm64() step, this is
>                          * converted into regular 64-bit imm load insn.
>                          */
> @@ -11392,10 +11491,6 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
>         if (is_priv)
>                 env->test_state_freq = attr->prog_flags & BPF_F_TEST_STATE_FREQ;
>
> -       ret = replace_map_fd_with_map_ptr(env);
> -       if (ret < 0)
> -               goto skip_full_check;

I'm not familiar with BPF offload stuff, so just flagging a change
here: previously offloaded BPF programs, when passed to offload's
verifier prep routine would already have ldimm64 processes, now they
won't. This might not be an issue and not an expectation we want, but
it would be nice if someone who knows something about offload stuff
confirms that this is ok.

> -
>         if (bpf_prog_is_dev_bound(env->prog->aux)) {
>                 ret = bpf_prog_offload_verifier_prep(env->prog);
>                 if (ret)

[...]