On Wed, Dec 2, 2020 at 9:30 PM John Fastabend <john.fastabend@xxxxxxxxx> wrote:
>
> Andrii Nakryiko wrote:
> > Teach libbpf to search for candidate types for CO-RE relocations across kernel
> > modules BTFs, in addition to vmlinux BTF. If at least one candidate type is
> > found in vmlinux BTF, kernel module BTFs are not iterated. If vmlinux BTF has
> > no matching candidates, then find all kernel module BTFs and search for all
> > matching candidates across all of them.
> >
> > Kernel's support for module BTFs are inferred from the support for BTF name
> > pointer in BPF UAPI.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
>
> small typo and missing close maybe? Otherwise,
>
> Acked-by: John Fastabend <john.fastabend@xxxxxxxxx>
>
> > +static int load_module_btfs(struct bpf_object *obj)
> > +{
> > + struct bpf_btf_info info;
> > + struct module_btf *mod_btf;
> > + struct btf *btf;
> > + char name[64];
> > + __u32 id = 0, len;
> > + int err, fd;
> > +
> > + if (obj->btf_modules_loaded)
> > + return 0;
> > +
> > + /* don't do this again, even if we find no module BTFs */
>
> I wonder a bit if we might load modules at some point and want
> to then run this, but I wouldn't worry about it. Its likely
> easy enough to just reload or delay load until module is in place.
right, internal function, super easy to adjust later
>
> > + obj->btf_modules_loaded = true;
> > +
> > + /* kernel too old to support module BTFs */
> > + if (!kernel_supports(FEAT_MODULE_BTF))
> > + return 0;
> > +
> > + while (true) {
> > + err = bpf_btf_get_next_id(id, &id);
> > + if (err && errno == ENOENT)
> > + return 0;
> > + if (err) {
> > + err = -errno;
> > + pr_warn("failed to iterate BTF objects: %d\n", err);
> > + return err;
> > + }
> > +
> > + fd = bpf_btf_get_fd_by_id(id);
> > + if (fd < 0) {
> > + if (errno == ENOENT)
> > + continue; /* expected race: BTF was unloaded */
> > + err = -errno;
> > + pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
> > + return err;
> > + }
> > +
> > + len = sizeof(info);
> > + memset(&info, 0, sizeof(info));
> > + info.name = ptr_to_u64(name);
> > + info.name_len = sizeof(name);
> > +
> > + err = bpf_obj_get_info_by_fd(fd, &info, &len);
> > + if (err) {
>
> do we also need to close(fd) here? If not then we don't need the close in xsk.c
> I guess.
Yes, we do. Nice catch! I added FD tracking for each module BTF in
patch #12 and adjusted the clean up code there, so it's already fixed
there. If I happen to do v6, I'll add close(fd) here just for
completeness/correctness, but otherwise probably not worth it to
respin just for that.
>
> > + err = -errno;
> > + pr_warn("failed to get BTF object #%d info: %d\n", id, err);
> > + return err;
> > + }
> > +
> > + /* ignore non-module BTFs */
> > + if (!info.kernel_btf || strcmp(name, "vmlinux") == 0) {
> > + close(fd);
> > + continue;
> > + }
> > +
> > + btf = btf_get_from_fd(fd, obj->btf_vmlinux);
> > + close(fd);
> > + if (IS_ERR(btf)) {
> > + pr_warn("failed to load module [%s]'s BTF object #%d: %ld\n",
> > + name, id, PTR_ERR(btf));
> > + return PTR_ERR(btf);
> > + }
> > +
> > + err = btf_ensure_mem((void **)&obj->btf_modules, &obj->btf_module_cap,
> > + sizeof(*obj->btf_modules), obj->btf_module_cnt + 1);
> > + if (err)
> > + return err;
> > +
> > + mod_btf = &obj->btf_modules[obj->btf_module_cnt++];
> > +
> > + mod_btf->btf = btf;
> > + mod_btf->id = id;
> > + mod_btf->name = strdup(name);
> > + if (!mod_btf->name)
> > + return -ENOMEM;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static struct core_cand_list *
> > bpf_core_find_cands(struct bpf_object *obj, const struct btf *local_btf, __u32 local_type_id)
> > {
> > struct core_cand local_cand = {};
> > struct core_cand_list *cands;
> > + const struct btf *main_btf;
> > size_t local_essent_len;
> > - int err;
> > + int err, i;
> >
> > local_cand.btf = local_btf;
> > local_cand.t = btf__type_by_id(local_btf, local_type_id);
> > @@ -4697,15 +4824,38 @@ bpf_core_find_cands(struct bpf_object *obj, const struct btf *local_btf, __u32 l
> > return ERR_PTR(-ENOMEM);
> >
> > /* Attempt to find target candidates in vmlinux BTF first */
> > - err = bpf_core_add_cands(&local_cand, local_essent_len,
> > - obj->btf_vmlinux_override ?: obj->btf_vmlinux,
> > - "vmlinux", 1, cands);
> > - if (err) {
> > - bpf_core_free_cands(cands);
> > - return ERR_PTR(err);
> > + main_btf = obj->btf_vmlinux_override ?: obj->btf_vmlinux;
> > + err = bpf_core_add_cands(&local_cand, local_essent_len, main_btf, "vmlinux", 1, cands);
> > + if (err)
> > + goto err_out;
> > +
> > + /* if vmlinux BTF has any candidate, don't got for module BTFs */
>
> small typo: don't got for -> don't go for
>
oops
> > + if (cands->len)
> > + return cands;
> > +
> > + /* if vmlinux BTF was overridden, don't attempt to load module BTFs */
> > + if (obj->btf_vmlinux_override)
> > + return cands;
> > +
> > + /* now look through module BTFs, trying to still find candidates */
> > + err = load_module_btfs(obj);
> > + if (err)
> > + goto err_out;
> > +
> > + for (i = 0; i < obj->btf_module_cnt; i++) {
> > + err = bpf_core_add_cands(&local_cand, local_essent_len,
> > + obj->btf_modules[i].btf,
> > + obj->btf_modules[i].name,
> > + btf__get_nr_types(obj->btf_vmlinux) + 1,
> > + cands);
> > + if (err)
> > + goto err_out;
> > }
> >
> > return cands;
> > +err_out:
> > + bpf_core_free_cands(cands);
> > + return ERR_PTR(err);
> > }
> >
