Re: Better ways to validate map via BTF?

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Nov 29, 2019 at 12:27 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Nov 28, 2019 at 8:08 AM Jesper Dangaard Brouer
> > <brouer@redhat.com> wrote:
> >>
> >> Hi Andrii,
> >
> >
> > Hey, Jesper! Sorry for late reply, I'm on vacation for few days, so my
> > availability is irregular at best :)
> >
> >>
> >> Is there are better way to validate that a userspace BPF-program uses
> >> the correct map via BTF?
> >>
> >> Below and in attached patch, I'm using bpf_obj_get_info_by_fd() to get
> >> some map-info, and check info.value_size and info.max_entries match
> >> what I expect.  What I really want, is to check that "map-value" have
> >> same struct layout as:
> >>
> >>  struct config {
> >>         __u32 action;
> >>         int ifindex;
> >>         __u32 options;
> >>  };
> >
> > Well, there is no existing magical way to do this, but it is doable by
> > comparing BTFs of two maps. It's not too hard to compare all the
> > members of a struct, their names, sizes, types, etc (and do that
> > recursively, if necessary), but it's a bunch of code requiring due
> > diligence. Libbpf doesn't provide that in a ready-to-use form (it does
> > implement equivalence checks between two type graphs for dedup, but
> > it's quite coupled with and specific to BTF deduplication algorithm).
> > Keep in mind, when Toke implemented map pinning support in libbpf, we
> > decided to not check BTF for now, and just check key/value size,
> > flags, type, max_elements, etc.
>
> Yeah. Probably a good idea to provide convenience functions for this in
> libbpf (split out the existing code and make it more general?). Then we
> can also use that for the test in the map pinning code :)

As I said, type graph equivalence for btf_dedup() is very specific to
dedup. It does deep (i.e., structs that are referenced by pointer only
also have to match exactly) and strict (const, volatile, typedefs, all
that matters **and** has to come in exactly the same order)
equivalence checks. In addition, it does forward declaration
resolution into concrete struct/union. So no, it can't be reused or
generalized.

It has to be a new code, but even then I'm hesitant to provide
something "generic", because it's again not clear what the right
semantics is for all the cases. E.g., should we ignore
const/volatile/restrict? Or, if some typedef is used, which ultimately
resolves to the same underlying type -- should we ignore such
differences? Also, should we follow and check types that are
referenced through pointers only? I think in different cases users
might be want to be strict or more lenient about such cases, which
suggests that we shouldn't have a generic API (at least yet, until we
see 2, 3, 4, real-life use cases). And there are more potential
differences in semantics without a clear answer of which one should be
used. So we can code it up for map pinning case (after having a
discussion of what two maps should be considered compatible), but I
don't think we should go all the way to exposing it as an API.


>
> -Toke
>