Re: [PATCH bpf-next] libbpf: Load btf_vmlinux only once per object.

[Andrii Nakryiko]

On Fri, Jan 17, 2020 at 8:58 AM KP Singh <kpsingh@xxxxxxxxxxxx> wrote:
>
> From: KP Singh <kpsingh@xxxxxxxxxx>
>
> As more programs (TRACING, STRUCT_OPS, and upcoming LSM) use vmlinux
> BTF information, loading the BTF vmlinux information for every program
> in an object is sub-optimal. The fix was originally proposed in:
>
>    https://lore.kernel.org/bpf/CAEf4BzZodr3LKJuM7QwD38BiEH02Cc1UbtnGpVkCJ00Mf+V_Qg@xxxxxxxxxxxxxx/
>
> The btf_vmlinux is populated in the object if any of the programs in
> the object requires it just before the programs are loaded and freed
> after the programs finish loading.
>
> Reported-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> Reviewed-by: Brendan Jackman <jackmanb@xxxxxxxxxxxx>
> Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> ---

Thanks for the clean up! Few issues, but overall I like this.

>  tools/lib/bpf/libbpf.c | 148 +++++++++++++++++++++++++++--------------
>  1 file changed, 97 insertions(+), 51 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3afaca9bce1d..db0e93882a3b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -385,6 +385,10 @@ struct bpf_object {

[...]

> @@ -2364,6 +2357,38 @@ static int bpf_object__finalize_btf(struct bpf_object *obj)
>         return 0;
>  }
>
> +static inline bool libbpf_prog_needs_vmlinux_btf(struct bpf_program *prog)
> +{

I suspect that at some point this approach won't be flexible enough,
but it simplifies error handling right now, so I'm ok with it.

> +       if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
> +               return true;
> +
> +       /* BPF_PROG_TYPE_TRACING programs which do not attach to other programs
> +        * also need vmlinux BTF
> +        */
> +       if (prog->type == BPF_PROG_TYPE_TRACING && !prog->attach_prog_fd)
> +               return true;
> +
> +       return false;
> +}
> +
> +static int bpf_object__load_vmlinux_btf(struct bpf_object *obj)
> +{
> +       struct bpf_program *prog;
> +
> +       bpf_object__for_each_program(prog, obj) {
> +               if (libbpf_prog_needs_vmlinux_btf(prog)) {
> +                       obj->btf_vmlinux = libbpf_find_kernel_btf();
> +                       if (IS_ERR(obj->btf_vmlinux)) {
> +                               pr_warn("vmlinux BTF is not found\n");

please, emit error code as well

also, clear out btf_vmlinux, otherwise your code will attempt to free
invalid pointer later on

> +                               return -EINVAL;
> +                       }
> +                       return 0;
> +               }
> +       }
> +
> +       return 0;
> +}

[...]

> @@ -5280,10 +5301,17 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>         err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
>         err = err ? : bpf_object__sanitize_and_load_btf(obj);
>         err = err ? : bpf_object__sanitize_maps(obj);
> +       err = err ? : bpf_object__load_vmlinux_btf(obj);
>         err = err ? : bpf_object__init_kern_struct_ops_maps(obj);
>         err = err ? : bpf_object__create_maps(obj);
>         err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
>         err = err ? : bpf_object__load_progs(obj, attr->log_level);
> +
> +       if (obj->btf_vmlinux) {

you can skip this check, btf__free(NULL) is handled properly as noop

> +               btf__free(obj->btf_vmlinux);
> +               obj->btf_vmlinux = NULL;
> +       }
> +
>         if (err)
>                 goto out;

[...]

> +
> +static inline int __find_vmlinux_btf_id(struct btf *btf, const char *name,
> +                                       enum bpf_attach_type attach_type)
> +{
> +       int err;
> +
> +       if (attach_type == BPF_TRACE_RAW_TP)
> +               err = find_btf_by_prefix_kind(btf, BTF_TRACE_PREFIX, name,
> +                                             BTF_KIND_TYPEDEF);
> +       else
> +               err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> +
> +       return err;
> +}
> +
>  int libbpf_find_vmlinux_btf_id(const char *name,
>                                enum bpf_attach_type attach_type)
>  {
>         struct btf *btf = libbpf_find_kernel_btf();

I had complaints previously about doing too much heavy-lifting in
variable assignment, not sure why this slipped through. Can you please
split this into variable declaration and separate assignment below?

> -       char raw_tp_btf[128] = BTF_PREFIX;
> -       char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
> -       const char *btf_name;
> -       int err = -EINVAL;
> -       __u32 kind;
>
>         if (IS_ERR(btf)) {
>                 pr_warn("vmlinux BTF is not found\n");
>                 return -EINVAL;
>         }
>
> -       if (attach_type == BPF_TRACE_RAW_TP) {
> -               /* prepend "btf_trace_" prefix per kernel convention */
> -               strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
> -               btf_name = raw_tp_btf;
> -               kind = BTF_KIND_TYPEDEF;
> -       } else {
> -               btf_name = name;
> -               kind = BTF_KIND_FUNC;
> -       }
> -       err = btf__find_by_name_kind(btf, btf_name, kind);
> -       btf__free(btf);
> -       return err;
> +       return __find_vmlinux_btf_id(btf, name, attach_type);
>  }
>
>  static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> @@ -6567,10 +6612,11 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
>         return err;
>  }
>
> -static int libbpf_find_attach_btf_id(const char *name,
> -                                    enum bpf_attach_type attach_type,
> -                                    __u32 attach_prog_fd)
> +static int libbpf_find_attach_btf_id(struct bpf_program *prog)
>  {
> +       enum bpf_attach_type attach_type = prog->expected_attach_type;
> +       __u32 attach_prog_fd = prog->attach_prog_fd;
> +       const char *name = prog->section_name;
>         int i, err;
>
>         if (!name)
> @@ -6585,8 +6631,8 @@ static int libbpf_find_attach_btf_id(const char *name,
>                         err = libbpf_find_prog_btf_id(name + section_defs[i].len,
>                                                       attach_prog_fd);
>                 else
> -                       err = libbpf_find_vmlinux_btf_id(name + section_defs[i].len,
> -                                                        attach_type);
> +                       err = __find_vmlinux_btf_id(prog->obj->btf_vmlinux,
> +                               name + section_defs[i].len, attach_type);

argument indentation is off here, please fix

>                 if (err <= 0)
>                         pr_warn("%s is not found in vmlinux BTF\n", name);
>                 return err;
> --
> 2.20.1
>