Message-ID: <CAEf4BzbK4q9fTTYA5Apyn_wn0tb5K1_Vm7DX_OwM133V6XHB7g@mail.gmail.com>
Date:   Fri, 19 Jun 2020 11:12:29 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Andrii Nakryiko <andriin@...com>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH 01/11] bpf: Add btfid tool to resolve BTF IDs in ELF object

On Fri, Jun 19, 2020 at 6:04 AM Jiri Olsa <jolsa@...hat.com> wrote:
>
> On Thu, Jun 18, 2020 at 05:38:03PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@...nel.org> wrote:
> > >
> > > The btfid tool scans Elf object for .BTF_ids section and
> > > resolves its symbols with BTF IDs.
> >
> > naming is hard and subjective, I know. But given this actively
> > modifies ELF file it probably should indicate this in the name. So
> > something like patch_btfids or resolve_btfids would be a bit more
> > accurate and for people not in the know will still trigger the
> > "warning, tool can modify something" flag, if there are any problems.
>
> resolve_btfids sounds good to me
>
> >
> > >
> > > It will be used to during linking time to resolve arrays
> > > of BTF IDs used in verifier, so these IDs do not need to
> > > be resolved in runtime.
> > >
> > > The expected layout of .BTF_ids section is described
> > > in btfid.c header. Related kernel changes are coming in
> > > following changes.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > > ---
> > >  tools/bpf/btfid/Build    |  26 ++
> > >  tools/bpf/btfid/Makefile |  71 +++++
> > >  tools/bpf/btfid/btfid.c  | 627 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 724 insertions(+)
> > >  create mode 100644 tools/bpf/btfid/Build
> > >  create mode 100644 tools/bpf/btfid/Makefile
> > >  create mode 100644 tools/bpf/btfid/btfid.c
> > >
> >
> > [...]
> >
> > > diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
> > > new file mode 100644
> > > index 000000000000..7cdf39bfb150
> > > --- /dev/null
> > > +++ b/tools/bpf/btfid/btfid.c
> > > @@ -0,0 +1,627 @@
> > > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > > +#define  _GNU_SOURCE
> > > +
> > > +/*
> > > + * btfid scans Elf object for .BTF_ids section and resolves
> > > + * its symbols with BTF IDs.
> > > + *
> > > + * Each symbol points to 4 bytes data and is expected to have
> > > + * following name syntax:
> > > + *
> > > + * __BTF_ID__<type>__<symbol>[__<id>]
> >
> > This ___<id> thingy is just disambiguation between multiple places in
> > the code that could have BTF_ID macro, right? Or it has extra meaning?
>
> it's there so you could multiple BTF_ID instances with the same
> symbol name

yeah, reading subsequent patches clarified that for me :)

>
> >
> > > + *
> > > + * type is:
> > > + *
> > > + *   func   - lookup BTF_KIND_FUNC symbol with <symbol> name
> > > + *            and put its ID into its data
> > > + *
> > > + *             __BTF_ID__func__vfs_close__1:
> > > + *             .zero 4
> > > + *
> > > + *   struct - lookup BTF_KIND_STRUCT symbol with <symbol> name
> > > + *            and put its ID into its data
> > > + *
> > > + *             __BTF_ID__struct__sk_buff__1:
> > > + *             .zero 4
> > > + *
> > > + *   sort   - put symbol size into data area and sort following
> >
> > Oh, I finally got what "put symbol size" means :) It's quite unclear,
> > to be honest. Also, is this size in bytes or number of IDs? Clarifying
> > would be helpful (I'll probably get this from reading further down the
> > code, but still..)
>
> I 'put' ;-) the documentation mainly to kernel/bpf/btf_ids.h,
>
> so there are 2 types of lists, first one defines
> just IDs as they go:
>
>  BTF_ID_LIST(list1)
>  BTF_ID(type1, name1)
>  BTF_ID(type2, name2)
>
> and it's used for helpers btf_id array
>
> 2nd one provides count and is sorted:
>
>  BTF_WHITELIST_ENTRY(list2)
>  BTF_ID(type1, name1)
>  BTF_ID(type2, name2)
>  BTF_WHITELIST_END(list)
>
> and it's used for d_path whitelist so far


yeah, because BTF_ID_LIST and BTF_WHITELIST stuff was in two different
patches, I initially was confused by BTF_ID_LIST and thought it's
actually what BTF_WHITELIST is. Sorry for too many questions!


>
> SNIP
>
> > > +               if (sym.st_shndx != obj->efile.idlist_shndx)
> > > +                       continue;
> > > +
> > > +               name = elf_strptr(obj->efile.elf, obj->efile.strtabidx,
> > > +                                 sym.st_name);
> > > +
> > > +               if (!is_btf_id(name))
> > > +                       continue;
> > > +
> > > +               /*
> > > +                * __BTF_ID__TYPE__vfs_truncate__0
> > > +                * prefix =  ^
> > > +                */
> > > +               prefix = name + sizeof(BTF_ID) - 1;
> > > +
> > > +               if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> > > +                       id = add_struct(obj, prefix);
> > > +               } else if (!strncmp(prefix, BTF_FUNC, sizeof(BTF_FUNC) - 1)) {
> > > +                       id = add_func(obj, prefix);
> > > +               } else if (!strncmp(prefix, BTF_SORT, sizeof(BTF_SORT) - 1)) {
> > > +                       id = add_sort(obj, prefix);
> > > +
> > > +                       /*
> > > +                        * SORT objects store list's count, which is encoded
> > > +                        * in symbol's size.
> > > +                        */
> > > +                       if (id)
> > > +                               id->cnt = sym.st_size / sizeof(int);
> >
> > doesn't sym.st_size also include extra 4 bytes for length prefix?
>
> no, count is excluded from the size
>
> SNIP
>
> > > +       btf = btf__parse_elf(obj->path, NULL);
> > > +       err = libbpf_get_error(btf);
> > > +       if (err) {
> > > +               pr_err("FAILED: load BTF from %s: %s",
> > > +                       obj->path, strerror(err));
> > > +               return -1;
> > > +       }
> > > +
> > > +       nr = btf__get_nr_types(btf);
> > > +
> > > +       /*
> > > +        * Iterate all the BTF types and search for collected symbol IDs.
> > > +        */
> > > +       for (type_id = 0; type_id < nr; type_id++) {
> >
> > common gotcha: type_id <= nr, you can also skip type_id == 0 (always VOID)
>
> ugh, yep.. thanks ;-)
>
> >
> > > +               const struct btf_type *type;
> > > +               struct rb_root *root = NULL;
> > > +               struct btf_id *id;
> > > +               const char *str;
> > > +               int *nr;
> > > +
> > > +               type = btf__type_by_id(btf, type_id);
> > > +               if (!type)
> > > +                       continue;
> >
> > This ought to be an error...
>
> ok, something like "BTF malformed error"
>
> >
> > > +
> > > +               /* We support func/struct types. */
> > > +               if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC && nr_funcs) {
> >
> > see libbpf's btf.h: btf_is_func(type)
>
> ok
>
> >
> > > +                       root = &obj->funcs;
> > > +                       nr = &nr_funcs;
> > > +               } else if (BTF_INFO_KIND(type->info) == BTF_KIND_STRUCT && nr_structs) {
> >
> > same as above: btf_is_struct
> >
> > But I think you also need to support unions?
> >
> > Also what about typedefs? A lot of types are typedefs to struct/func_proto/etc.
>
> I added only types which are needed at the moment, but maybe
> we can add the basic types now, so we don't need to bother later,
> when we forget how this all work ;-)

