lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 2 Dec 2020 21:38:21 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v5 bpf-next 05/14] libbpf: add kernel module BTF support
 for CO-RE relocations

On Wed, Dec 2, 2020 at 9:30 PM John Fastabend <john.fastabend@...il.com> 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@...nel.org>
> > ---
>
> small typo and missing close maybe? Otherwise,
>
> Acked-by: John Fastabend <john.fastabend@...il.com>
>
> > +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);
> >  }
> >

Powered by blists - more mailing lists