Re: [PATCH bpf-next 3/7] bpf: Add type match support

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

On Wed, Jun 22, 2022 at 10:22 AM Daniel Müller <deso@posteo.net> wrote:
>
> On Tue, Jun 21, 2022 at 12:41:22PM -0700, Joanne Koong wrote:
> >  On Mon, Jun 20, 2022 at 4:25 PM Daniel Müller <deso@posteo.net> wrote:
> > >
> > > This change implements the kernel side of the "type matches" support.
> > > Please refer to the next change ("libbpf: Add type match support") for
> > > more details on the relation. This one is first in the stack because
> > > the follow-on libbpf changes depend on it.
> > >
> > > Signed-off-by: Daniel Müller <deso@posteo.net>
> > > ---
> > >  include/linux/btf.h |   5 +
> > >  kernel/bpf/btf.c    | 267 ++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 272 insertions(+)
> > >
> > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > index 1bfed7..7376934 100644
> > > --- a/include/linux/btf.h
> > > +++ b/include/linux/btf.h
> > > @@ -242,6 +242,11 @@ static inline u8 btf_int_offset(const struct btf_type *t)
> > >         return BTF_INT_OFFSET(*(u32 *)(t + 1));
> > >  }
> > >
> > > +static inline u8 btf_int_bits(const struct btf_type *t)
> > > +{
> > > +       return BTF_INT_BITS(*(__u32 *)(t + 1));
> > nit: u32 here instead of __u32
>
> Ah yeah, changed!
>
> > > +}
> > > +
> > >  static inline u8 btf_int_encoding(const struct btf_type *t)
> > >  {
> > >         return BTF_INT_ENCODING(*(u32 *)(t + 1));
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index f08037..3790b4 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -7524,6 +7524,273 @@ int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
> > >                                            MAX_TYPES_ARE_COMPAT_DEPTH);
> > >  }
> > >
> > > +#define MAX_TYPES_MATCH_DEPTH 2
> > > +
> > > +static bool bpf_core_names_match(const struct btf *local_btf, u32 local_id,
> > > +                                const struct btf *targ_btf, u32 targ_id)
> > > +{
> > > +       const struct btf_type *local_t, *targ_t;
> > > +       const char *local_n, *targ_n;
> > > +       size_t local_len, targ_len;
> > > +
> > > +       local_t = btf_type_by_id(local_btf, local_id);
> > > +       targ_t = btf_type_by_id(targ_btf, targ_id);
> > > +       local_n = btf_str_by_offset(local_btf, local_t->name_off);
> > > +       targ_n = btf_str_by_offset(targ_btf, targ_t->name_off);
> > > +       local_len = bpf_core_essential_name_len(local_n);
> > > +       targ_len = bpf_core_essential_name_len(targ_n);
> > nit: i personally think this would be a little visually easier to read
> > if there was a line space between targ_t and local_n, and between
> > targ_n and local_len
>
> Will add spaces as you suggest. I've also changed the signature to pass in the
> actual btf_type pointer directly, which is trivially available at the call site.
> That makes the block a bit shorter.
>
> > > +
> > > +       return local_len == targ_len && strncmp(local_n, targ_n, local_len) == 0;
> > Does calling "return !strcmp(local_n, targ_n);" do the same thing here?
>
> I think it does. Changed. Thanks!

No, it doesn't. task_struct___kernel and task_struct___libbpf will
have same local_len and targ_len and should be considered a name
match, but strcmp() will return false. That strncmp() is there for a
very good reason.

And as an aside, it's very much personal preference, but I find
!strcmp() form very disruptive to reason about, so with all the string
apis returning 0 on match I prefere == 0 explicitly. Let's keep that
convention as is.

>
> > > +}
> > > +
> > > +static int bpf_core_enums_match(const struct btf *local_btf, const struct btf_type *local_t,
> > I find the return values a bit confusing here.  The convention in
> > linux is to return 0 for the success case. Maybe I'm totally missing
> > something here, but is there a reason this doesn't just return a
> > boolean?
>
> I basically took bpf_core_types_are_compat() as the guiding function for the
> signature, because bpf_core_enums_match() is used in the same contexts alongside
> it. The reason it uses int, from what I can tell, is because it merges error
> returns in there as well (-EINVAL). Given that we do the same, I think we should
> stick to the same signature as well.

Yes, it's a boolean function that can fail, so it has to return int.

>
> > > +                               const struct btf *targ_btf, const struct btf_type *targ_t)
> > > +{
> > > +       u16 local_vlen = btf_vlen(local_t);
> > > +       u16 targ_vlen = btf_vlen(targ_t);
> > > +       int i, j;
> > > +
> > > +       if (local_t->size != targ_t->size)
> > > +               return 0;
> > > +
> > > +       if (local_vlen > targ_vlen)
> > > +               return 0;
> > > +
> > > +       /* iterate over the local enum's variants and make sure each has
> > > +        * a symbolic name correspondent in the target
> > > +        */
> > > +       for (i = 0; i < local_vlen; i++) {
> > > +               bool matched = false;
> > > +               const char *local_n;
> > > +               __u32 local_n_off;
> > nit: u32 instead of __u32 :)
>
> As per discussion with Alexei I have deduplicated this function (between kernel
> and userspace) and moved it into relo_core.c. Unfortunately, this file insists
> on usage of __32 (for better or worse):
>
>   xxxx:yyy:zz: error: attempt to use poisoned "u32"
>

right, libbpf can't use u32, it's a kernel-only alias

> > > +               size_t local_len;
> > > +
> > > +               local_n_off = btf_is_enum(local_t) ? btf_type_enum(local_t)[i].name_off :
> > > +                                                    btf_type_enum64(local_t)[i].name_off;
> > > +
> > > +               local_n = btf_name_by_offset(local_btf, local_n_off);
> > > +               local_len = bpf_core_essential_name_len(local_n);
> > > +
> > > +               for (j = 0; j < targ_vlen; j++) {
> > > +                       const char *targ_n;
> > > +                       __u32 targ_n_off;
> > > +                       size_t targ_len;
> > > +
> > > +                       targ_n_off = btf_is_enum(targ_t) ? btf_type_enum(targ_t)[j].name_off :
> > > +                                                          btf_type_enum64(targ_t)[j].name_off;
> > > +                       targ_n = btf_name_by_offset(targ_btf, targ_n_off);
> > > +
> > > +                       if (str_is_empty(targ_n))
> > > +                               continue;
> > > +
> > > +                       targ_len = bpf_core_essential_name_len(targ_n);
> > > +
> > > +                       if (local_len == targ_len && strncmp(local_n, targ_n, local_len) == 0) {
> > same question here - does strcmp suffice?
>
> I believe it does. Changed.

see above, it doesn't

>
> > > +                               matched = true;
> > > +                               break;
> > > +                       }
> > > +               }
> > > +
> > > +               if (!matched)
> > > +                       return 0;
> > > +       }
> > > +       return 1;
> > > +}
> > > +