yeah, exactly. Once this works, no one will want to go and revisit it,
so I'd rather make it generic from the get go, especially that it's
really easy in this case, right?

>
> >
> > > +                       root = &obj->structs;
> > > +                       nr = &nr_structs;
> > > +               } else {
> > > +                       continue;
> > > +               }
> > > +
> > > +               str = btf__name_by_offset(btf, type->name_off);
> > > +               if (!str)
> > > +                       continue;
> >
> > error, shouldn't happen
>
> ok
>
> >
> > > +
> > > +               id = btf_id__find(root, str);
> > > +               if (id) {
> >
> > isn't it an error, if not found?
>
> no, at this point we are checking if this BTF type was collected
> as a symbol for struct/func in some list.. if not, we continue the
> iteration to next BTF type

ah, ok

>
> >
> > > +                       id->id = type_id;
> > > +                       (*nr)--;
> > > +               }
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> >
> > [...]
> >
> > > +
> > > +       /*
> > > +        * We do proper cleanup and file close
> > > +        * intentionally only on success.
> > > +        */
> > > +       if (elf_collect(&obj))
> > > +               return -1;
> > > +
> > > +       if (symbols_collect(&obj))
> > > +               return -1;
> > > +
> > > +       if (symbols_resolve(&obj))
> > > +               return -1;
> > > +
> > > +       if (symbols_patch(&obj))
> > > +               return -1;
> >
> > nit: should these elf_end/close properly on error?
>
> I wrote in the comment above that I intentionaly do not cleanup
> on error path.. I wanted to save some time, but actualy I think
> that would not be so expensive, I can add it

as in save CPU time in error case? Who cares about that? If saving
developer time, well... `goto cleanup` is common and simple pattern ;)

>
> thanks,
> jirka
>
> >
> >
> > > +
> > > +       elf_end(obj.efile.elf);
> > > +       close(obj.efile.fd);
> > > +       return 0;
> > > +}
> > > --
> > > 2.25.4
> > >
> >
>

Powered by blists - more mailing lists