Message-ID: <CAEf4BzYUfvgYx-MPY05_rtwZics1ze0812xVxYBn=RSqSvhDpg@mail.gmail.com>
Date:   Wed, 4 Nov 2020 15:51:23 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Song Liu <songliubraving@...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 bpf-next 04/11] libbpf: implement basic split BTF support

On Mon, Nov 2, 2020 at 9:41 PM Song Liu <songliubraving@...com> wrote:
>
>
>
> > On Nov 2, 2020, at 9:02 PM, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >
> > On Mon, Nov 2, 2020 at 3:24 PM Song Liu <songliubraving@...com> wrote:
> >>
> >>
> >>
> >>> On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@...nel.org> wrote:
> >>>
> >>
> >> [...]
> >>
> >>>
> >>> BTF deduplication is not yet supported for split BTF and support for it will
> >>> be added in separate patch.
> >>>
> >>> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> >>
> >> Acked-by: Song Liu <songliubraving@...com>
> >>
> >> With a couple nits:
> >>
> >>> ---
> >>> tools/lib/bpf/btf.c      | 205 ++++++++++++++++++++++++++++++---------
> >>> tools/lib/bpf/btf.h      |   8 ++
> >>> tools/lib/bpf/libbpf.map |   9 ++
> >>> 3 files changed, 175 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >>> index db9331fea672..20c64a8441a8 100644
> >>> --- a/tools/lib/bpf/btf.c
> >>> +++ b/tools/lib/bpf/btf.c
> >>> @@ -78,10 +78,32 @@ struct btf {
> >>>      void *types_data;
> >>>      size_t types_data_cap; /* used size stored in hdr->type_len */
> >>>
> >>> -     /* type ID to `struct btf_type *` lookup index */
> >>> +     /* type ID to `struct btf_type *` lookup index
> >>> +      * type_offs[0] corresponds to the first non-VOID type:
> >>> +      *   - for base BTF it's type [1];
> >>> +      *   - for split BTF it's the first non-base BTF type.
> >>> +      */
> >>>      __u32 *type_offs;
> >>>      size_t type_offs_cap;
> >>> +     /* number of types in this BTF instance:
> >>> +      *   - doesn't include special [0] void type;
> >>> +      *   - for split BTF counts number of types added on top of base BTF.
> >>> +      */
> >>>      __u32 nr_types;
> >>
> >> This is a little confusing. Maybe add a void type for every split BTF?
> >
> > Agree about being a bit confusing. But I don't want VOID in every BTF,
> > that seems sloppy (there's no continuity). I'm currently doing similar
> > changes on kernel side, and so far everything also works cleanly with
> > start_id == 0 && nr_types including VOID (for base BTF), and start_id
> > == base_btf->nr_type && nr_types has all the added types (for split
> > BTF). That seems a bit more straightforward, so I'll probably do that
> > here as well (unless I'm missing something, I'll double check).
>
> That sounds good.

So I don't think I can do that in libbpf representation,
unfortunately. I did miss something, turns out. The difference is that
in kernel BTF is always immutable, so we can store stable pointers for
id -> btf_type lookups. For libbpf, BTF can be modified, so pointers
could be invalidated. So I instead store offsets relative to the
beginning of the type data array. With such representation having VOID
as element #0 is more tricky (I actually tried, but it's too
cumbersome). So this representation will have to be slightly different
between kernel and libbpf. But that's ok, because it's just an
internal implementation. API abstracts all of that.

>
> >
> >>
> >>> +     /* if not NULL, points to the base BTF on top of which the current
> >>> +      * split BTF is based
> >>> +      */
> >>
> >> [...]
> >>
> >>>
> >>> @@ -252,12 +274,20 @@ static int btf_parse_str_sec(struct btf *btf)
> >>>      const char *start = btf->strs_data;
> >>>      const char *end = start + btf->hdr->str_len;
> >>>
> >>> -     if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET ||
> >>> -         start[0] || end[-1]) {
> >>> -             pr_debug("Invalid BTF string section\n");
> >>> -             return -EINVAL;
> >>> +     if (btf->base_btf) {
> >>> +             if (hdr->str_len == 0)
> >>> +                     return 0;
> >>> +             if (hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1]) {
> >>> +                     pr_debug("Invalid BTF string section\n");
> >>> +                     return -EINVAL;
> >>> +             }
> >>> +     } else {
> >>> +             if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET ||
> >>> +                 start[0] || end[-1]) {
> >>> +                     pr_debug("Invalid BTF string section\n");
> >>> +                     return -EINVAL;
> >>> +             }
> >>>      }
> >>> -
> >>>      return 0;
> >>
> >> I found this function a little difficult to follow. Maybe rearrange it as
> >>
> >>        /* too long, or not \0 terminated */
> >>        if (hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1])
> >>                goto err_out;
> >
> > this won't work, if str_len == 0. Both str_len - 1 will underflow, and
> > end[-1] will be reading garbage
> >
> > How about this:
> >
> > if (btf->base_btf && hdr->str_len == 0)
> >    return 0;
> >
> > if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1])
> >    return -EINVAL;
> >
> > if (!btf->base_btf && start[0])
> >    return -EINVAL;
> >
> > return 0;
> >
> > This seems more straightforward, right?
>
> Yeah, I like this version. BTW, short comment for each condition will be
> helpful.
>
>

Powered by blists - more mailing lists