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 > +} > + > 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 > + > + 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? > +} > + > +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? > + 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 :) > + 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? > + matched = true; > + break; > + } > + } > + > + if (!matched) > + return 0; > + } > + return 1; > +} > + > +static int __bpf_core_types_match(const struct btf *local_btf, u32 local_id, > + const struct btf *targ_btf, u32 targ_id, int level); > + > +static int bpf_core_composites_match(const struct btf *local_btf, const struct btf_type *local_t, Same question here - is there a reason this doesn't use a boolean as its return value? > + const struct btf *targ_btf, const struct btf_type *targ_t, > + int level) > +{ > + /* check that all local members have a match in the target */ > + const struct btf_member *local_m = btf_members(local_t); > + u16 local_vlen = btf_vlen(local_t); > + u16 targ_vlen = btf_vlen(targ_t); > + int i, j, err; > + > + if (local_vlen > targ_vlen) > + return 0; > + > + for (i = 0; i < local_vlen; i++, local_m++) { > + const char *local_n = btf_name_by_offset(local_btf, local_m->name_off); > + const struct btf_member *targ_m = btf_members(targ_t); > + bool matched = false; > + > + for (j = 0; j < targ_vlen; j++, targ_m++) { > + const char *targ_n = btf_name_by_offset(targ_btf, targ_m->name_off); > + > + if (str_is_empty(targ_n)) > + continue; > + > + if (strcmp(local_n, targ_n) != 0) > + continue; > + > + err = __bpf_core_types_match(local_btf, local_m->type, targ_btf, > + targ_m->type, level - 1); > + if (err > 0) { > + matched = true; > + break; > + } > + } > + > + if (!matched) > + return 0; > + } > + return 1; > +} > + > +static int __bpf_core_types_match(const struct btf *local_btf, u32 local_id, I personally think it's cleaner (though more verbose) if a boolean return arg is passed in to denote whether there's a match, instead of returning error, 0 for not a match, and 1 for a match > + const struct btf *targ_btf, u32 targ_id, int level) > +{ > + const struct btf_type *local_t, *targ_t, *prev_local_t; > + int depth = 32; /* max recursion depth */ > + __u16 local_k; nit: u16 and elsewhere in this function > + > + if (level <= 0) > + return -EINVAL; > + > + local_t = btf_type_by_id(local_btf, local_id); > + targ_t = btf_type_by_id(targ_btf, targ_id); > + > +recur: > + depth--; > + if (depth < 0) > + return -EINVAL; > + > + prev_local_t = local_t; > + > + local_t = btf_type_skip_modifiers(local_btf, local_id, &local_id); > + targ_t = btf_type_skip_modifiers(targ_btf, targ_id, &targ_id); > + if (!local_t || !targ_t) > + return -EINVAL; > + > + if (!bpf_core_names_match(local_btf, local_id, targ_btf, targ_id)) > + return 0; > + > + local_k = btf_kind(local_t); > + > + switch (local_k) { > + case BTF_KIND_UNKN: > + return local_k == btf_kind(targ_t); > + case BTF_KIND_FWD: { > + bool local_f = btf_type_kflag(local_t); > + __u16 targ_k = btf_kind(targ_t); > + > + if (btf_is_ptr(prev_local_t)) { > + if (local_k == targ_k) > + return local_f == btf_type_kflag(local_t); > + > + return (targ_k == BTF_KIND_STRUCT && !local_f) || > + (targ_k == BTF_KIND_UNION && local_f); I think it'd be helpful if a comment was included here that the kind flag for BTF_KIND_FWD is 0 for struct and 1 for union > + } else { > + if (local_k != targ_k) > + return 0; > + > + /* match if the forward declaration is for the same kind */ > + return local_f == btf_type_kflag(local_t); > + } > + } > + case BTF_KIND_ENUM: > + case BTF_KIND_ENUM64: > + if (!btf_is_any_enum(targ_t)) > + return 0; > + > + return bpf_core_enums_match(local_btf, local_t, targ_btf, targ_t); > + case BTF_KIND_STRUCT: > + case BTF_KIND_UNION: { > + __u16 targ_k = btf_kind(targ_t); > + > + if (btf_is_ptr(prev_local_t)) { > + bool targ_f = btf_type_kflag(local_t); Did you mean btf_type_kflag(targ_t)? > + > + if (local_k == targ_k) > + return 1; Why don't we need to check if bpf_core_composites_match() in this case? > + > + if (targ_k != BTF_KIND_FWD) > + return 0; Can there be the case where targ_k is a BTF_KIND_PTR to the same struct/union? > + > + return (local_k == BTF_KIND_UNION) == targ_f; > + } else { > + if (local_k != targ_k) > + return 0; > + > + return bpf_core_composites_match(local_btf, local_t, targ_btf, targ_t, > + level); > + } > + } > + case BTF_KIND_INT: { > + __u8 local_sgn; > + __u8 targ_sgn; > + > + if (local_k != btf_kind(targ_t)) > + return 0; > + > + local_sgn = btf_int_encoding(local_t) & BTF_INT_SIGNED; > + targ_sgn = btf_int_encoding(targ_t) & BTF_INT_SIGNED; > + > + return btf_int_bits(local_t) == btf_int_bits(targ_t) && local_sgn == targ_sgn; > + } > + case BTF_KIND_PTR: > + if (local_k != btf_kind(targ_t)) > + return 0; > + > + local_id = local_t->type; > + targ_id = targ_t->type; > + goto recur; > + case BTF_KIND_ARRAY: { > + const struct btf_array *local_array = btf_type_array(local_t); > + const struct btf_array *targ_array = btf_type_array(targ_t); > + > + if (local_k != btf_kind(targ_t)) > + return 0; > + > + if (local_array->nelems != targ_array->nelems) > + return 0; > + > + local_id = local_array->type; > + targ_id = targ_array->type; > + goto recur; > + } > + 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? > + 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 > + > static bool bpf_core_is_flavor_sep(const char *s) > { > /* check X___Y name pattern, where X and Y are not underscores */ > -- > 2.30.2 >