netdev - Re: [PATCH bpf-next 04/11] libbpf: implement basic split BTF support

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]

Message-ID: <CAEf4BzanQsEopXA7cGQi51hf_Q0hNb7NUTvtnkD8xg9AHoU9Ng@mail.gmail.com>
Date:   Mon, 2 Nov 2020 21:02:38 -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 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).

>
> > +     /* 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?


>
>         /* for base btf, .... */
>         if (!btf->base_btf && (!hdr->str_len || start[0]))
>                 goto err_out;
>
>         return 0;
> err_out:
>         pr_debug("Invalid BTF string section\n");
>         return -EINVAL;
> }
> > }
> >
> > @@ -372,19 +402,9 @@ static int btf_parse_type_sec(struct btf *btf)
> >       struct btf_header *hdr = btf->hdr;
> >       void *next_type = btf->types_data;
> >       void *end_type = next_type + hdr->type_len;
> > -     int err, i = 0, type_size;
>
> [...]
>