Date:   Mon, 13 May 2019 09:37:54 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Stanislav Fomichev <sdf@...ichev.me>,
        Andrii Nakryiko <andriin@...com>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>, bpf@...r.kernel.org,
        Alexei Starovoitov <ast@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH v3 bpf] libbpf: detect supported kernel BTF features and
 sanitize BTF

On Sun, May 12, 2019 at 4:52 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 05/12/2019 03:09 AM, Stanislav Fomichev wrote:
> > On 05/11, Andrii Nakryiko wrote:
> >> On Fri, May 10, 2019 at 3:00 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
> >>> On 05/10, Andrii Nakryiko wrote:
> >>>> On Fri, May 10, 2019 at 2:36 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
> >>>>> On 05/10, Andrii Nakryiko wrote:
> >>>>>> Depending on used versions of libbpf, Clang, and kernel, it's possible to
> >>>>>> have valid BPF object files with valid BTF information, that still won't
> >>>>>> load successfully due to Clang emitting newer BTF features (e.g.,
> >>>>>> BTF_KIND_FUNC, .BTF.ext's line_info/func_info, BTF_KIND_DATASEC, etc), that
> >>>>>> are not yet supported by older kernel.
> >>>>>>
> >>>>>> This patch adds detection of BTF features and sanitizes BPF object's BTF
> >>>>>> by substituting various supported BTF kinds, which have compatible layout:
> >>>>>>   - BTF_KIND_FUNC -> BTF_KIND_TYPEDEF
> >>>>>>   - BTF_KIND_FUNC_PROTO -> BTF_KIND_ENUM
> >>>>>>   - BTF_KIND_VAR -> BTF_KIND_INT
> >>>>>>   - BTF_KIND_DATASEC -> BTF_KIND_STRUCT
> >>>>>>
> >>>>>> Replacement is done in such a way as to preserve as much information as
> >>>>>> possible (names, sizes, etc) where possible without violating kernel's
> >>>>>> validation rules.
> >>>>>>
> >>>>>> v2->v3:
> >>>>>>   - remove duplicate #defines from libbpf_util.h
> >>>>>>
> >>>>>> v1->v2:
> >>>>>>   - add internal libbpf_internal.h w/ common stuff
> >>>>> How is libbpf_internal.h different from libbpf_util.h? libbpf_util.h
> >>>>> looks pretty "internal" to me. Maybe use that one instead?
> >>>>
> >>>> It's not anymore. It's included from xsk.h, which is not internal, so
> >>>> libbpf_util.h was recently exposed as public as well.
> >>> But I still don't see any LIBBPF_API exported functions in libbpf_util.h.
> >>> It looks like the usage is still mostly (only?) internal. The barrier
> >>> stuff is for internal usage as well.
> >>
> >> libbpf_util.h is installed along xsk.h, bpf.h, etc, so it is becoming
> >> part of public API, even if it's not exposing any LIBBPF_API calls.
> >> Those barrier calls are intended for internal usage, but we can't
> >> enforce that. With libbpf_internal.h we can (as we don't install it).
> >> We should probably move libbpf_print and related #defines out of
> >> libbpf_util.h, which I can do in separate patch, if we agree on that.
> > We could move libbpf_print into libbpf_internal.h, but the barrier defines
> > are used in xsk.h. If we do that, libbpf_util.h should probably
> > be renamed to libbpf_barrier.h :-/
>
> Agree on the libbpf_print move into libbpf_internal.h (Andrii or Stanislav, could
> you send a patch? Thx). But calling the remainder libbpf_barrier.h is imho too

I can do that, it's still technically a fix, as libbpf_print shouldn't
have been exposed in the first place. I'll send a patch against bpf
tree.

> specific and thus a bit overkill. Given we can control what is being exposed and
> installed as header, we should only place helpers in there that are used in the
> other installed helpers.
>
> >>> Also, why do think your new probe helper should be internal? I guess
> >>> that at some point bpftool might use it to probe and dump BTF features
> >>> as well.
> >>
> >> I don't think it's a proper level of abstraction to be exposed as
> >> public API. In it's current form, that thing takes raw arrays of
> >> bytes, constructs BTF out of it and tries to load it without logging
> >> any errors. There seems to be little of use for external application
> >> in it and I don't think those applications should construct BTF out of
> >> raw integers (see below).
> > SGTM, we can export it if/when needed.
> >
> >>>
> >>> I also see us copying around all the BTF_XXX macros, I brought this up for
> >>> some selftest patches and now we have a single place for BTX_XXX macros
> >>> in selftests (tools/testing/selftests/test_bpf.h).
> >>> Maybe they should belong to libbpf instead?
> >>
> >> I think, ideally, we should get rid of those BTF_XXX macros in favor
> >> of some kind of BTF writer/builder, e.g.:
> >>
> >> struct btf_builder *b = btf_bldr__new();
> >> struct btf *btf;
> >> char buf[256];
> >> int i=0;
> >>
> >> btf_bldr__add_enum(b, "some_enum");
> >> for (i = 0; i < 5; i++) {
> >>         sprintf(buf, "enum_val_%d", i);
> >>         btf_bldr__add_enum_value(b, buf, i);
> >> }
> >> /* ... and so on ... */
> >>
> >> btf = btf_bldr__create_btf();
> >>
> >> So I don't mind moving those macros to libbpf_internal.h for now, I
> >> think in the longer-term they should be gone, though. But I'd like to
> >> keep the scope of this patch smaller and not do too much refactoring
> >> of tests.
>
> Such BTF writer would be nice actually.

I'll try to add that as time allows.

>
> > Do you think that libbpf probes can/will be converted to this API?
> > It looks like test_btf.c will always use defines, so if we use them in libbpf

Yeah, I don't see why not. It will change the way tests are written,
though, as now you won't be able to define BTF statically, you'll need
to write corresponding BTF-generating callbacks, so it might not make
sense to migrate all the tests immediately.

> > as well (internally), it probably makes sense just to move test_btf.h
> > into libbpf and rename it to something like raw_btf.h (and keep it
> > internal, don't install it).
>
> Kind of agree with Stanislav that avoiding duplication here would be
> nice. Maybe they could be a libbpf internal header, but selftests would
> include them directly as it's the only other place where the raw form is
> currently used ... and given both are in tree.

Yeah, sure, we can consolidate macros in libbpf_internal.h. It feels
like calling those macros as BTF_RAW_XXX would be explicit about what
they are and distinguishing from other macros very clearly, I'll work
on a patch against bpf-next for that.

>
> Anyway, as all this is kind of separate follow-up to the current patch,
> I've applied the fix to bpf to unblock users, thanks.

Thanks!

Powered by blists - more mailing lists