* [PATCH bpf-next 1/7] bpf: Introduce TYPE_MATCH related constants/macros
2022-06-20 23:17 [PATCH bpf-next 0/7] Introduce type match support Daniel Müller
@ 2022-06-20 23:17 ` Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 2/7] bpftool: Honor BPF_CORE_TYPE_MATCHES relocation Daniel Müller
` (5 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-20 23:17 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team
In order to provide type match support we require a new type of
relocation which, in turn, requires toolchain support. Recent LLVM/Clang
versions support a new value for the last argument to the
__builtin_preserve_type_info builtin, for example.
With this change we introduce the necessary constants into relevant
header files, mirroring what the compiler may support.
Signed-off-by: Daniel Müller <deso@posteo.net>
---
include/uapi/linux/bpf.h | 1 +
tools/include/uapi/linux/bpf.h | 1 +
tools/lib/bpf/bpf_core_read.h | 1 +
3 files changed, 3 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e81362..42605c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6782,6 +6782,7 @@ enum bpf_core_relo_kind {
BPF_CORE_TYPE_SIZE = 9, /* type size in bytes */
BPF_CORE_ENUMVAL_EXISTS = 10, /* enum value existence in target kernel */
BPF_CORE_ENUMVAL_VALUE = 11, /* enum value integer value */
+ BPF_CORE_TYPE_MATCHES = 12, /* type match in target kernel */
};
/*
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e81362..42605c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6782,6 +6782,7 @@ enum bpf_core_relo_kind {
BPF_CORE_TYPE_SIZE = 9, /* type size in bytes */
BPF_CORE_ENUMVAL_EXISTS = 10, /* enum value existence in target kernel */
BPF_CORE_ENUMVAL_VALUE = 11, /* enum value integer value */
+ BPF_CORE_TYPE_MATCHES = 12, /* type match in target kernel */
};
/*
diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
index fd48b1..2308f49 100644
--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -29,6 +29,7 @@ enum bpf_type_id_kind {
enum bpf_type_info_kind {
BPF_TYPE_EXISTS = 0, /* type existence in target kernel */
BPF_TYPE_SIZE = 1, /* type size in target kernel */
+ BPF_TYPE_MATCHES = 2, /* type match in target kernel */
};
/* second argument to __builtin_preserve_enum_value() built-in */
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH bpf-next 2/7] bpftool: Honor BPF_CORE_TYPE_MATCHES relocation
2022-06-20 23:17 [PATCH bpf-next 0/7] Introduce type match support Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 1/7] bpf: Introduce TYPE_MATCH related constants/macros Daniel Müller
@ 2022-06-20 23:17 ` Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 3/7] bpf: Add type match support Daniel Müller
` (4 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-20 23:17 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team; +Cc: Quentin Monnet
bpftool needs to know about the newly introduced BPF_CORE_TYPE_MATCHES
relocation for its 'gen min_core_btf' command to work properly in the
present of this relocation.
Specifically, we need to make sure to mark types and fields so that they
are present in the minimized BTF for "type match" checks to work out.
However, contrary to the existing btfgen_record_field_relo, we need to
rely on the BTF -- and not the spec -- to find fields. With this change
we handle this new variant correctly. The functionality will be tested
with follow on changes to BPF selftests, which already run against a
minimized BTF created with bpftool.
Cc: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Daniel Müller <deso@posteo.net>
---
tools/bpf/bpftool/gen.c | 106 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 106 insertions(+)
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 480cbd8..cf45ce 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1856,6 +1856,110 @@ static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_sp
return 0;
}
+/* Mark types, members, and member types. Compared to btfgen_record_field_relo,
+ * this function does not rely on the target spec for inferring members, but
+ * uses the associated BTF.
+ *
+ * The `behind_ptr` argument is used to stop marking of composite types reached
+ * through a pointer. This way, we keep can keep BTF size in check while
+ * providing reasonable match semantics.
+ */
+static int btfgen_mark_types_match(struct btfgen_info *info, __u32 type_id, bool behind_ptr)
+{
+ const struct btf_type *btf_type;
+ struct btf *btf = info->src_btf;
+ struct btf_type *cloned_type;
+ int i, err;
+
+ if (type_id == 0)
+ return 0;
+
+ btf_type = btf__type_by_id(btf, type_id);
+ /* mark type on cloned BTF as used */
+ cloned_type = (struct btf_type *)btf__type_by_id(info->marked_btf, type_id);
+ cloned_type->name_off = MARKED;
+
+ switch (btf_kind(btf_type)) {
+ case BTF_KIND_UNKN:
+ case BTF_KIND_INT:
+ case BTF_KIND_FLOAT:
+ case BTF_KIND_ENUM:
+ case BTF_KIND_ENUM64:
+ break;
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION: {
+ struct btf_member *m = btf_members(btf_type);
+ __u16 vlen = btf_vlen(btf_type);
+
+ if (behind_ptr)
+ break;
+
+ for (i = 0; i < vlen; i++, m++) {
+ /* mark member */
+ btfgen_mark_member(info, type_id, i);
+
+ /* mark member's type */
+ err = btfgen_mark_types_match(info, m->type, false);
+ if (err)
+ return err;
+ }
+ break;
+ }
+ case BTF_KIND_CONST:
+ case BTF_KIND_VOLATILE:
+ case BTF_KIND_TYPEDEF:
+ return btfgen_mark_types_match(info, btf_type->type, false);
+ case BTF_KIND_PTR:
+ return btfgen_mark_types_match(info, btf_type->type, true);
+ case BTF_KIND_ARRAY: {
+ struct btf_array *array;
+
+ array = btf_array(btf_type);
+ /* mark array type */
+ err = btfgen_mark_types_match(info, array->type, false);
+ /* mark array's index type */
+ err = err ? : btfgen_mark_types_match(info, array->index_type, false);
+ if (err)
+ return err;
+ break;
+ }
+ case BTF_KIND_FUNC_PROTO: {
+ __u16 vlen = btf_vlen(btf_type);
+ struct btf_param *param;
+
+ /* mark ret type */
+ err = btfgen_mark_types_match(info, btf_type->type, false);
+ if (err)
+ return err;
+
+ /* mark parameters types */
+ param = btf_params(btf_type);
+ for (i = 0; i < vlen; i++) {
+ err = btfgen_mark_types_match(info, param->type, false);
+ if (err)
+ return err;
+ param++;
+ }
+ break;
+ }
+ /* tells if some other type needs to be handled */
+ default:
+ p_err("unsupported kind: %s (%d)", btf_kind_str(btf_type), type_id);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* Mark types, members, and member types. Compared to btfgen_record_field_relo,
+ * this function does not rely on the target spec for inferring members, but
+ * uses the associated BTF.
+ */
+static int btfgen_record_types_match_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
+{
+ return btfgen_mark_types_match(info, targ_spec->root_type_id, false);
+}
+
static int btfgen_record_type_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
{
return btfgen_mark_type(info, targ_spec->root_type_id, true);
@@ -1882,6 +1986,8 @@ static int btfgen_record_reloc(struct btfgen_info *info, struct bpf_core_spec *r
case BPF_CORE_TYPE_EXISTS:
case BPF_CORE_TYPE_SIZE:
return btfgen_record_type_relo(info, res);
+ case BPF_CORE_TYPE_MATCHES:
+ return btfgen_record_types_match_relo(info, res);
case BPF_CORE_ENUMVAL_EXISTS:
case BPF_CORE_ENUMVAL_VALUE:
return btfgen_record_enumval_relo(info, res);
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-20 23:17 [PATCH bpf-next 0/7] Introduce type match support Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 1/7] bpf: Introduce TYPE_MATCH related constants/macros Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 2/7] bpftool: Honor BPF_CORE_TYPE_MATCHES relocation Daniel Müller
@ 2022-06-20 23:17 ` Daniel Müller
2022-06-21 19:41 ` Joanne Koong
2022-06-20 23:17 ` [PATCH bpf-next 4/7] libbpf: " Daniel Müller
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Daniel Müller @ 2022-06-20 23:17 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team
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));
+}
+
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);
+
+ return local_len == targ_len && strncmp(local_n, targ_n, local_len) == 0;
+}
+
+static int bpf_core_enums_match(const struct btf *local_btf, const struct btf_type *local_t,
+ 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;
+ 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) {
+ 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,
+ 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,
+ 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;
+
+ 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);
+ } 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);
+
+ if (local_k == targ_k)
+ return 1;
+
+ if (targ_k != BTF_KIND_FWD)
+ return 0;
+
+ 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:
+ 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);
+}
+
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-20 23:17 ` [PATCH bpf-next 3/7] bpf: Add type match support Daniel Müller
@ 2022-06-21 19:41 ` Joanne Koong
2022-06-22 17:22 ` Daniel Müller
0 siblings, 1 reply; 20+ messages in thread
From: Joanne Koong @ 2022-06-21 19:41 UTC (permalink / raw)
To: Daniel Müller
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
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
> +}
> +
> 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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-21 19:41 ` Joanne Koong
@ 2022-06-22 17:22 ` Daniel Müller
2022-06-23 19:19 ` Daniel Müller
2022-06-24 21:09 ` Andrii Nakryiko
0 siblings, 2 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-22 17:22 UTC (permalink / raw)
To: Joanne Koong
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
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!
> > +}
> > +
> > +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.
> > + 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"
> > + 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.
> > + 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?
Same explanation as above. Please let me know if you disagree with the
reasoning.
> > + 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
It basically gets back to the points raised earlier already of us just copying
the signature of existing functionality for consistency while also having the
potential for error returns.
I don't know whether it's good practice, but I do feel that if we change this
function we should change bpf_core_types_are_compat() (and if we go with bool
we'd loose information about potential errors).
> > + 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
I do have the same comment as above. Once moved to relo_core.c, u16 is flagged
by the compiler :-|
> > +
> > + 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
Makes sense. Added.
> > + } 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)?
I did! Good catch. Changed it.
> > +
> > + if (local_k == targ_k)
> > + return 1;
> Why don't we need to check if bpf_core_composites_match() in this case?
We basically agreed that once we reach a composite type that is behind a
pointer, we should stop performing a full member match-up and just check name
and kind and be done. bpf_core_composites_match() would perform the full check
and so we don't use it in this branch.
> > +
> > + 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?
If I understand what you are asking correctly then yes, this case can happen,
but it should not result in a match.
I believe we could hit this case when trying to match up
a_struct* x
with
a_struct** x
We do want to make sure that the same number of indirections are present for a
match to be recorded.
> > +
> > + 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?
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?).
> > + 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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-22 17:22 ` Daniel Müller
@ 2022-06-23 19:19 ` Daniel Müller
2022-06-24 21:09 ` Andrii Nakryiko
1 sibling, 0 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-23 19:19 UTC (permalink / raw)
To: Joanne Koong
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
On Wed, Jun 22, 2022 at 05:22:24PM +0000, Daniel Müller 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:
[...]
> > > + 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?).
So typedefs are all skipped by the logic, see calls to btf_type_skip_modifiers
at the top of the function. That's why we don't need to handle them explicitly.
I should have a revised version addressing the other points up shortly.
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-22 17:22 ` Daniel Müller
2022-06-23 19:19 ` Daniel Müller
@ 2022-06-24 21:09 ` Andrii Nakryiko
2022-06-27 17:34 ` Daniel Müller
2022-06-27 21:03 ` Daniel Müller
1 sibling, 2 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2022-06-24 21:09 UTC (permalink / raw)
To: Daniel Müller
Cc: Joanne Koong, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-24 21:09 ` Andrii Nakryiko
@ 2022-06-27 17:34 ` Daniel Müller
2022-06-27 21:03 ` Daniel Müller
1 sibling, 0 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-27 17:34 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Joanne Koong, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team
On Fri, Jun 24, 2022 at 02:09:33PM -0700, Andrii Nakryiko wrote:
> 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:
> >
> > > > +
> > > > + 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.
Ah, I actually misread the request to be for keeping strncmp(), but omitting the
explicit length equality check beforehand. That's how I updated the code.
[...]
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-24 21:09 ` Andrii Nakryiko
2022-06-27 17:34 ` Daniel Müller
@ 2022-06-27 21:03 ` Daniel Müller
2022-06-27 21:12 ` Andrii Nakryiko
1 sibling, 1 reply; 20+ messages in thread
From: Daniel Müller @ 2022-06-27 21:03 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Joanne Koong, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team
On Fri, Jun 24, 2022 at 02:09:33PM -0700, Andrii Nakryiko wrote:
> 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:
> > > 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).
[...]
Thanks for clarifying. Let's leave FLOAT support for follow-on changes, though,
and not bloat this patch set unnecessarily. It's not currently support by the
existing libbpf/kernel checks or by bpftool's BTF minimization logic from what I
can tell -- preferably all of which would need to be updated, tests be added,
etc. This is entirely orthogonal to what is being added here from my
perspective.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next 3/7] bpf: Add type match support
2022-06-27 21:03 ` Daniel Müller
@ 2022-06-27 21:12 ` Andrii Nakryiko
0 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2022-06-27 21:12 UTC (permalink / raw)
To: Daniel Müller
Cc: Joanne Koong, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team
On Mon, Jun 27, 2022 at 2:03 PM Daniel Müller <deso@posteo.net> wrote:
>
> On Fri, Jun 24, 2022 at 02:09:33PM -0700, Andrii Nakryiko wrote:
> > 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:
> > > > 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).
>
> [...]
>
> Thanks for clarifying. Let's leave FLOAT support for follow-on changes, though,
> and not bloat this patch set unnecessarily. It's not currently support by the
> existing libbpf/kernel checks or by bpftool's BTF minimization logic from what I
it seems to be handled by bpf_core_fields_are_compat(), but yeah, we
can do a follow up for this
> can tell -- preferably all of which would need to be updated, tests be added,
> etc. This is entirely orthogonal to what is being added here from my
> perspective.
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH bpf-next 4/7] libbpf: Add type match support
2022-06-20 23:17 [PATCH bpf-next 0/7] Introduce type match support Daniel Müller
` (2 preceding siblings ...)
2022-06-20 23:17 ` [PATCH bpf-next 3/7] bpf: Add type match support Daniel Müller
@ 2022-06-20 23:17 ` Daniel Müller
2022-06-20 23:59 ` Alexei Starovoitov
2022-06-20 23:17 ` [PATCH bpf-next 5/7] selftests/bpf: Add type-match checks to type-based tests Daniel Müller
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Daniel Müller @ 2022-06-20 23:17 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team
This patch adds support for the proposed type match relation to libbpf.
Support of this relation hinges on the corresponding relocation to be
understood by LLVM, which is happening as part of D126838
(https://reviews.llvm.org/D126838).
The functionality is present behind the newly introduced
bpf_core_type_matches macro (similar to bpf_core_type_exists). This
macro can be used to check whether a local type has a "match" in a
target BTF.
The matching relation is defined as follows (copy from source):
- modifiers and typedefs are stripped (and, hence, effectively ignored)
- generally speaking types need to be of same kind (struct vs. struct, union
vs. union, etc.)
- exceptions are struct/union behind a pointer which could also match a
forward declaration of a struct or union, respectively, and enum vs.
enum64 (see below)
Then, depending on type:
- integers:
- match if size and signedness match
- arrays & pointers:
- target types are recursively matched
- structs & unions:
- local members need to exist in target with the same name
- for each member we recursively check match unless it is already behind a
pointer, in which case we only check matching names and compatible kind
- enums:
- local variants have to have a match in target by symbolic name (but not
numeric value)
- size has to match (but enum may match enum64 and vice versa)
- function pointers:
- number and position of arguments in local type has to match target
- for each argument and the return value we recursively check match
Signed-off-by: Daniel Müller <deso@posteo.net>
---
tools/lib/bpf/bpf_core_read.h | 10 ++
tools/lib/bpf/libbpf.c | 277 ++++++++++++++++++++++++++++++++++
tools/lib/bpf/relo_core.c | 16 +-
tools/lib/bpf/relo_core.h | 2 +
4 files changed, 301 insertions(+), 4 deletions(-)
diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
index 2308f49..496e6a 100644
--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -184,6 +184,16 @@ enum bpf_enum_value_kind {
#define bpf_core_type_exists(type) \
__builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_EXISTS)
+/*
+ * Convenience macro to check that provided named type
+ * (struct/union/enum/typedef) "matches" that in a target kernel.
+ * Returns:
+ * 1, if the type matches in the target kernel's BTF;
+ * 0, if the type does not match any in the target kernel
+ */
+#define bpf_core_type_matches(type) \
+ __builtin_preserve_type_info(*(typeof(type) *)0, BPF_TYPE_MATCHES)
+
/*
* Convenience macro to get the byte size of a provided named type
* (struct/union/enum/typedef) in a target kernel.
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 49e359c..8f7674 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5805,6 +5805,283 @@ int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
}
}
+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);
+
+ return local_len == targ_len && strncmp(local_n, targ_n, local_len) == 0;
+}
+
+static int bpf_core_enums_match(const struct btf *local_btf, const struct btf_type *local_t,
+ 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;
+ size_t local_len;
+
+ local_n_off = btf_is_enum(local_t) ? btf_enum(local_t)[i].name_off :
+ btf_enum64(local_t)[i].name_off;
+
+ local_n = btf__str_by_offset(local_btf, local_n_off);
+ local_len = bpf_core_essential_name_len(local_n);
+
+ for (j = 0; j < targ_vlen; j++) {
+ __u32 targ_n_off;
+ const char *targ_n;
+ size_t targ_len;
+
+ targ_n_off = btf_is_enum(targ_t) ? btf_enum(targ_t)[j].name_off :
+ btf_enum64(targ_t)[j].name_off;
+ targ_n = btf__str_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) {
+ matched = true;
+ break;
+ }
+ }
+
+ if (!matched)
+ return 0;
+ }
+ return 1;
+}
+
+static int bpf_core_composites_match(const struct btf *local_btf, const struct btf_type *local_t,
+ const struct btf *targ_btf, const struct btf_type *targ_t)
+{
+ 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;
+
+ /* check that all local members have a match in the target */
+ for (i = 0; i < local_vlen; i++, local_m++) {
+ const char *local_n = btf__str_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__str_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);
+ if (err > 0) {
+ matched = true;
+ break;
+ }
+ }
+
+ if (!matched)
+ return 0;
+ }
+ return 1;
+}
+
+/* Check that two types "match".
+ *
+ * The matching relation is defined as follows:
+ * - modifiers and typedefs are stripped (and, hence, effectively ignored)
+ * - generally speaking types need to be of same kind (struct vs. struct, union
+ * vs. union, etc.)
+ * - exceptions are struct/union behind a pointer which could also match a
+ * forward declaration of a struct or union, respectively, and enum vs.
+ * enum64 (see below)
+ * Then, depending on type:
+ * - integers:
+ * - match if size and signedness match
+ * - arrays & pointers:
+ * - target types are recursively matched
+ * - structs & unions:
+ * - local members need to exist in target with the same name
+ * - for each member we recursively check match unless it is already behind a
+ * pointer, in which case we only check matching names and compatible kind
+ * - enums:
+ * - local variants have to have a match in target by symbolic name (but not
+ * numeric value)
+ * - size has to match (but enum may match enum64 and vice versa)
+ * - function pointers:
+ * - number and position of arguments in local type has to match target
+ * - for each argument and the return value we recursively check match
+ */
+int bpf_core_types_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, *prev_local_t;
+ int depth = 32; /* max recursion depth */
+ __u16 local_k;
+
+ 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 = skip_mods_and_typedefs(local_btf, local_id, &local_id);
+ targ_t = skip_mods_and_typedefs(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_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_kflag(local_t);
+
+ return (targ_k == BTF_KIND_STRUCT && !local_f) ||
+ (targ_k == BTF_KIND_UNION && local_f);
+ } else {
+ if (local_k != targ_k)
+ return 0;
+
+ /* match if the forward declaration is for the same kind */
+ return local_f == btf_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_kflag(local_t);
+
+ if (local_k == targ_k)
+ return 1;
+
+ if (targ_k != BTF_KIND_FWD)
+ return 0;
+
+ 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);
+ }
+ }
+ 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_array(local_t);
+ const struct btf_array *targ_array = btf_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);
+ if (err <= 0)
+ return err;
+ }
+
+ /* tail recurse for return type check */
+ local_id = local_t->type;
+ targ_id = targ_t->type;
+ goto recur;
+ }
+ default:
+ pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n",
+ btf_kind_str(local_t), local_id, targ_id);
+ return 0;
+ }
+}
+
static size_t bpf_core_hash_fn(const void *key, void *ctx)
{
return (size_t)key;
diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
index 6ad3c3..bb9b67a 100644
--- a/tools/lib/bpf/relo_core.c
+++ b/tools/lib/bpf/relo_core.c
@@ -95,6 +95,7 @@ static const char *core_relo_kind_str(enum bpf_core_relo_kind kind)
case BPF_CORE_TYPE_ID_LOCAL: return "local_type_id";
case BPF_CORE_TYPE_ID_TARGET: return "target_type_id";
case BPF_CORE_TYPE_EXISTS: return "type_exists";
+ case BPF_CORE_TYPE_MATCHES: return "type_matches";
case BPF_CORE_TYPE_SIZE: return "type_size";
case BPF_CORE_ENUMVAL_EXISTS: return "enumval_exists";
case BPF_CORE_ENUMVAL_VALUE: return "enumval_value";
@@ -123,6 +124,7 @@ static bool core_relo_is_type_based(enum bpf_core_relo_kind kind)
case BPF_CORE_TYPE_ID_LOCAL:
case BPF_CORE_TYPE_ID_TARGET:
case BPF_CORE_TYPE_EXISTS:
+ case BPF_CORE_TYPE_MATCHES:
case BPF_CORE_TYPE_SIZE:
return true;
default:
@@ -171,7 +173,7 @@ static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind)
* - field 'a' access (corresponds to '2' in low-level spec);
* - array element #3 access (corresponds to '3' in low-level spec).
*
- * Type-based relocations (TYPE_EXISTS/TYPE_SIZE,
+ * Type-based relocations (TYPE_EXISTS/TYPE_MATCHES/TYPE_SIZE,
* TYPE_ID_LOCAL/TYPE_ID_TARGET) don't capture any field information. Their
* spec and raw_spec are kept empty.
*
@@ -488,9 +490,14 @@ static int bpf_core_spec_match(struct bpf_core_spec *local_spec,
targ_spec->relo_kind = local_spec->relo_kind;
if (core_relo_is_type_based(local_spec->relo_kind)) {
- return bpf_core_types_are_compat(local_spec->btf,
- local_spec->root_type_id,
- targ_btf, targ_id);
+ if (local_spec->relo_kind == BPF_CORE_TYPE_MATCHES)
+ return bpf_core_types_match(local_spec->btf,
+ local_spec->root_type_id,
+ targ_btf, targ_id);
+ else
+ return bpf_core_types_are_compat(local_spec->btf,
+ local_spec->root_type_id,
+ targ_btf, targ_id);
}
local_acc = &local_spec->spec[0];
@@ -739,6 +746,7 @@ static int bpf_core_calc_type_relo(const struct bpf_core_relo *relo,
*validate = false;
break;
case BPF_CORE_TYPE_EXISTS:
+ case BPF_CORE_TYPE_MATCHES:
*val = 1;
break;
case BPF_CORE_TYPE_SIZE:
diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
index 7df0da0..6b6afa 100644
--- a/tools/lib/bpf/relo_core.h
+++ b/tools/lib/bpf/relo_core.h
@@ -70,6 +70,8 @@ struct bpf_core_relo_res {
int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
const struct btf *targ_btf, __u32 targ_id);
+int bpf_core_types_match(const struct btf *local_btf, __u32 local_id,
+ const struct btf *targ_btf, __u32 targ_id);
size_t bpf_core_essential_name_len(const char *name);
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 4/7] libbpf: Add type match support
2022-06-20 23:17 ` [PATCH bpf-next 4/7] libbpf: " Daniel Müller
@ 2022-06-20 23:59 ` Alexei Starovoitov
2022-06-21 16:45 ` Daniel Müller
0 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2022-06-20 23:59 UTC (permalink / raw)
To: Daniel Müller; +Cc: bpf, ast, andrii, daniel, kernel-team
On Mon, Jun 20, 2022 at 11:17:10PM +0000, Daniel Müller wrote:
> +int bpf_core_types_match(const struct btf *local_btf, __u32 local_id,
> + const struct btf *targ_btf, __u32 targ_id)
> +{
The libbpf and kernel support for types_match looks nearly identical.
Maybe put in tools/lib/bpf/relo_core.c so it's one copy for both?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 4/7] libbpf: Add type match support
2022-06-20 23:59 ` Alexei Starovoitov
@ 2022-06-21 16:45 ` Daniel Müller
2022-06-21 18:44 ` Alexei Starovoitov
0 siblings, 1 reply; 20+ messages in thread
From: Daniel Müller @ 2022-06-21 16:45 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: bpf, ast, andrii, daniel, kernel-team
On Mon, Jun 20, 2022 at 04:59:19PM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 20, 2022 at 11:17:10PM +0000, Daniel Müller wrote:
> > +int bpf_core_types_match(const struct btf *local_btf, __u32 local_id,
> > + const struct btf *targ_btf, __u32 targ_id)
> > +{
>
> The libbpf and kernel support for types_match looks nearly identical.
> Maybe put in tools/lib/bpf/relo_core.c so it's one copy for both?
Thanks for the suggestion. Yes, at least for parts we should probably do it.
Would you happen to know why that has not been done for
bpf_core_types_are_compat equally? Is it because of the recursion level
tracking that is only present in the kernel? I'd think that similar reasoning
applies here.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 4/7] libbpf: Add type match support
2022-06-21 16:45 ` Daniel Müller
@ 2022-06-21 18:44 ` Alexei Starovoitov
2022-06-21 21:10 ` Daniel Müller
0 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2022-06-21 18:44 UTC (permalink / raw)
To: Daniel Müller
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team
On Tue, Jun 21, 2022 at 9:46 AM Daniel Müller <deso@posteo.net> wrote:
>
> On Mon, Jun 20, 2022 at 04:59:19PM -0700, Alexei Starovoitov wrote:
> > On Mon, Jun 20, 2022 at 11:17:10PM +0000, Daniel Müller wrote:
> > > +int bpf_core_types_match(const struct btf *local_btf, __u32 local_id,
> > > + const struct btf *targ_btf, __u32 targ_id)
> > > +{
> >
> > The libbpf and kernel support for types_match looks nearly identical.
> > Maybe put in tools/lib/bpf/relo_core.c so it's one copy for both?
>
> Thanks for the suggestion. Yes, at least for parts we should probably do it.
>
> Would you happen to know why that has not been done for
> bpf_core_types_are_compat equally? Is it because of the recursion level
> tracking that is only present in the kernel? I'd think that similar reasoning
> applies here.
Historical. Probably should be combined.
Code duplication is the source of all kinds of maintenance issues
and subtle bugs.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 4/7] libbpf: Add type match support
2022-06-21 18:44 ` Alexei Starovoitov
@ 2022-06-21 21:10 ` Daniel Müller
2022-06-21 21:14 ` Alexei Starovoitov
0 siblings, 1 reply; 20+ messages in thread
From: Daniel Müller @ 2022-06-21 21:10 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Yonghong Song
On Tue, Jun 21, 2022 at 11:44:17AM -0700, Alexei Starovoitov wrote:
> On Tue, Jun 21, 2022 at 9:46 AM Daniel Müller <deso@posteo.net> wrote:
> >
> > On Mon, Jun 20, 2022 at 04:59:19PM -0700, Alexei Starovoitov wrote:
> > > On Mon, Jun 20, 2022 at 11:17:10PM +0000, Daniel Müller wrote:
> > > > +int bpf_core_types_match(const struct btf *local_btf, __u32 local_id,
> > > > + const struct btf *targ_btf, __u32 targ_id)
> > > > +{
> > >
> > > The libbpf and kernel support for types_match looks nearly identical.
> > > Maybe put in tools/lib/bpf/relo_core.c so it's one copy for both?
> >
> > Thanks for the suggestion. Yes, at least for parts we should probably do it.
> >
> > Would you happen to know why that has not been done for
> > bpf_core_types_are_compat equally? Is it because of the recursion level
> > tracking that is only present in the kernel? I'd think that similar reasoning
> > applies here.
>
> Historical. Probably should be combined.
> Code duplication is the source of all kinds of maintenance issues
> and subtle bugs.
Certainly. I noticed that btf.c's bpf_core_types_are_compat uses direct equality
check of the local and target kind while libbpf.c's version utilizes
btf_kind_core_compat, which treats enum and enum64 as compatible. I suspect that
may be a bug in the former.
Will move the implementation then. Thanks!
Cc: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH bpf-next 4/7] libbpf: Add type match support
2022-06-21 21:10 ` Daniel Müller
@ 2022-06-21 21:14 ` Alexei Starovoitov
0 siblings, 0 replies; 20+ messages in thread
From: Alexei Starovoitov @ 2022-06-21 21:14 UTC (permalink / raw)
To: Daniel Müller
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Yonghong Song
On Tue, Jun 21, 2022 at 2:11 PM Daniel Müller <deso@posteo.net> wrote:
>
> On Tue, Jun 21, 2022 at 11:44:17AM -0700, Alexei Starovoitov wrote:
> > On Tue, Jun 21, 2022 at 9:46 AM Daniel Müller <deso@posteo.net> wrote:
> > >
> > > On Mon, Jun 20, 2022 at 04:59:19PM -0700, Alexei Starovoitov wrote:
> > > > On Mon, Jun 20, 2022 at 11:17:10PM +0000, Daniel Müller wrote:
> > > > > +int bpf_core_types_match(const struct btf *local_btf, __u32 local_id,
> > > > > + const struct btf *targ_btf, __u32 targ_id)
> > > > > +{
> > > >
> > > > The libbpf and kernel support for types_match looks nearly identical.
> > > > Maybe put in tools/lib/bpf/relo_core.c so it's one copy for both?
> > >
> > > Thanks for the suggestion. Yes, at least for parts we should probably do it.
> > >
> > > Would you happen to know why that has not been done for
> > > bpf_core_types_are_compat equally? Is it because of the recursion level
> > > tracking that is only present in the kernel? I'd think that similar reasoning
> > > applies here.
> >
> > Historical. Probably should be combined.
> > Code duplication is the source of all kinds of maintenance issues
> > and subtle bugs.
>
> Certainly. I noticed that btf.c's bpf_core_types_are_compat uses direct equality
> check of the local and target kind while libbpf.c's version utilizes
> btf_kind_core_compat, which treats enum and enum64 as compatible. I suspect that
> may be a bug in the former.
Great catch. Indeed that does look like a bug that slipped in
due to code duplication :(
> Will move the implementation then. Thanks!
>
> Cc: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH bpf-next 5/7] selftests/bpf: Add type-match checks to type-based tests
2022-06-20 23:17 [PATCH bpf-next 0/7] Introduce type match support Daniel Müller
` (3 preceding siblings ...)
2022-06-20 23:17 ` [PATCH bpf-next 4/7] libbpf: " Daniel Müller
@ 2022-06-20 23:17 ` Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 6/7] selftests/bpf: Add test checking more characteristics Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 7/7] selftests/bpf: Add nested type to type based tests Daniel Müller
6 siblings, 0 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-20 23:17 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team
Now that we have type-match logic in both libbpf and the kernel, this
change adjusts the existing BPF self tests to check this functionality.
Specifically, we extend the existing type-based tests to check the
previously introduced bpf_core_type_matches macro.
Signed-off-by: Daniel Müller <deso@posteo.net>
---
.../selftests/bpf/prog_tests/core_reloc.c | 31 ++++++++++++++++--
.../selftests/bpf/progs/core_reloc_types.h | 14 +++++++-
.../bpf/progs/test_core_reloc_type_based.c | 32 ++++++++++++++++++-
3 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index 2f92fe..328dd7 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -543,7 +543,6 @@ static int __trigger_module_test_read(const struct core_reloc_test_case *test)
return 0;
}
-
static const struct core_reloc_test_case test_cases[] = {
/* validate we can find kernel image and use its BTF for relocs */
{
@@ -752,7 +751,7 @@ static const struct core_reloc_test_case test_cases[] = {
SIZE_CASE(size___diff_offs),
SIZE_ERR_CASE(size___err_ambiguous),
- /* validate type existence and size relocations */
+ /* validate type existence, match, and size relocations */
TYPE_BASED_CASE(type_based, {
.struct_exists = 1,
.union_exists = 1,
@@ -765,6 +764,19 @@ static const struct core_reloc_test_case test_cases[] = {
.typedef_void_ptr_exists = 1,
.typedef_func_proto_exists = 1,
.typedef_arr_exists = 1,
+
+ .struct_matches = 1,
+ .union_matches = 1,
+ .enum_matches = 1,
+ .typedef_named_struct_matches = 1,
+ .typedef_anon_struct_matches = 1,
+ .typedef_struct_ptr_matches = 1,
+ .typedef_int_matches = 1,
+ .typedef_enum_matches = 1,
+ .typedef_void_ptr_matches = 1,
+ .typedef_func_proto_matches = 1,
+ .typedef_arr_matches = 1,
+
.struct_sz = sizeof(struct a_struct),
.union_sz = sizeof(union a_union),
.enum_sz = sizeof(enum an_enum),
@@ -792,6 +804,19 @@ static const struct core_reloc_test_case test_cases[] = {
.typedef_void_ptr_exists = 1,
.typedef_func_proto_exists = 1,
.typedef_arr_exists = 1,
+
+ .struct_matches = 0,
+ .union_matches = 0,
+ .enum_matches = 0,
+ .typedef_named_struct_matches = 0,
+ .typedef_anon_struct_matches = 0,
+ .typedef_struct_ptr_matches = 1,
+ .typedef_int_matches = 0,
+ .typedef_enum_matches = 0,
+ .typedef_void_ptr_matches = 1,
+ .typedef_func_proto_matches = 0,
+ .typedef_arr_matches = 0,
+
.struct_sz = sizeof(struct a_struct___diff_sz),
.union_sz = sizeof(union a_union___diff_sz),
.enum_sz = sizeof(enum an_enum___diff_sz),
@@ -806,10 +831,12 @@ static const struct core_reloc_test_case test_cases[] = {
}),
TYPE_BASED_CASE(type_based___incompat, {
.enum_exists = 1,
+ .enum_matches = 1,
.enum_sz = sizeof(enum an_enum),
}),
TYPE_BASED_CASE(type_based___fn_wrong_args, {
.struct_exists = 1,
+ .struct_matches = 1,
.struct_sz = sizeof(struct a_struct),
}),
diff --git a/tools/testing/selftests/bpf/progs/core_reloc_types.h b/tools/testing/selftests/bpf/progs/core_reloc_types.h
index 26e1033..6a44f3e 100644
--- a/tools/testing/selftests/bpf/progs/core_reloc_types.h
+++ b/tools/testing/selftests/bpf/progs/core_reloc_types.h
@@ -860,7 +860,7 @@ struct core_reloc_size___err_ambiguous2 {
};
/*
- * TYPE EXISTENCE & SIZE
+ * TYPE EXISTENCE, MATCH & SIZE
*/
struct core_reloc_type_based_output {
bool struct_exists;
@@ -875,6 +875,18 @@ struct core_reloc_type_based_output {
bool typedef_func_proto_exists;
bool typedef_arr_exists;
+ bool struct_matches;
+ bool union_matches;
+ bool enum_matches;
+ bool typedef_named_struct_matches;
+ bool typedef_anon_struct_matches;
+ bool typedef_struct_ptr_matches;
+ bool typedef_int_matches;
+ bool typedef_enum_matches;
+ bool typedef_void_ptr_matches;
+ bool typedef_func_proto_matches;
+ bool typedef_arr_matches;
+
int struct_sz;
int union_sz;
int enum_sz;
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c b/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c
index fb60f8..325ead 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c
@@ -61,6 +61,18 @@ struct core_reloc_type_based_output {
bool typedef_func_proto_exists;
bool typedef_arr_exists;
+ bool struct_matches;
+ bool union_matches;
+ bool enum_matches;
+ bool typedef_named_struct_matches;
+ bool typedef_anon_struct_matches;
+ bool typedef_struct_ptr_matches;
+ bool typedef_int_matches;
+ bool typedef_enum_matches;
+ bool typedef_void_ptr_matches;
+ bool typedef_func_proto_matches;
+ bool typedef_arr_matches;
+
int struct_sz;
int union_sz;
int enum_sz;
@@ -77,7 +89,13 @@ struct core_reloc_type_based_output {
SEC("raw_tracepoint/sys_enter")
int test_core_type_based(void *ctx)
{
-#if __has_builtin(__builtin_preserve_type_info)
+ /* Support for the BPF_TYPE_MATCHES argument to the
+ * __builtin_preserve_type_info builtin was added at some point during
+ * development of clang 15 and it's what we require for this test. Part of it
+ * could run with merely __builtin_preserve_type_info (which could be checked
+ * separately), but we have to find an upper bound.
+ */
+#if __has_builtin(__builtin_preserve_type_info) && __clang_major__ >= 15
struct core_reloc_type_based_output *out = (void *)&data.out;
out->struct_exists = bpf_core_type_exists(struct a_struct);
@@ -92,6 +110,18 @@ int test_core_type_based(void *ctx)
out->typedef_func_proto_exists = bpf_core_type_exists(func_proto_typedef);
out->typedef_arr_exists = bpf_core_type_exists(arr_typedef);
+ out->struct_matches = bpf_core_type_matches(struct a_struct);
+ out->union_matches = bpf_core_type_matches(union a_union);
+ out->enum_matches = bpf_core_type_matches(enum an_enum);
+ out->typedef_named_struct_matches = bpf_core_type_matches(named_struct_typedef);
+ out->typedef_anon_struct_matches = bpf_core_type_matches(anon_struct_typedef);
+ out->typedef_struct_ptr_matches = bpf_core_type_matches(struct_ptr_typedef);
+ out->typedef_int_matches = bpf_core_type_matches(int_typedef);
+ out->typedef_enum_matches = bpf_core_type_matches(enum_typedef);
+ out->typedef_void_ptr_matches = bpf_core_type_matches(void_ptr_typedef);
+ out->typedef_func_proto_matches = bpf_core_type_matches(func_proto_typedef);
+ out->typedef_arr_matches = bpf_core_type_matches(arr_typedef);
+
out->struct_sz = bpf_core_type_size(struct a_struct);
out->union_sz = bpf_core_type_size(union a_union);
out->enum_sz = bpf_core_type_size(enum an_enum);
--
2.30.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH bpf-next 6/7] selftests/bpf: Add test checking more characteristics
2022-06-20 23:17 [PATCH bpf-next 0/7] Introduce type match support Daniel Müller
` (4 preceding siblings ...)
2022-06-20 23:17 ` [PATCH bpf-next 5/7] selftests/bpf: Add type-match checks to type-based tests Daniel Müller
@ 2022-06-20 23:17 ` Daniel Müller
2022-06-20 23:17 ` [PATCH bpf-next 7/7] selftests/bpf: Add nested type to type based tests Daniel Müller
6 siblings, 0 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-20 23:17 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team
This change adds another type-based self-test that specifically aims to
test some more characteristics of the TYPE_MATCH logic. Specifically, it
covers a few more potential differences between types, such as different
orders, enum variant values, and integer signedness.
Signed-off-by: Daniel Müller <deso@posteo.net>
---
.../selftests/bpf/prog_tests/core_reloc.c | 37 ++++++++++++++
.../progs/btf__core_reloc_type_based___diff.c | 3 ++
.../selftests/bpf/progs/core_reloc_types.h | 51 +++++++++++++++++++
3 files changed, 91 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_type_based___diff.c
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index 328dd7..eb47bf 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -792,6 +792,43 @@ static const struct core_reloc_test_case test_cases[] = {
TYPE_BASED_CASE(type_based___all_missing, {
/* all zeros */
}),
+ TYPE_BASED_CASE(type_based___diff, {
+ .struct_exists = 1,
+ .union_exists = 1,
+ .enum_exists = 1,
+ .typedef_named_struct_exists = 1,
+ .typedef_anon_struct_exists = 1,
+ .typedef_struct_ptr_exists = 1,
+ .typedef_int_exists = 1,
+ .typedef_enum_exists = 1,
+ .typedef_void_ptr_exists = 1,
+ .typedef_func_proto_exists = 1,
+ .typedef_arr_exists = 1,
+
+ .struct_matches = 1,
+ .union_matches = 1,
+ .enum_matches = 1,
+ .typedef_named_struct_matches = 1,
+ .typedef_anon_struct_matches = 1,
+ .typedef_struct_ptr_matches = 1,
+ .typedef_int_matches = 0,
+ .typedef_enum_matches = 1,
+ .typedef_void_ptr_matches = 1,
+ .typedef_func_proto_matches = 0,
+ .typedef_arr_matches = 0,
+
+ .struct_sz = sizeof(struct a_struct___diff),
+ .union_sz = sizeof(union a_union___diff),
+ .enum_sz = sizeof(enum an_enum___diff),
+ .typedef_named_struct_sz = sizeof(named_struct_typedef___diff),
+ .typedef_anon_struct_sz = sizeof(anon_struct_typedef___diff),
+ .typedef_struct_ptr_sz = sizeof(struct_ptr_typedef___diff),
+ .typedef_int_sz = sizeof(int_typedef___diff),
+ .typedef_enum_sz = sizeof(enum_typedef___diff),
+ .typedef_void_ptr_sz = sizeof(void_ptr_typedef___diff),
+ .typedef_func_proto_sz = sizeof(func_proto_typedef___diff),
+ .typedef_arr_sz = sizeof(arr_typedef___diff),
+ }),
TYPE_BASED_CASE(type_based___diff_sz, {
.struct_exists = 1,
.union_exists = 1,
diff --git a/tools/testing/selftests/bpf/progs/btf__core_reloc_type_based___diff.c b/tools/testing/selftests/bpf/progs/btf__core_reloc_type_based___diff.c
new file mode 100644
index 0000000..57ae2c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btf__core_reloc_type_based___diff.c
@@ -0,0 +1,3 @@
+#include "core_reloc_types.h"
+
+void f(struct core_reloc_type_based___diff x) {}
diff --git a/tools/testing/selftests/bpf/progs/core_reloc_types.h b/tools/testing/selftests/bpf/progs/core_reloc_types.h
index 6a44f3e..e326b6 100644
--- a/tools/testing/selftests/bpf/progs/core_reloc_types.h
+++ b/tools/testing/selftests/bpf/progs/core_reloc_types.h
@@ -951,6 +951,57 @@ struct core_reloc_type_based {
struct core_reloc_type_based___all_missing {
};
+/* different member orders, enum variant values, signedness, etc */
+struct a_struct___diff {
+ int x;
+ int a;
+};
+
+union a_union___diff {
+ int z;
+ int y;
+};
+
+typedef struct a_struct___diff named_struct_typedef___diff;
+
+typedef struct { int z, x, y; } anon_struct_typedef___diff;
+
+typedef struct {
+ int c;
+ int b;
+ int a;
+} *struct_ptr_typedef___diff;
+
+enum an_enum___diff {
+ AN_ENUM_VAL2___diff = 0,
+ AN_ENUM_VAL1___diff = 42,
+ AN_ENUM_VAL3___diff = 1,
+};
+
+typedef unsigned int int_typedef___diff;
+
+typedef enum { TYPEDEF_ENUM_VAL2___diff, TYPEDEF_ENUM_VAL1___diff = 50 } enum_typedef___diff;
+
+typedef const void *void_ptr_typedef___diff;
+
+typedef int_typedef___diff (*func_proto_typedef___diff)(long);
+
+typedef char arr_typedef___diff[3];
+
+struct core_reloc_type_based___diff {
+ struct a_struct___diff f1;
+ union a_union___diff f2;
+ enum an_enum___diff f3;
+ named_struct_typedef___diff f4;
+ anon_struct_typedef___diff f5;
+ struct_ptr_typedef___diff f6;
+ int_typedef___diff f7;
+ enum_typedef___diff f8;
+ void_ptr_typedef___diff f9;
+ func_proto_typedef___diff f10;
+ arr_typedef___diff f11;
+};
+
/* different type sizes, extra modifiers, anon vs named enums, etc */
struct a_struct___diff_sz {
long x;
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH bpf-next 7/7] selftests/bpf: Add nested type to type based tests
2022-06-20 23:17 [PATCH bpf-next 0/7] Introduce type match support Daniel Müller
` (5 preceding siblings ...)
2022-06-20 23:17 ` [PATCH bpf-next 6/7] selftests/bpf: Add test checking more characteristics Daniel Müller
@ 2022-06-20 23:17 ` Daniel Müller
6 siblings, 0 replies; 20+ messages in thread
From: Daniel Müller @ 2022-06-20 23:17 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team
This change extends the type based tests with another struct type (in
addition to a_struct) to check relocations against: a_complex_struct.
This type is nested more deeply to provide additional coverage of
certain paths in the type match logic.
Signed-off-by: Daniel Müller <deso@posteo.net>
---
.../selftests/bpf/prog_tests/core_reloc.c | 4 ++
.../selftests/bpf/progs/core_reloc_types.h | 62 +++++++++++++------
.../bpf/progs/test_core_reloc_type_based.c | 12 ++++
3 files changed, 58 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index eb47bf..8882c9c 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -754,6 +754,7 @@ static const struct core_reloc_test_case test_cases[] = {
/* validate type existence, match, and size relocations */
TYPE_BASED_CASE(type_based, {
.struct_exists = 1,
+ .complex_struct_exists = 1,
.union_exists = 1,
.enum_exists = 1,
.typedef_named_struct_exists = 1,
@@ -766,6 +767,7 @@ static const struct core_reloc_test_case test_cases[] = {
.typedef_arr_exists = 1,
.struct_matches = 1,
+ .complex_struct_matches = 1,
.union_matches = 1,
.enum_matches = 1,
.typedef_named_struct_matches = 1,
@@ -794,6 +796,7 @@ static const struct core_reloc_test_case test_cases[] = {
}),
TYPE_BASED_CASE(type_based___diff, {
.struct_exists = 1,
+ .complex_struct_exists = 1,
.union_exists = 1,
.enum_exists = 1,
.typedef_named_struct_exists = 1,
@@ -806,6 +809,7 @@ static const struct core_reloc_test_case test_cases[] = {
.typedef_arr_exists = 1,
.struct_matches = 1,
+ .complex_struct_matches = 1,
.union_matches = 1,
.enum_matches = 1,
.typedef_named_struct_matches = 1,
diff --git a/tools/testing/selftests/bpf/progs/core_reloc_types.h b/tools/testing/selftests/bpf/progs/core_reloc_types.h
index e326b6..aa265c3 100644
--- a/tools/testing/selftests/bpf/progs/core_reloc_types.h
+++ b/tools/testing/selftests/bpf/progs/core_reloc_types.h
@@ -864,6 +864,7 @@ struct core_reloc_size___err_ambiguous2 {
*/
struct core_reloc_type_based_output {
bool struct_exists;
+ bool complex_struct_exists;
bool union_exists;
bool enum_exists;
bool typedef_named_struct_exists;
@@ -876,6 +877,7 @@ struct core_reloc_type_based_output {
bool typedef_arr_exists;
bool struct_matches;
+ bool complex_struct_matches;
bool union_matches;
bool enum_matches;
bool typedef_named_struct_matches;
@@ -904,6 +906,14 @@ struct a_struct {
int x;
};
+struct a_complex_struct {
+ union {
+ struct a_struct *a;
+ void *b;
+ } x;
+ volatile long y;
+};
+
union a_union {
int y;
int z;
@@ -935,16 +945,17 @@ typedef char arr_typedef[20];
struct core_reloc_type_based {
struct a_struct f1;
- union a_union f2;
- enum an_enum f3;
- named_struct_typedef f4;
- anon_struct_typedef f5;
- struct_ptr_typedef f6;
- int_typedef f7;
- enum_typedef f8;
- void_ptr_typedef f9;
- func_proto_typedef f10;
- arr_typedef f11;
+ struct a_complex_struct f2;
+ union a_union f3;
+ enum an_enum f4;
+ named_struct_typedef f5;
+ anon_struct_typedef f6;
+ struct_ptr_typedef f7;
+ int_typedef f8;
+ enum_typedef f9;
+ void_ptr_typedef f10;
+ func_proto_typedef f11;
+ arr_typedef f12;
};
/* no types in target */
@@ -957,6 +968,16 @@ struct a_struct___diff {
int a;
};
+struct a_struct___forward;
+
+struct a_complex_struct___diff {
+ union {
+ struct a_struct___forward *a;
+ void *b;
+ } x;
+ volatile long y;
+};
+
union a_union___diff {
int z;
int y;
@@ -990,16 +1011,17 @@ typedef char arr_typedef___diff[3];
struct core_reloc_type_based___diff {
struct a_struct___diff f1;
- union a_union___diff f2;
- enum an_enum___diff f3;
- named_struct_typedef___diff f4;
- anon_struct_typedef___diff f5;
- struct_ptr_typedef___diff f6;
- int_typedef___diff f7;
- enum_typedef___diff f8;
- void_ptr_typedef___diff f9;
- func_proto_typedef___diff f10;
- arr_typedef___diff f11;
+ struct a_complex_struct___diff f2;
+ union a_union___diff f3;
+ enum an_enum___diff f4;
+ named_struct_typedef___diff f5;
+ anon_struct_typedef___diff f6;
+ struct_ptr_typedef___diff f7;
+ int_typedef___diff f8;
+ enum_typedef___diff f9;
+ void_ptr_typedef___diff f10;
+ func_proto_typedef___diff f11;
+ arr_typedef___diff f12;
};
/* different type sizes, extra modifiers, anon vs named enums, etc */
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c b/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c
index 325ead..d95bc08 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_type_based.c
@@ -19,6 +19,14 @@ struct a_struct {
int x;
};
+struct a_complex_struct {
+ union {
+ struct a_struct *a;
+ void *b;
+ } x;
+ volatile long y;
+};
+
union a_union {
int y;
int z;
@@ -50,6 +58,7 @@ typedef char arr_typedef[20];
struct core_reloc_type_based_output {
bool struct_exists;
+ bool complex_struct_exists;
bool union_exists;
bool enum_exists;
bool typedef_named_struct_exists;
@@ -62,6 +71,7 @@ struct core_reloc_type_based_output {
bool typedef_arr_exists;
bool struct_matches;
+ bool complex_struct_matches;
bool union_matches;
bool enum_matches;
bool typedef_named_struct_matches;
@@ -99,6 +109,7 @@ int test_core_type_based(void *ctx)
struct core_reloc_type_based_output *out = (void *)&data.out;
out->struct_exists = bpf_core_type_exists(struct a_struct);
+ out->complex_struct_exists = bpf_core_type_exists(struct a_complex_struct);
out->union_exists = bpf_core_type_exists(union a_union);
out->enum_exists = bpf_core_type_exists(enum an_enum);
out->typedef_named_struct_exists = bpf_core_type_exists(named_struct_typedef);
@@ -111,6 +122,7 @@ int test_core_type_based(void *ctx)
out->typedef_arr_exists = bpf_core_type_exists(arr_typedef);
out->struct_matches = bpf_core_type_matches(struct a_struct);
+ out->complex_struct_matches = bpf_core_type_matches(struct a_complex_struct);
out->union_matches = bpf_core_type_matches(union a_union);
out->enum_matches = bpf_core_type_matches(enum an_enum);
out->typedef_named_struct_matches = bpf_core_type_matches(named_struct_typedef);
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread