Thanks for taking a look. Sending out a v2 with the fixes. On 17-Jan 11:02, Andrii Nakryiko wrote: > 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. Thanks! > > > 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. Acknowledged. > > > + 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 Done, also changed it so that the error code does not get clobbered. > > + 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 Done. Skipped. > > > + 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? Done. > > > - 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 Fixed. - KP > > > if (err <= 0) > > pr_warn("%s is not found in vmlinux BTF\n", name); > > return err; > > -- > > 2.20.1 > >
