netdev - Re: [PATCH bpf-next v5 4/9] bpftool: Add struct definitions and helpers for BTFGen

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 linux-cve-announce 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:   Thu, 3 Feb 2022 09:24:20 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Mauricio Vásquez Bernal <mauricio@...volk.io>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Quentin Monnet <quentin@...valent.com>,
        Rafael David Tinoco <rafaeldtinoco@...il.com>,
        Lorenzo Fontana <lorenzo.fontana@...stic.co>,
        Leonardo Di Donato <leonardo.didonato@...stic.co>
Subject: Re: [PATCH bpf-next v5 4/9] bpftool: Add struct definitions and
 helpers for BTFGen

On Thu, Feb 3, 2022 at 8:08 AM Mauricio Vásquez Bernal
<mauricio@...volk.io> wrote:
>
> On Wed, Feb 2, 2022 at 1:55 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@...volk.io> wrote:
> > >
> > > Add some structs and helpers that will be used by BTFGen in the next
> > > commits.
> > >
> > > Signed-off-by: Mauricio Vásquez <mauricio@...volk.io>
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@...asec.com>
> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@...stic.co>
> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@...stic.co>
> > > ---
> >
> > Similar considerations with unused static functions. It's also harder
> > to review when I don't see how these types are actually used, so
> > probably better to put it in relevant patches that are using this?
> >
>
> The next iteration splits the patches in a way that types are
> introduced in the same commit they're used.
>
> > >  tools/bpf/bpftool/gen.c | 75 +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 75 insertions(+)
> > >
> > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > > index 64371f466fa6..68bb88e86b27 100644
> > > --- a/tools/bpf/bpftool/gen.c
> > > +++ b/tools/bpf/bpftool/gen.c
> > > @@ -1118,6 +1118,81 @@ static int btf_save_raw(const struct btf *btf, const char *path)
> > >         return err;
> > >  }
> > >
> > > +struct btfgen_type {
> > > +       struct btf_type *type;
> > > +       unsigned int id;
> > > +};
> > > +
> > > +struct btfgen_info {
> > > +       struct hashmap *types;
> > > +       struct btf *src_btf;
> > > +};
> > > +
> > > +static size_t btfgen_hash_fn(const void *key, void *ctx)
> > > +{
> > > +       return (size_t)key;
> > > +}
> > > +
> > > +static bool btfgen_equal_fn(const void *k1, const void *k2, void *ctx)
> > > +{
> > > +       return k1 == k2;
> > > +}
> > > +
> > > +static void *uint_as_hash_key(int x)
> > > +{
> > > +       return (void *)(uintptr_t)x;
> > > +}
> > > +
> > > +static void btfgen_free_type(struct btfgen_type *type)
> > > +{
> > > +       free(type);
> > > +}
> > > +
> > > +static void btfgen_free_info(struct btfgen_info *info)
> > > +{
> > > +       struct hashmap_entry *entry;
> > > +       size_t bkt;
> > > +
> > > +       if (!info)
> > > +               return;
> > > +
> > > +       if (!IS_ERR_OR_NULL(info->types)) {
> > > +               hashmap__for_each_entry(info->types, entry, bkt) {
> > > +                       btfgen_free_type(entry->value);
> > > +               }
> > > +               hashmap__free(info->types);
> > > +       }
> > > +
> > > +       btf__free(info->src_btf);
> > > +
> > > +       free(info);
> > > +}
> > > +
> > > +static struct btfgen_info *
> > > +btfgen_new_info(const char *targ_btf_path)
> > > +{
> > > +       struct btfgen_info *info;
> > > +
> > > +       info = calloc(1, sizeof(*info));
> > > +       if (!info)
> > > +               return NULL;
> > > +
> > > +       info->src_btf = btf__parse(targ_btf_path, NULL);
> > > +       if (libbpf_get_error(info->src_btf)) {
> >
> > bpftool is using libbpf 1.0 mode, so don't use libbpf_get_error()
> > anymore, just check for NULL
> >
>
> hmm, I got confused because libbpf_get_error() is still used in many
> places in bpftool. I suppose those need to be updated.

It's ok to use, but it's not necessary. Eventually I'd like to
deprecate libbpf_get_error() is it won't be necessary. So for new code
let's not add new uses of libbpf_get_error(). We can phase out
existing uses gradually (just like we do with CHECK() in selftests).


>
> > also, if you are using errno for propagating error, you need to store
> > it locally before btfgen_free_info() call, otherwise it can be
> > clobbered
> >
>
> Fixed.
>
>
>
> > > +               btfgen_free_info(info);
> > > +               return NULL;
> > > +       }
> > > +
> > > +       info->types = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> > > +       if (IS_ERR(info->types)) {
> > > +               errno = -PTR_ERR(info->types);
> > > +               btfgen_free_info(info);
> > > +               return NULL;
> > > +       }
> > > +
> > > +       return info;
> > > +}
> > > +
> > >  /* Create BTF file for a set of BPF objects */
> > >  static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> > >  {
> > > --
> > > 2.25.1
> > >