| Message ID | 20220622173506.860578-2-deso@posteo.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Deduplicate type compat check | expand |
On Wed, Jun 22, 2022 at 05:35:05PM +0000, Daniel Müller wrote: > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c > index 6ad3c3..e54370 100644 > --- a/tools/lib/bpf/relo_core.c > +++ b/tools/lib/bpf/relo_core.c > @@ -141,6 +141,84 @@ static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind) > } > } > > +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id, > + const struct btf *targ_btf, __u32 targ_id, int level) > +{ [...] > + > + /* tail recurse for return type check */ > + skip_mods_and_typedefs(local_btf, local_type->type, &local_id); > + skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id); > + goto recur; > + } > + default: > + return 0; I actually left out the pr_warn here (present in libbpf but not the kernel). I suppose it would be best to carry it over from libbpf? Thanks, Daniel
On Wed, Jun 22, 2022 at 10:35 AM Daniel Müller <deso@posteo.net> wrote: > > This change merges the two existing implementations of the > bpf_core_types_are_compat() function into relo_core.c, inheriting the > recursion tracking from the kernel and the usage of > btf_kind_core_compat() from libbpf. The kernel is left untouched and > will be adjusted subsequently. > > Signed-off-by: Daniel Müller <deso@posteo.net> > --- I don't feel very strongly about this, but given we are consolidating kernel and libbpf code, I think it makes sense to do it in one patch. > tools/lib/bpf/libbpf.c | 72 +----------------------------------- > tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++++++ > tools/lib/bpf/relo_core.h | 2 + > 3 files changed, 81 insertions(+), 71 deletions(-) > [...] > - default: > - pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n", > - btf_kind_str(local_type), local_id, targ_id); > - return 0; > - } > + return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id, INT_MAX); INT_MAX seems like an overkill, let's just hard-code 32 just like we have for a local recursion limit here? > } > [...] > /* > * Turn bpf_core_relo into a low- and high-level spec representation, > * validating correctness along the way, as well as calculating resulting > diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h > index 7df0da0..b8998f 100644 > --- a/tools/lib/bpf/relo_core.h > +++ b/tools/lib/bpf/relo_core.h > @@ -68,6 +68,8 @@ struct bpf_core_relo_res { > __u32 new_type_id; > }; > > +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id, > + const struct btf *targ_btf, __u32 targ_id, int level); Just leave it called __bpf_core_types_are_compat like in kernel, it is clearly an "internal" version of bpf_core_types_are_compat(), so it's more proper naming convention. > int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id, > const struct btf *targ_btf, __u32 targ_id); > > -- > 2.30.2 >
On Wed, Jun 22, 2022 at 3:19 PM Daniel Müller <deso@posteo.net> wrote: > > On Wed, Jun 22, 2022 at 05:35:05PM +0000, Daniel Müller wrote: > > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c > > index 6ad3c3..e54370 100644 > > --- a/tools/lib/bpf/relo_core.c > > +++ b/tools/lib/bpf/relo_core.c > > @@ -141,6 +141,84 @@ static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind) > > } > > } > > > > +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id, > > + const struct btf *targ_btf, __u32 targ_id, int level) > > +{ > > [...] > > > + > > + /* tail recurse for return type check */ > > + skip_mods_and_typedefs(local_btf, local_type->type, &local_id); > > + skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id); > > + goto recur; > > + } > > + default: > > + return 0; > > I actually left out the pr_warn here (present in libbpf but not the kernel). I > suppose it would be best to carry it over from libbpf? Yes, please. > > Thanks, > Daniel
On Wed, Jun 22, 2022 at 09:15:01PM -0700, Andrii Nakryiko wrote: > On Wed, Jun 22, 2022 at 10:35 AM Daniel Müller <deso@posteo.net> wrote: > > > > This change merges the two existing implementations of the > > bpf_core_types_are_compat() function into relo_core.c, inheriting the > > recursion tracking from the kernel and the usage of > > btf_kind_core_compat() from libbpf. The kernel is left untouched and > > will be adjusted subsequently. > > > > Signed-off-by: Daniel Müller <deso@posteo.net> > > --- > > I don't feel very strongly about this, but given we are consolidating > kernel and libbpf code, I think it makes sense to do it in one patch. Sure. > > tools/lib/bpf/libbpf.c | 72 +----------------------------------- > > tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++++++ > > tools/lib/bpf/relo_core.h | 2 + > > 3 files changed, 81 insertions(+), 71 deletions(-) > > > > [...] > > > - default: > > - pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n", > > - btf_kind_str(local_type), local_id, targ_id); > > - return 0; > > - } > > + return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id, INT_MAX); > > INT_MAX seems like an overkill, let's just hard-code 32 just like we > have for a local recursion limit here? Okay. > > } > > > > [...] > > > /* > > * Turn bpf_core_relo into a low- and high-level spec representation, > > * validating correctness along the way, as well as calculating resulting > > diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h > > index 7df0da0..b8998f 100644 > > --- a/tools/lib/bpf/relo_core.h > > +++ b/tools/lib/bpf/relo_core.h > > @@ -68,6 +68,8 @@ struct bpf_core_relo_res { > > __u32 new_type_id; > > }; > > > > +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id, > > + const struct btf *targ_btf, __u32 targ_id, int level); > > Just leave it called __bpf_core_types_are_compat like in kernel, it is > clearly an "internal" version of bpf_core_types_are_compat(), so it's > more proper naming convention. Sounds good. [...] Thanks, Daniel
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 49e359c..ca912c 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -5732,77 +5732,7 @@ bpf_core_find_cands(struct bpf_object *obj, const struct btf *local_btf, __u32 l int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id, const struct btf *targ_btf, __u32 targ_id) { - const struct btf_type *local_type, *targ_type; - int depth = 32; /* max recursion depth */ - - /* caller made sure that names match (ignoring flavor suffix) */ - local_type = btf__type_by_id(local_btf, local_id); - targ_type = btf__type_by_id(targ_btf, targ_id); - if (!btf_kind_core_compat(local_type, targ_type)) - return 0; - -recur: - depth--; - if (depth < 0) - return -EINVAL; - - local_type = skip_mods_and_typedefs(local_btf, local_id, &local_id); - targ_type = skip_mods_and_typedefs(targ_btf, targ_id, &targ_id); - if (!local_type || !targ_type) - return -EINVAL; - - if (!btf_kind_core_compat(local_type, targ_type)) - return 0; - - switch (btf_kind(local_type)) { - case BTF_KIND_UNKN: - case BTF_KIND_STRUCT: - case BTF_KIND_UNION: - case BTF_KIND_ENUM: - case BTF_KIND_ENUM64: - case BTF_KIND_FWD: - return 1; - case BTF_KIND_INT: - /* just reject deprecated bitfield-like integers; all other - * integers are by default compatible between each other - */ - return btf_int_offset(local_type) == 0 && btf_int_offset(targ_type) == 0; - case BTF_KIND_PTR: - local_id = local_type->type; - targ_id = targ_type->type; - goto recur; - case BTF_KIND_ARRAY: - local_id = btf_array(local_type)->type; - targ_id = btf_array(targ_type)->type; - goto recur; - case BTF_KIND_FUNC_PROTO: { - struct btf_param *local_p = btf_params(local_type); - struct btf_param *targ_p = btf_params(targ_type); - __u16 local_vlen = btf_vlen(local_type); - __u16 targ_vlen = btf_vlen(targ_type); - int i, err; - - if (local_vlen != targ_vlen) - return 0; - - for (i = 0; i < local_vlen; i++, local_p++, targ_p++) { - skip_mods_and_typedefs(local_btf, local_p->type, &local_id); - skip_mods_and_typedefs(targ_btf, targ_p->type, &targ_id); - err = bpf_core_types_are_compat(local_btf, local_id, targ_btf, targ_id); - if (err <= 0) - return err; - } - - /* tail recurse for return type check */ - skip_mods_and_typedefs(local_btf, local_type->type, &local_id); - skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id); - goto recur; - } - default: - pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n", - btf_kind_str(local_type), local_id, targ_id); - return 0; - } + return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id, INT_MAX); } static size_t bpf_core_hash_fn(const void *key, void *ctx) diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c index 6ad3c3..e54370 100644 --- a/tools/lib/bpf/relo_core.c +++ b/tools/lib/bpf/relo_core.c @@ -141,6 +141,84 @@ static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind) } } +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id, + const struct btf *targ_btf, __u32 targ_id, int level) +{ + const struct btf_type *local_type, *targ_type; + int depth = 32; /* max recursion depth */ + + /* caller made sure that names match (ignoring flavor suffix) */ + local_type = btf_type_by_id(local_btf, local_id); + targ_type = btf_type_by_id(targ_btf, targ_id); + if (!btf_kind_core_compat(local_type, targ_type)) + return 0; + +recur: + depth--; + if (depth < 0) + return -EINVAL; + + local_type = skip_mods_and_typedefs(local_btf, local_id, &local_id); + targ_type = skip_mods_and_typedefs(targ_btf, targ_id, &targ_id); + if (!local_type || !targ_type) + return -EINVAL; + + if (!btf_kind_core_compat(local_type, targ_type)) + return 0; + + switch (btf_kind(local_type)) { + case BTF_KIND_UNKN: + case BTF_KIND_STRUCT: + case BTF_KIND_UNION: + case BTF_KIND_ENUM: + case BTF_KIND_FWD: + case BTF_KIND_ENUM64: + return 1; + case BTF_KIND_INT: + /* just reject deprecated bitfield-like integers; all other + * integers are by default compatible between each other + */ + return btf_int_offset(local_type) == 0 && btf_int_offset(targ_type) == 0; + case BTF_KIND_PTR: + local_id = local_type->type; + targ_id = targ_type->type; + goto recur; + case BTF_KIND_ARRAY: + local_id = btf_array(local_type)->type; + targ_id = btf_array(targ_type)->type; + goto recur; + case BTF_KIND_FUNC_PROTO: { + struct btf_param *local_p = btf_params(local_type); + struct btf_param *targ_p = btf_params(targ_type); + __u16 local_vlen = btf_vlen(local_type); + __u16 targ_vlen = btf_vlen(targ_type); + int i, err; + + if (local_vlen != targ_vlen) + return 0; + + for (i = 0; i < local_vlen; i++, local_p++, targ_p++) { + if (level <= 0) + return -EINVAL; + + skip_mods_and_typedefs(local_btf, local_p->type, &local_id); + skip_mods_and_typedefs(targ_btf, targ_p->type, &targ_id); + err = bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, + targ_id, level - 1); + if (err <= 0) + return err; + } + + /* tail recurse for return type check */ + skip_mods_and_typedefs(local_btf, local_type->type, &local_id); + skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id); + goto recur; + } + default: + return 0; + } +} + /* * Turn bpf_core_relo into a low- and high-level spec representation, * validating correctness along the way, as well as calculating resulting diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h index 7df0da0..b8998f 100644 --- a/tools/lib/bpf/relo_core.h +++ b/tools/lib/bpf/relo_core.h @@ -68,6 +68,8 @@ struct bpf_core_relo_res { __u32 new_type_id; }; +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id, + const struct btf *targ_btf, __u32 targ_id, int level); int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id, const struct btf *targ_btf, __u32 targ_id);
This change merges the two existing implementations of the bpf_core_types_are_compat() function into relo_core.c, inheriting the recursion tracking from the kernel and the usage of btf_kind_core_compat() from libbpf. The kernel is left untouched and will be adjusted subsequently. Signed-off-by: Daniel Müller <deso@posteo.net> --- tools/lib/bpf/libbpf.c | 72 +----------------------------------- tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++++++ tools/lib/bpf/relo_core.h | 2 + 3 files changed, 81 insertions(+), 71 deletions(-)