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 >
