On Wed, Jun 22, 2022 at 10:22 AM Daniel Müller <deso@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
