Re: [PATCH bpf-next v3 4/6] bpf: Introduce bpf_per_cpu_ptr()

[Andrii Nakryiko]

On Wed, Sep 16, 2020 at 3:39 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> Add bpf_per_cpu_ptr() to help bpf programs access percpu vars.
> bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel
> except that it may return NULL. This happens when the cpu parameter is
> out of range. So the caller must check the returned value.
>
> Acked-by: Andrii Nakryiko <andriin@xxxxxx>
> Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> ---
>  include/linux/bpf.h            |  4 +++
>  include/linux/btf.h            | 11 ++++++
>  include/uapi/linux/bpf.h       | 18 ++++++++++
>  kernel/bpf/btf.c               | 10 ------
>  kernel/bpf/helpers.c           | 18 ++++++++++
>  kernel/bpf/verifier.c          | 64 ++++++++++++++++++++++++++++++++--
>  kernel/trace/bpf_trace.c       |  2 ++
>  tools/include/uapi/linux/bpf.h | 18 ++++++++++
>  8 files changed, 132 insertions(+), 13 deletions(-)
>

I already acked this, but see my concern about O(N) look up for
.data..percpu. Feel free to follow up on this with a separate patch.
Thanks!

[...]

> @@ -4003,6 +4008,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                         if (type != expected_type)
>                                 goto err_type;
>                 }
> +       } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
> +               expected_type = PTR_TO_PERCPU_BTF_ID;
> +               if (type != expected_type)
> +                       goto err_type;
> +               if (!reg->btf_id) {
> +                       verbose(env, "Helper has invalid btf_id in R%d\n", regno);
> +                       return -EACCES;
> +               }
> +               meta->ret_btf_id = reg->btf_id;

FYI, this will conflict with Lorenz's refactoring, so you might need
to rebase and solve the conflicts if his patch set lands first.

>         } else if (arg_type == ARG_PTR_TO_BTF_ID) {
>                 bool ids_match = false;
>
> @@ -5002,6 +5016,30 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>                 regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
>                 regs[BPF_REG_0].id = ++env->id_gen;
>                 regs[BPF_REG_0].mem_size = meta.mem_size;
> +       } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL) {
> +               const struct btf_type *t;
> +
> +               mark_reg_known_zero(env, regs, BPF_REG_0);
> +               t = btf_type_skip_modifiers(btf_vmlinux, meta.ret_btf_id, NULL);
> +               if (!btf_type_is_struct(t)) {
> +                       u32 tsize;
> +                       const struct btf_type *ret;
> +                       const char *tname;
> +
> +                       /* resolve the type size of ksym. */
> +                       ret = btf_resolve_size(btf_vmlinux, t, &tsize);
> +                       if (IS_ERR(ret)) {
> +                               tname = btf_name_by_offset(btf_vmlinux, t->name_off);
> +                               verbose(env, "unable to resolve the size of type '%s': %ld\n",
> +                                       tname, PTR_ERR(ret));
> +                               return -EINVAL;
> +                       }
> +                       regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
> +                       regs[BPF_REG_0].mem_size = tsize;
> +               } else {
> +                       regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
> +                       regs[BPF_REG_0].btf_id = meta.ret_btf_id;
> +               }
>         } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
>                 int ret_btf_id;
>
> @@ -7413,6 +7451,7 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
>                         dst_reg->mem_size = aux->btf_var.mem_size;
>                         break;
>                 case PTR_TO_BTF_ID:
> +               case PTR_TO_PERCPU_BTF_ID:
>                         dst_reg->btf_id = aux->btf_var.btf_id;
>                         break;
>                 default:
> @@ -9313,10 +9352,14 @@ 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;
> +       u32 datasec_id, type, id = insn->imm;
> +       const struct btf_var_secinfo *vsi;
> +       const struct btf_type *datasec;
>         const struct btf_type *t;
>         const char *sym_name;
> +       bool percpu = false;
>         u64 addr;
> +       int i;
>
>         if (!btf_vmlinux) {
>                 verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
> @@ -9348,12 +9391,27 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
>                 return -ENOENT;
>         }
>
> +       datasec_id = btf_find_by_name_kind(btf_vmlinux, ".data..percpu",
> +                                          BTF_KIND_DATASEC);

this is a relatively expensive O(N) operation, it probably makes sense
to cache it (there are about 80'000 types now in BTF for my typical
kernel config, so iterating that much for every single ldimm64 for
ksym is kind of expensive.

> +       if (datasec_id > 0) {
> +               datasec = btf_type_by_id(btf_vmlinux, datasec_id);
> +               for_each_vsi(i, datasec, vsi) {
> +                       if (vsi->type == id) {
> +                               percpu = true;
> +                               break;
> +                       }
> +               }
> +       }
> +

[...]