[...]

> > > +       case BTF_KIND_FUNC_PROTO: {
> > > +               struct btf_param *local_p = btf_params(local_t);
> > > +               struct btf_param *targ_p = btf_params(targ_t);
> > > +               u16 local_vlen = btf_vlen(local_t);
> > > +               u16 targ_vlen = btf_vlen(targ_t);
> > > +               int i, err;
> > > +
> > > +               if (local_k != btf_kind(targ_t))
> > > +                       return 0;
> > > +
> > > +               if (local_vlen != targ_vlen)
> > > +                       return 0;
> > > +
> > > +               for (i = 0; i < local_vlen; i++, local_p++, targ_p++) {
> > > +                       err = __bpf_core_types_match(local_btf, local_p->type, targ_btf,
> > > +                                                    targ_p->type, level - 1);
> > > +                       if (err <= 0)
> > > +                               return err;
> > > +               }
> > > +
> > > +               /* tail recurse for return type check */
> > > +               local_id = local_t->type;
> > > +               targ_id = targ_t->type;
> > > +               goto recur;
> > > +       }
> > > +       default:
> > Do BTF_KIND_FLOAT and BTF_KIND_TYPEDEF need to be checked as well?
>
> Lack of BTF_KIND_TYPEDEF is a good question. I don't know why it's missing from
> bpf_core_types_are_compat() as well, which I took as a template. I will do some
> testing to better understand if we can hit this case or whether there is some
> magic going on that would have resolved typedefs already at this point (which is
> my suspicion).
> My understanding why we don't cover floats is because we do not allow floating
> point operations in kernel code (right?).

FLOAT is an omission, we need to add it (kernel types do have floats).
But TYPEDEF (as well as CONST/VOLATILE/RESTRICT) will be skipped by
btf_type_skip_modifiers(), so we should never see them in this switch.

>
> > > +               return 0;
> > > +       }
> > > +}
> > > +
> > > +int bpf_core_types_match(const struct btf *local_btf, u32 local_id,
> > > +                        const struct btf *targ_btf, u32 targ_id)
> > > +{
> > > +       return __bpf_core_types_match(local_btf, local_id,
> > > +                                     targ_btf, targ_id,
> > > +                                     MAX_TYPES_MATCH_DEPTH);
> > > +}
> > Also, btw, thanks for the thorough cover letter - its high-level
> > overview made it easier to understand the patches
>
> Thanks!
>
> Daniel