* [PATCH bpf-next 1/9] bpf: support for new btf kind BTF_KIND_TAG
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
@ 2021-09-07 23:00 ` Yonghong Song
2021-09-09 5:09 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 2/9] libbpf: rename btf_{hash,equal}_int to btf_{hash,equal}_int_tag Yonghong Song
` (8 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:00 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
LLVM14 added support for a new C attribute ([1])
__attribute__((btf_tag("arbitrary_str")))
This attribute will be emitted to dwarf ([2]) and pahole
will convert it to BTF. Or for bpf target, this
attribute will be emitted to BTF directly ([3]).
The attribute is intended to provide additional
information for
- struct/union type or struct/union member
- static/global variables
- static/global function or function parameter.
For linux kernel, the btf_tag can be applied
in various places to specify user pointer,
function pre- or post- condition, function
allow/deny in certain context, etc. Such information
will be encoded in vmlinux BTF and can be used
by verifier.
The btf_tag can also be applied to bpf programs
to help global verifiable functions, e.g.,
specifying preconditions, etc.
This patch added basic parsing and checking support
in kernel for new BTF_KIND_TAG kind.
[1] https://reviews.llvm.org/D106614
[2] https://reviews.llvm.org/D106621
[3] https://reviews.llvm.org/D106622
Signed-off-by: Yonghong Song <yhs@fb.com>
---
include/uapi/linux/btf.h | 15 ++++-
kernel/bpf/btf.c | 115 +++++++++++++++++++++++++++++++++
tools/include/uapi/linux/btf.h | 15 ++++-
3 files changed, 139 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
index d27b1708efe9..ca73c4449116 100644
--- a/include/uapi/linux/btf.h
+++ b/include/uapi/linux/btf.h
@@ -36,14 +36,14 @@ struct btf_type {
* bits 24-27: kind (e.g. int, ptr, array...etc)
* bits 28-30: unused
* bit 31: kind_flag, currently used by
- * struct, union and fwd
+ * struct, union, fwd and tag
*/
__u32 info;
/* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC.
* "size" tells the size of the type it is describing.
*
* "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
- * FUNC, FUNC_PROTO and VAR.
+ * FUNC, FUNC_PROTO, VAR and TAG.
* "type" is a type_id referring to another type.
*/
union {
@@ -73,7 +73,8 @@ struct btf_type {
#define BTF_KIND_VAR 14 /* Variable */
#define BTF_KIND_DATASEC 15 /* Section */
#define BTF_KIND_FLOAT 16 /* Floating point */
-#define BTF_KIND_MAX BTF_KIND_FLOAT
+#define BTF_KIND_TAG 17 /* Tag */
+#define BTF_KIND_MAX BTF_KIND_TAG
#define NR_BTF_KINDS (BTF_KIND_MAX + 1)
/* For some specific BTF_KIND, "struct btf_type" is immediately
@@ -170,4 +171,12 @@ struct btf_var_secinfo {
__u32 size;
};
+/* BTF_KIND_TAG is followed by a single "struct btf_tag" to describe
+ * additional information related to the tag such as which field of
+ * a struct or union or which argument of a function.
+ */
+struct btf_tag {
+ __u32 comp_id;
+};
+
#endif /* _UAPI__LINUX_BTF_H__ */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index dfe61df4f974..9545290f804b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -281,6 +281,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
[BTF_KIND_VAR] = "VAR",
[BTF_KIND_DATASEC] = "DATASEC",
[BTF_KIND_FLOAT] = "FLOAT",
+ [BTF_KIND_TAG] = "TAG",
};
const char *btf_type_str(const struct btf_type *t)
@@ -459,6 +460,17 @@ static bool btf_type_is_datasec(const struct btf_type *t)
return BTF_INFO_KIND(t->info) == BTF_KIND_DATASEC;
}
+static bool btf_type_is_tag(const struct btf_type *t)
+{
+ return BTF_INFO_KIND(t->info) == BTF_KIND_TAG;
+}
+
+static bool btf_type_is_tag_target(const struct btf_type *t)
+{
+ return btf_type_is_func(t) || btf_type_is_struct(t) ||
+ btf_type_is_var(t);
+}
+
u32 btf_nr_types(const struct btf *btf)
{
u32 total = 0;
@@ -537,6 +549,7 @@ const struct btf_type *btf_type_resolve_func_ptr(const struct btf *btf,
static bool btf_type_is_resolve_source_only(const struct btf_type *t)
{
return btf_type_is_var(t) ||
+ btf_type_is_tag(t) ||
btf_type_is_datasec(t);
}
@@ -563,6 +576,7 @@ static bool btf_type_needs_resolve(const struct btf_type *t)
btf_type_is_struct(t) ||
btf_type_is_array(t) ||
btf_type_is_var(t) ||
+ btf_type_is_tag(t) ||
btf_type_is_datasec(t);
}
@@ -616,6 +630,11 @@ static const struct btf_var *btf_type_var(const struct btf_type *t)
return (const struct btf_var *)(t + 1);
}
+static const struct btf_tag *btf_type_tag(const struct btf_type *t)
+{
+ return (const struct btf_tag *)(t + 1);
+}
+
static const struct btf_kind_operations *btf_type_ops(const struct btf_type *t)
{
return kind_ops[BTF_INFO_KIND(t->info)];
@@ -3801,6 +3820,97 @@ static const struct btf_kind_operations float_ops = {
.show = btf_df_show,
};
+static s32 btf_tag_check_meta(struct btf_verifier_env *env,
+ const struct btf_type *t,
+ u32 meta_left)
+{
+ const struct btf_tag *tag;
+ u32 meta_needed = sizeof(*tag);
+
+ if (meta_left < meta_needed) {
+ btf_verifier_log_basic(env, t,
+ "meta_left:%u meta_needed:%u",
+ meta_left, meta_needed);
+ return -EINVAL;
+ }
+
+ if (!t->name_off) {
+ btf_verifier_log_type(env, t, "Invalid name");
+ return -EINVAL;
+ }
+
+ if (btf_type_vlen(t)) {
+ btf_verifier_log_type(env, t, "vlen != 0");
+ return -EINVAL;
+ }
+
+ tag = btf_type_tag(t);
+ if (btf_type_kflag(t) && tag->comp_id) {
+ btf_verifier_log_type(env, t, "kflag/comp_id mismatch");
+ return -EINVAL;
+ }
+
+ btf_verifier_log_type(env, t, NULL);
+
+ return meta_needed;
+}
+
+static int btf_tag_resolve(struct btf_verifier_env *env,
+ const struct resolve_vertex *v)
+{
+ const struct btf_type *next_type;
+ const struct btf_type *t = v->t;
+ u32 next_type_id = t->type;
+ struct btf *btf = env->btf;
+ u32 vlen, comp_id;
+
+ next_type = btf_type_by_id(btf, next_type_id);
+ if (!next_type || !btf_type_is_tag_target(next_type)) {
+ btf_verifier_log_type(env, v->t, "Invalid type_id");
+ return -EINVAL;
+ }
+
+ if (!env_type_is_resolve_sink(env, next_type) &&
+ !env_type_is_resolved(env, next_type_id))
+ return env_stack_push(env, next_type, next_type_id);
+
+ if (!btf_type_kflag(t)) {
+ if (btf_type_is_struct(next_type)) {
+ vlen = btf_type_vlen(next_type);
+ } else if (btf_type_is_func(next_type)) {
+ next_type = btf_type_by_id(btf, next_type->type);
+ vlen = btf_type_vlen(next_type);
+ } else {
+ btf_verifier_log_type(env, v->t, "Invalid next_type");
+ return -EINVAL;
+ }
+
+ comp_id = btf_type_tag(t)->comp_id;
+ if (comp_id >= vlen) {
+ btf_verifier_log_type(env, v->t, "Invalid comp_id");
+ return -EINVAL;
+ }
+ }
+
+ env_stack_pop_resolved(env, next_type_id, 0);
+
+ return 0;
+}
+
+static void btf_tag_log(struct btf_verifier_env *env, const struct btf_type *t)
+{
+ btf_verifier_log(env, "type=%u", t->type);
+}
+
+static const struct btf_kind_operations tag_ops = {
+ .check_meta = btf_tag_check_meta,
+ .resolve = btf_tag_resolve,
+ .check_member = btf_df_check_member,
+ .check_kflag_member = btf_df_check_kflag_member,
+ .log_details = btf_tag_log,
+ .show = btf_df_show,
+};
+
static int btf_func_proto_check(struct btf_verifier_env *env,
const struct btf_type *t)
{
@@ -3935,6 +4045,7 @@ static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = {
[BTF_KIND_VAR] = &var_ops,
[BTF_KIND_DATASEC] = &datasec_ops,
[BTF_KIND_FLOAT] = &float_ops,
+ [BTF_KIND_TAG] = &tag_ops,
};
static s32 btf_check_meta(struct btf_verifier_env *env,
@@ -4019,6 +4130,10 @@ static bool btf_resolve_valid(struct btf_verifier_env *env,
return !btf_resolved_type_id(btf, type_id) &&
!btf_resolved_type_size(btf, type_id);
+ if (btf_type_is_tag(t))
+ return btf_resolved_type_id(btf, type_id) &&
+ !btf_resolved_type_size(btf, type_id);
+
if (btf_type_is_modifier(t) || btf_type_is_ptr(t) ||
btf_type_is_var(t)) {
t = btf_type_id_resolve(btf, &type_id);
diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h
index d27b1708efe9..ca73c4449116 100644
--- a/tools/include/uapi/linux/btf.h
+++ b/tools/include/uapi/linux/btf.h
@@ -36,14 +36,14 @@ struct btf_type {
* bits 24-27: kind (e.g. int, ptr, array...etc)
* bits 28-30: unused
* bit 31: kind_flag, currently used by
- * struct, union and fwd
+ * struct, union, fwd and tag
*/
__u32 info;
/* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC.
* "size" tells the size of the type it is describing.
*
* "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
- * FUNC, FUNC_PROTO and VAR.
+ * FUNC, FUNC_PROTO, VAR and TAG.
* "type" is a type_id referring to another type.
*/
union {
@@ -73,7 +73,8 @@ struct btf_type {
#define BTF_KIND_VAR 14 /* Variable */
#define BTF_KIND_DATASEC 15 /* Section */
#define BTF_KIND_FLOAT 16 /* Floating point */
-#define BTF_KIND_MAX BTF_KIND_FLOAT
+#define BTF_KIND_TAG 17 /* Tag */
+#define BTF_KIND_MAX BTF_KIND_TAG
#define NR_BTF_KINDS (BTF_KIND_MAX + 1)
/* For some specific BTF_KIND, "struct btf_type" is immediately
@@ -170,4 +171,12 @@ struct btf_var_secinfo {
__u32 size;
};
+/* BTF_KIND_TAG is followed by a single "struct btf_tag" to describe
+ * additional information related to the tag such as which field of
+ * a struct or union or which argument of a function.
+ */
+struct btf_tag {
+ __u32 comp_id;
+};
+
#endif /* _UAPI__LINUX_BTF_H__ */
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 1/9] bpf: support for new btf kind BTF_KIND_TAG
2021-09-07 23:00 ` [PATCH bpf-next 1/9] bpf: " Yonghong Song
@ 2021-09-09 5:09 ` Andrii Nakryiko
2021-09-10 15:55 ` Yonghong Song
0 siblings, 1 reply; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:09 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>
> LLVM14 added support for a new C attribute ([1])
> __attribute__((btf_tag("arbitrary_str")))
> This attribute will be emitted to dwarf ([2]) and pahole
> will convert it to BTF. Or for bpf target, this
> attribute will be emitted to BTF directly ([3]).
> The attribute is intended to provide additional
> information for
> - struct/union type or struct/union member
> - static/global variables
> - static/global function or function parameter.
>
> For linux kernel, the btf_tag can be applied
> in various places to specify user pointer,
> function pre- or post- condition, function
> allow/deny in certain context, etc. Such information
> will be encoded in vmlinux BTF and can be used
> by verifier.
>
> The btf_tag can also be applied to bpf programs
> to help global verifiable functions, e.g.,
> specifying preconditions, etc.
>
> This patch added basic parsing and checking support
> in kernel for new BTF_KIND_TAG kind.
>
> [1] https://reviews.llvm.org/D106614
> [2] https://reviews.llvm.org/D106621
> [3] https://reviews.llvm.org/D106622
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> include/uapi/linux/btf.h | 15 ++++-
> kernel/bpf/btf.c | 115 +++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/btf.h | 15 ++++-
> 3 files changed, 139 insertions(+), 6 deletions(-)
>
> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> index d27b1708efe9..ca73c4449116 100644
> --- a/include/uapi/linux/btf.h
> +++ b/include/uapi/linux/btf.h
> @@ -36,14 +36,14 @@ struct btf_type {
> * bits 24-27: kind (e.g. int, ptr, array...etc)
> * bits 28-30: unused
> * bit 31: kind_flag, currently used by
> - * struct, union and fwd
> + * struct, union, fwd and tag
> */
> __u32 info;
> /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC.
> * "size" tells the size of the type it is describing.
> *
> * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
> - * FUNC, FUNC_PROTO and VAR.
> + * FUNC, FUNC_PROTO, VAR and TAG.
> * "type" is a type_id referring to another type.
> */
> union {
> @@ -73,7 +73,8 @@ struct btf_type {
> #define BTF_KIND_VAR 14 /* Variable */
> #define BTF_KIND_DATASEC 15 /* Section */
> #define BTF_KIND_FLOAT 16 /* Floating point */
> -#define BTF_KIND_MAX BTF_KIND_FLOAT
> +#define BTF_KIND_TAG 17 /* Tag */
> +#define BTF_KIND_MAX BTF_KIND_TAG
> #define NR_BTF_KINDS (BTF_KIND_MAX + 1)
offtop, but realized reading this: we should probably turn these into
enums and capture them in vmlinux BTF and subsequently in vmlinux.h
>
> /* For some specific BTF_KIND, "struct btf_type" is immediately
> @@ -170,4 +171,12 @@ struct btf_var_secinfo {
> __u32 size;
> };
>
> +/* BTF_KIND_TAG is followed by a single "struct btf_tag" to describe
> + * additional information related to the tag such as which field of
> + * a struct or union or which argument of a function.
> + */
> +struct btf_tag {
> + __u32 comp_id;
what does "comp" stand for, component? If yes, it's quite non-obvious,
I wonder if just as generic "member" would be better (and no
contractions)? Maybe also not id (because I immediately thought about
BTF type IDs), but "index". So "member_idx"? "component_idx" would be
quite obvious as well, just a bit longer.
> +};
> +
> #endif /* _UAPI__LINUX_BTF_H__ */
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index dfe61df4f974..9545290f804b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -281,6 +281,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
> [BTF_KIND_VAR] = "VAR",
> [BTF_KIND_DATASEC] = "DATASEC",
> [BTF_KIND_FLOAT] = "FLOAT",
> + [BTF_KIND_TAG] = "TAG",
> };
>
[...]
> + const struct btf_tag *tag;
> + u32 meta_needed = sizeof(*tag);
> +
> + if (meta_left < meta_needed) {
> + btf_verifier_log_basic(env, t,
> + "meta_left:%u meta_needed:%u",
> + meta_left, meta_needed);
> + return -EINVAL;
> + }
> +
> + if (!t->name_off) {
> + btf_verifier_log_type(env, t, "Invalid name");
> + return -EINVAL;
> + }
> +
> + if (btf_type_vlen(t)) {
> + btf_verifier_log_type(env, t, "vlen != 0");
> + return -EINVAL;
> + }
> +
> + tag = btf_type_tag(t);
> + if (btf_type_kflag(t) && tag->comp_id) {
just realized that we could have reserved comp_id == (u32)-1 as the
meaning "applies to entire struct/func/etc"? This might be a bit
cleaner, because if you forget about kflag() semantics, you can treat
comp_id == 0 as if it applied to first member, but if we put
0xffffffff, you'll get SIGSEGV with high probability (making the
problem more obvious)?
> + btf_verifier_log_type(env, t, "kflag/comp_id mismatch");
> + return -EINVAL;
> + }
> +
> + btf_verifier_log_type(env, t, NULL);
> +
> + return meta_needed;
> +}
> +
> +static int btf_tag_resolve(struct btf_verifier_env *env,
> + const struct resolve_vertex *v)
> +{
> + const struct btf_type *next_type;
> + const struct btf_type *t = v->t;
> + u32 next_type_id = t->type;
> + struct btf *btf = env->btf;
> + u32 vlen, comp_id;
> +
> + next_type = btf_type_by_id(btf, next_type_id);
> + if (!next_type || !btf_type_is_tag_target(next_type)) {
> + btf_verifier_log_type(env, v->t, "Invalid type_id");
> + return -EINVAL;
> + }
> +
> + if (!env_type_is_resolve_sink(env, next_type) &&
> + !env_type_is_resolved(env, next_type_id))
> + return env_stack_push(env, next_type, next_type_id);
> +
> + if (!btf_type_kflag(t)) {
> + if (btf_type_is_struct(next_type)) {
> + vlen = btf_type_vlen(next_type);
> + } else if (btf_type_is_func(next_type)) {
> + next_type = btf_type_by_id(btf, next_type->type);
> + vlen = btf_type_vlen(next_type);
> + } else {
> + btf_verifier_log_type(env, v->t, "Invalid next_type");
> + return -EINVAL;
> + }
> +
> + comp_id = btf_type_tag(t)->comp_id;
> + if (comp_id >= vlen) {
> + btf_verifier_log_type(env, v->t, "Invalid comp_id");
> + return -EINVAL;
> + }
> + }
> +
> + env_stack_pop_resolved(env, next_type_id, 0);
> +
> + return 0;
> +}
> +
> +static void btf_tag_log(struct btf_verifier_env *env, const struct btf_type *t)
> +{
> + btf_verifier_log(env, "type=%u", t->type);
comp_id and kflag should be logged as well, they are important part
> +}
> +
> +static const struct btf_kind_operations tag_ops = {
> + .check_meta = btf_tag_check_meta,
> + .resolve = btf_tag_resolve,
> + .check_member = btf_df_check_member,
> + .check_kflag_member = btf_df_check_kflag_member,
> + .log_details = btf_tag_log,
> + .show = btf_df_show,
> +};
> +
[...]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 1/9] bpf: support for new btf kind BTF_KIND_TAG
2021-09-09 5:09 ` Andrii Nakryiko
@ 2021-09-10 15:55 ` Yonghong Song
0 siblings, 0 replies; 31+ messages in thread
From: Yonghong Song @ 2021-09-10 15:55 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On 9/8/21 10:09 PM, Andrii Nakryiko wrote:
> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> LLVM14 added support for a new C attribute ([1])
>> __attribute__((btf_tag("arbitrary_str")))
>> This attribute will be emitted to dwarf ([2]) and pahole
>> will convert it to BTF. Or for bpf target, this
>> attribute will be emitted to BTF directly ([3]).
>> The attribute is intended to provide additional
>> information for
>> - struct/union type or struct/union member
>> - static/global variables
>> - static/global function or function parameter.
>>
>> For linux kernel, the btf_tag can be applied
>> in various places to specify user pointer,
>> function pre- or post- condition, function
>> allow/deny in certain context, etc. Such information
>> will be encoded in vmlinux BTF and can be used
>> by verifier.
>>
>> The btf_tag can also be applied to bpf programs
>> to help global verifiable functions, e.g.,
>> specifying preconditions, etc.
>>
>> This patch added basic parsing and checking support
>> in kernel for new BTF_KIND_TAG kind.
>>
>> [1] https://reviews.llvm.org/D106614
>> [2] https://reviews.llvm.org/D106621
>> [3] https://reviews.llvm.org/D106622
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> include/uapi/linux/btf.h | 15 ++++-
>> kernel/bpf/btf.c | 115 +++++++++++++++++++++++++++++++++
>> tools/include/uapi/linux/btf.h | 15 ++++-
>> 3 files changed, 139 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
>> index d27b1708efe9..ca73c4449116 100644
>> --- a/include/uapi/linux/btf.h
>> +++ b/include/uapi/linux/btf.h
>> @@ -36,14 +36,14 @@ struct btf_type {
>> * bits 24-27: kind (e.g. int, ptr, array...etc)
>> * bits 28-30: unused
>> * bit 31: kind_flag, currently used by
>> - * struct, union and fwd
>> + * struct, union, fwd and tag
>> */
>> __u32 info;
>> /* "size" is used by INT, ENUM, STRUCT, UNION and DATASEC.
>> * "size" tells the size of the type it is describing.
>> *
>> * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
>> - * FUNC, FUNC_PROTO and VAR.
>> + * FUNC, FUNC_PROTO, VAR and TAG.
>> * "type" is a type_id referring to another type.
>> */
>> union {
>> @@ -73,7 +73,8 @@ struct btf_type {
>> #define BTF_KIND_VAR 14 /* Variable */
>> #define BTF_KIND_DATASEC 15 /* Section */
>> #define BTF_KIND_FLOAT 16 /* Floating point */
>> -#define BTF_KIND_MAX BTF_KIND_FLOAT
>> +#define BTF_KIND_TAG 17 /* Tag */
>> +#define BTF_KIND_MAX BTF_KIND_TAG
>> #define NR_BTF_KINDS (BTF_KIND_MAX + 1)
>
> offtop, but realized reading this: we should probably turn these into
> enums and capture them in vmlinux BTF and subsequently in vmlinux.h
Sure. Will look into this.
>
>>
>> /* For some specific BTF_KIND, "struct btf_type" is immediately
>> @@ -170,4 +171,12 @@ struct btf_var_secinfo {
>> __u32 size;
>> };
>>
>> +/* BTF_KIND_TAG is followed by a single "struct btf_tag" to describe
>> + * additional information related to the tag such as which field of
>> + * a struct or union or which argument of a function.
>> + */
>> +struct btf_tag {
>> + __u32 comp_id;
>
> what does "comp" stand for, component? If yes, it's quite non-obvious,
> I wonder if just as generic "member" would be better (and no
> contractions)? Maybe also not id (because I immediately thought about
> BTF type IDs), but "index". So "member_idx"? "component_idx" would be
> quite obvious as well, just a bit longer.
I will use component_idx as member_idx doesn't align well with function
parameters.
>
>> +};
>> +
>> #endif /* _UAPI__LINUX_BTF_H__ */
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index dfe61df4f974..9545290f804b 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -281,6 +281,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
>> [BTF_KIND_VAR] = "VAR",
>> [BTF_KIND_DATASEC] = "DATASEC",
>> [BTF_KIND_FLOAT] = "FLOAT",
>> + [BTF_KIND_TAG] = "TAG",
>> };
>>
>
> [...]
>
>> + const struct btf_tag *tag;
>> + u32 meta_needed = sizeof(*tag);
>> +
>> + if (meta_left < meta_needed) {
>> + btf_verifier_log_basic(env, t,
>> + "meta_left:%u meta_needed:%u",
>> + meta_left, meta_needed);
>> + return -EINVAL;
>> + }
>> +
>> + if (!t->name_off) {
>> + btf_verifier_log_type(env, t, "Invalid name");
>> + return -EINVAL;
>> + }
>> +
>> + if (btf_type_vlen(t)) {
>> + btf_verifier_log_type(env, t, "vlen != 0");
>> + return -EINVAL;
>> + }
>> +
>> + tag = btf_type_tag(t);
>> + if (btf_type_kflag(t) && tag->comp_id) {
>
> just realized that we could have reserved comp_id == (u32)-1 as the
> meaning "applies to entire struct/func/etc"? This might be a bit
> cleaner, because if you forget about kflag() semantics, you can treat
> comp_id == 0 as if it applied to first member, but if we put
> 0xffffffff, you'll get SIGSEGV with high probability (making the
> problem more obvious)?
Good idea. I will get rid of kflag requirement and only use
component_idx to indicate where the attribute is attached with
-1 indicate it is attached to the type itself. The llvm has
been changed with the new ELF format: https://reviews.llvm.org/D109560
>
>
>> + btf_verifier_log_type(env, t, "kflag/comp_id mismatch");
>> + return -EINVAL;
>> + }
>> +
>> + btf_verifier_log_type(env, t, NULL);
>> +
>> + return meta_needed;
>> +}
>> +
>> +static int btf_tag_resolve(struct btf_verifier_env *env,
>> + const struct resolve_vertex *v)
>> +{
>> + const struct btf_type *next_type;
>> + const struct btf_type *t = v->t;
>> + u32 next_type_id = t->type;
>> + struct btf *btf = env->btf;
>> + u32 vlen, comp_id;
>> +
>> + next_type = btf_type_by_id(btf, next_type_id);
>> + if (!next_type || !btf_type_is_tag_target(next_type)) {
>> + btf_verifier_log_type(env, v->t, "Invalid type_id");
>> + return -EINVAL;
>> + }
>> +
>> + if (!env_type_is_resolve_sink(env, next_type) &&
>> + !env_type_is_resolved(env, next_type_id))
>> + return env_stack_push(env, next_type, next_type_id);
>> +
>> + if (!btf_type_kflag(t)) {
>> + if (btf_type_is_struct(next_type)) {
>> + vlen = btf_type_vlen(next_type);
>> + } else if (btf_type_is_func(next_type)) {
>> + next_type = btf_type_by_id(btf, next_type->type);
>> + vlen = btf_type_vlen(next_type);
>> + } else {
>> + btf_verifier_log_type(env, v->t, "Invalid next_type");
>> + return -EINVAL;
>> + }
>> +
>> + comp_id = btf_type_tag(t)->comp_id;
>> + if (comp_id >= vlen) {
>> + btf_verifier_log_type(env, v->t, "Invalid comp_id");
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + env_stack_pop_resolved(env, next_type_id, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static void btf_tag_log(struct btf_verifier_env *env, const struct btf_type *t)
>> +{
>> + btf_verifier_log(env, "type=%u", t->type);
>
> comp_id and kflag should be logged as well, they are important part
Right, will log component_idx. kflag is not needed per above discussion.
>
>> +}
>> +
>> +static const struct btf_kind_operations tag_ops = {
>> + .check_meta = btf_tag_check_meta,
>> + .resolve = btf_tag_resolve,
>> + .check_member = btf_df_check_member,
>> + .check_kflag_member = btf_df_check_kflag_member,
>> + .log_details = btf_tag_log,
>> + .show = btf_df_show,
>> +};
>> +
>
> [...]
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH bpf-next 2/9] libbpf: rename btf_{hash,equal}_int to btf_{hash,equal}_int_tag
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
2021-09-07 23:00 ` [PATCH bpf-next 1/9] bpf: " Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-09 5:10 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG Yonghong Song
` (7 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
This patch renames functions btf_{hash,equal}_int() to
btf_{hash,equal}_int_tag() so they can be reused for
BTF_KIND_TAG support. There is no functionality change for
this patch.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/lib/bpf/btf.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 77dc24d58302..7cb6ebf1be37 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -3256,8 +3256,8 @@ static bool btf_equal_common(struct btf_type *t1, struct btf_type *t2)
t1->size == t2->size;
}
-/* Calculate type signature hash of INT. */
-static long btf_hash_int(struct btf_type *t)
+/* Calculate type signature hash of INT or TAG. */
+static long btf_hash_int_tag(struct btf_type *t)
{
__u32 info = *(__u32 *)(t + 1);
long h;
@@ -3267,8 +3267,8 @@ static long btf_hash_int(struct btf_type *t)
return h;
}
-/* Check structural equality of two INTs. */
-static bool btf_equal_int(struct btf_type *t1, struct btf_type *t2)
+/* Check structural equality of two INTs or TAGs. */
+static bool btf_equal_int_tag(struct btf_type *t1, struct btf_type *t2)
{
__u32 info1, info2;
@@ -3535,7 +3535,7 @@ static int btf_dedup_prep(struct btf_dedup *d)
h = btf_hash_common(t);
break;
case BTF_KIND_INT:
- h = btf_hash_int(t);
+ h = btf_hash_int_tag(t);
break;
case BTF_KIND_ENUM:
h = btf_hash_enum(t);
@@ -3593,11 +3593,11 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
return 0;
case BTF_KIND_INT:
- h = btf_hash_int(t);
+ h = btf_hash_int_tag(t);
for_each_dedup_cand(d, hash_entry, h) {
cand_id = (__u32)(long)hash_entry->value;
cand = btf_type_by_id(d->btf, cand_id);
- if (btf_equal_int(t, cand)) {
+ if (btf_equal_int_tag(t, cand)) {
new_id = cand_id;
break;
}
@@ -3881,7 +3881,7 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id,
switch (cand_kind) {
case BTF_KIND_INT:
- return btf_equal_int(cand_type, canon_type);
+ return btf_equal_int_tag(cand_type, canon_type);
case BTF_KIND_ENUM:
if (d->opts.dont_resolve_fwds)
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 2/9] libbpf: rename btf_{hash,equal}_int to btf_{hash,equal}_int_tag
2021-09-07 23:01 ` [PATCH bpf-next 2/9] libbpf: rename btf_{hash,equal}_int to btf_{hash,equal}_int_tag Yonghong Song
@ 2021-09-09 5:10 ` Andrii Nakryiko
0 siblings, 0 replies; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:10 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>
> This patch renames functions btf_{hash,equal}_int() to
> btf_{hash,equal}_int_tag() so they can be reused for
> BTF_KIND_TAG support. There is no functionality change for
> this patch.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> tools/lib/bpf/btf.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
2021-09-07 23:00 ` [PATCH bpf-next 1/9] bpf: " Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 2/9] libbpf: rename btf_{hash,equal}_int to btf_{hash,equal}_int_tag Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-09 5:26 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 4/9] bpftool: " Yonghong Song
` (6 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
Add BTF_KIND_TAG support for parsing and dedup.
Also added sanitization for BTF_KIND_TAG. If BTF_KIND_TAG is not
supported in the kernel, sanitize it to INTs.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/lib/bpf/btf.c | 61 +++++++++++++++++++++++++++++++++
tools/lib/bpf/btf.h | 13 +++++++
tools/lib/bpf/btf_dump.c | 3 ++
tools/lib/bpf/libbpf.c | 31 +++++++++++++++--
tools/lib/bpf/libbpf.map | 1 +
tools/lib/bpf/libbpf_internal.h | 2 ++
6 files changed, 108 insertions(+), 3 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 7cb6ebf1be37..ed02b17aad17 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -304,6 +304,8 @@ static int btf_type_size(const struct btf_type *t)
return base_size + sizeof(struct btf_var);
case BTF_KIND_DATASEC:
return base_size + vlen * sizeof(struct btf_var_secinfo);
+ case BTF_KIND_TAG:
+ return base_size + sizeof(struct btf_tag);
default:
pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
return -EINVAL;
@@ -376,6 +378,9 @@ static int btf_bswap_type_rest(struct btf_type *t)
v->size = bswap_32(v->size);
}
return 0;
+ case BTF_KIND_TAG:
+ btf_tag(t)->comp_id = bswap_32(btf_tag(t)->comp_id);
+ return 0;
default:
pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
return -EINVAL;
@@ -586,6 +591,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
case BTF_KIND_CONST:
case BTF_KIND_RESTRICT:
case BTF_KIND_VAR:
+ case BTF_KIND_TAG:
type_id = t->type;
break;
case BTF_KIND_ARRAY:
@@ -2440,6 +2446,41 @@ int btf__add_datasec_var_info(struct btf *btf, int var_type_id, __u32 offset, __
return 0;
}
+int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id)
+{
+ bool for_ref_type = false;
+ struct btf_type *t;
+ int sz, name_off;
+
+ if (!name || !name[0] || comp_id < -1)
+ return libbpf_err(-EINVAL);
+
+ if (validate_type_id(ref_type_id))
+ return libbpf_err(-EINVAL);
+
+ if (btf_ensure_modifiable(btf))
+ return libbpf_err(-ENOMEM);
+
+ sz = sizeof(struct btf_type) + sizeof(struct btf_tag);
+ t = btf_add_type_mem(btf, sz);
+ if (!t)
+ return libbpf_err(-ENOMEM);
+
+ name_off = btf__add_str(btf, name);
+ if (name_off < 0)
+ return name_off;
+
+ t->name_off = name_off;
+ t->type = ref_type_id;
+
+ if (comp_id == -1)
+ for_ref_type = true;
+ t->info = btf_type_info(BTF_KIND_TAG, 0, for_ref_type);
+ ((struct btf_tag *)(t + 1))->comp_id = for_ref_type ? 0 : comp_id;
+
+ return btf_commit_type(btf, sz);
+}
+
struct btf_ext_sec_setup_param {
__u32 off;
__u32 len;
@@ -3535,6 +3576,7 @@ static int btf_dedup_prep(struct btf_dedup *d)
h = btf_hash_common(t);
break;
case BTF_KIND_INT:
+ case BTF_KIND_TAG:
h = btf_hash_int_tag(t);
break;
case BTF_KIND_ENUM:
@@ -3590,6 +3632,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
case BTF_KIND_FUNC_PROTO:
case BTF_KIND_VAR:
case BTF_KIND_DATASEC:
+ case BTF_KIND_TAG:
return 0;
case BTF_KIND_INT:
@@ -4210,6 +4253,23 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
}
break;
+ case BTF_KIND_TAG:
+ ref_type_id = btf_dedup_ref_type(d, t->type);
+ if (ref_type_id < 0)
+ return ref_type_id;
+ t->type = ref_type_id;
+
+ h = btf_hash_int_tag(t);
+ for_each_dedup_cand(d, hash_entry, h) {
+ cand_id = (__u32)(long)hash_entry->value;
+ cand = btf_type_by_id(d->btf, cand_id);
+ if (btf_equal_int_tag(t, cand)) {
+ new_id = cand_id;
+ break;
+ }
+ }
+ break;
+
case BTF_KIND_ARRAY: {
struct btf_array *info = btf_array(t);
@@ -4482,6 +4542,7 @@ int btf_type_visit_type_ids(struct btf_type *t, type_id_visit_fn visit, void *ct
case BTF_KIND_TYPEDEF:
case BTF_KIND_FUNC:
case BTF_KIND_VAR:
+ case BTF_KIND_TAG:
return visit(&t->type, ctx);
case BTF_KIND_ARRAY: {
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 4a711f990904..a78cf8331d49 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -141,6 +141,9 @@ LIBBPF_API int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz
LIBBPF_API int btf__add_datasec_var_info(struct btf *btf, int var_type_id,
__u32 offset, __u32 byte_sz);
+/* tag contruction API */
+LIBBPF_API int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id);
+
struct btf_dedup_opts {
unsigned int dedup_table_size;
bool dont_resolve_fwds;
@@ -328,6 +331,11 @@ static inline bool btf_is_float(const struct btf_type *t)
return btf_kind(t) == BTF_KIND_FLOAT;
}
+static inline bool btf_is_tag(const struct btf_type *t)
+{
+ return btf_kind(t) == BTF_KIND_TAG;
+}
+
static inline __u8 btf_int_encoding(const struct btf_type *t)
{
return BTF_INT_ENCODING(*(__u32 *)(t + 1));
@@ -396,6 +404,11 @@ btf_var_secinfos(const struct btf_type *t)
return (struct btf_var_secinfo *)(t + 1);
}
+static inline struct btf_tag *btf_tag(const struct btf_type *t)
+{
+ return (struct btf_tag *)(t + 1);
+}
+
#ifdef __cplusplus
} /* extern "C" */
#endif
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index e4b483f15fb9..ad6df97295ae 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -316,6 +316,7 @@ static int btf_dump_mark_referenced(struct btf_dump *d)
case BTF_KIND_TYPEDEF:
case BTF_KIND_FUNC:
case BTF_KIND_VAR:
+ case BTF_KIND_TAG:
d->type_states[t->type].referenced = 1;
break;
@@ -583,6 +584,7 @@ static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr)
case BTF_KIND_FUNC:
case BTF_KIND_VAR:
case BTF_KIND_DATASEC:
+ case BTF_KIND_TAG:
d->type_states[id].order_state = ORDERED;
return 0;
@@ -2215,6 +2217,7 @@ static int btf_dump_dump_type_data(struct btf_dump *d,
case BTF_KIND_FWD:
case BTF_KIND_FUNC:
case BTF_KIND_FUNC_PROTO:
+ case BTF_KIND_TAG:
err = btf_dump_unsupported_data(d, t, id);
break;
case BTF_KIND_INT:
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 88d8825fc6f6..4bee9eb64aa0 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -195,6 +195,8 @@ enum kern_feature_id {
FEAT_BTF_FLOAT,
/* BPF perf link support */
FEAT_PERF_LINK,
+ /* BTF_KIND_ATTR support */
+ FEAT_BTF_TAG,
__FEAT_CNT,
};
@@ -1987,6 +1989,7 @@ static const char *__btf_kind_str(__u16 kind)
case BTF_KIND_VAR: return "var";
case BTF_KIND_DATASEC: return "datasec";
case BTF_KIND_FLOAT: return "float";
+ case BTF_KIND_TAG: return "tag";
default: return "unknown";
}
}
@@ -2486,8 +2489,9 @@ static bool btf_needs_sanitization(struct bpf_object *obj)
bool has_datasec = kernel_supports(obj, FEAT_BTF_DATASEC);
bool has_float = kernel_supports(obj, FEAT_BTF_FLOAT);
bool has_func = kernel_supports(obj, FEAT_BTF_FUNC);
+ bool has_tag = kernel_supports(obj, FEAT_BTF_TAG);
- return !has_func || !has_datasec || !has_func_global || !has_float;
+ return !has_func || !has_datasec || !has_func_global || !has_float || !has_tag;
}
static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
@@ -2496,14 +2500,15 @@ static void bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
bool has_datasec = kernel_supports(obj, FEAT_BTF_DATASEC);
bool has_float = kernel_supports(obj, FEAT_BTF_FLOAT);
bool has_func = kernel_supports(obj, FEAT_BTF_FUNC);
+ bool has_tag = kernel_supports(obj, FEAT_BTF_TAG);
struct btf_type *t;
int i, j, vlen;
for (i = 1; i <= btf__get_nr_types(btf); i++) {
t = (struct btf_type *)btf__type_by_id(btf, i);
- if (!has_datasec && btf_is_var(t)) {
- /* replace VAR with INT */
+ if ((!has_datasec && btf_is_var(t)) || (!has_tag && btf_is_tag(t))) {
+ /* replace VAR/TAG with INT */
t->info = BTF_INFO_ENC(BTF_KIND_INT, 0, 0);
/*
* using size = 1 is the safest choice, 4 will be too
@@ -4207,6 +4212,23 @@ static int probe_kern_btf_float(void)
strs, sizeof(strs)));
}
+static int probe_kern_btf_tag(void)
+{
+ static const char strs[] = "\0tag";
+ __u32 types[] = {
+ /* int */
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ /* VAR x */ /* [2] */
+ BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
+ BTF_VAR_STATIC,
+ /* attr */
+ BTF_TYPE_TAG_KIND_ENC(1, 2),
+ };
+
+ return probe_fd(libbpf__load_raw_btf((char *)types, sizeof(types),
+ strs, sizeof(strs)));
+}
+
static int probe_kern_array_mmap(void)
{
struct bpf_create_map_attr attr = {
@@ -4423,6 +4445,9 @@ static struct kern_feature_desc {
[FEAT_PERF_LINK] = {
"BPF perf link support", probe_perf_link,
},
+ [FEAT_BTF_TAG] = {
+ "BTF_KIND_TAG support", probe_kern_btf_tag,
+ },
};
static bool kernel_supports(const struct bpf_object *obj, enum kern_feature_id feat_id)
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index bbc53bb25f68..62c4c932ef6a 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -370,6 +370,7 @@ LIBBPF_0.4.0 {
LIBBPF_0.5.0 {
global:
+ btf__add_tag;
bpf_map__initial_value;
bpf_map__pin_path;
bpf_map_lookup_and_delete_elem_flags;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 533b0211f40a..7deb86d9af51 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -69,6 +69,8 @@
#define BTF_VAR_SECINFO_ENC(type, offset, size) (type), (offset), (size)
#define BTF_TYPE_FLOAT_ENC(name, sz) \
BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0), sz)
+#define BTF_TYPE_TAG_KIND_ENC(name, type) \
+ BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_TAG, 1, 0), type), (0)
#ifndef likely
#define likely(x) __builtin_expect(!!(x), 1)
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG
2021-09-07 23:01 ` [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG Yonghong Song
@ 2021-09-09 5:26 ` Andrii Nakryiko
2021-09-10 16:04 ` Yonghong Song
0 siblings, 1 reply; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:26 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>
> Add BTF_KIND_TAG support for parsing and dedup.
> Also added sanitization for BTF_KIND_TAG. If BTF_KIND_TAG is not
> supported in the kernel, sanitize it to INTs.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> tools/lib/bpf/btf.c | 61 +++++++++++++++++++++++++++++++++
> tools/lib/bpf/btf.h | 13 +++++++
> tools/lib/bpf/btf_dump.c | 3 ++
> tools/lib/bpf/libbpf.c | 31 +++++++++++++++--
> tools/lib/bpf/libbpf.map | 1 +
> tools/lib/bpf/libbpf_internal.h | 2 ++
> 6 files changed, 108 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 7cb6ebf1be37..ed02b17aad17 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -304,6 +304,8 @@ static int btf_type_size(const struct btf_type *t)
> return base_size + sizeof(struct btf_var);
> case BTF_KIND_DATASEC:
> return base_size + vlen * sizeof(struct btf_var_secinfo);
> + case BTF_KIND_TAG:
> + return base_size + sizeof(struct btf_tag);
> default:
> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
> return -EINVAL;
> @@ -376,6 +378,9 @@ static int btf_bswap_type_rest(struct btf_type *t)
> v->size = bswap_32(v->size);
> }
> return 0;
> + case BTF_KIND_TAG:
> + btf_tag(t)->comp_id = bswap_32(btf_tag(t)->comp_id);
> + return 0;
> default:
> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
> return -EINVAL;
> @@ -586,6 +591,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
> case BTF_KIND_CONST:
> case BTF_KIND_RESTRICT:
> case BTF_KIND_VAR:
> + case BTF_KIND_TAG:
> type_id = t->type;
> break;
> case BTF_KIND_ARRAY:
> @@ -2440,6 +2446,41 @@ int btf__add_datasec_var_info(struct btf *btf, int var_type_id, __u32 offset, __
> return 0;
> }
>
> +int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id)
Curious about the terminology here. The string recorded in bpf_tag, is
that a "name" of the tag, or rather a "value" of the tag? We should
reflect that in argument names for btf__add_tag.
I'll also nitpick on order of arguments. ref_type_id is always
specified, and it points to the entire type (struct/union/func), while
comp_id might, optionally, point inside that type. So I think the
order should be ref_type_id followed by comp_id.
Please also add a comment describing inputs (especially the -1 comp_id
case) and outputs, like all that other btf__add_xxx() APIs.
> +{
> + bool for_ref_type = false;
> + struct btf_type *t;
> + int sz, name_off;
> +
> + if (!name || !name[0] || comp_id < -1)
> + return libbpf_err(-EINVAL);
> +
> + if (validate_type_id(ref_type_id))
> + return libbpf_err(-EINVAL);
> +
> + if (btf_ensure_modifiable(btf))
> + return libbpf_err(-ENOMEM);
> +
> + sz = sizeof(struct btf_type) + sizeof(struct btf_tag);
> + t = btf_add_type_mem(btf, sz);
> + if (!t)
> + return libbpf_err(-ENOMEM);
> +
> + name_off = btf__add_str(btf, name);
> + if (name_off < 0)
> + return name_off;
> +
> + t->name_off = name_off;
> + t->type = ref_type_id;
> +
> + if (comp_id == -1)
> + for_ref_type = true;
> + t->info = btf_type_info(BTF_KIND_TAG, 0, for_ref_type);
> + ((struct btf_tag *)(t + 1))->comp_id = for_ref_type ? 0 : comp_id;
As I mentioned in the previous patch, it feels cleaner to just have
this -1 special value and not utilize kflag at all. It will match
libbpf API as you defined it naturally.
> +
> + return btf_commit_type(btf, sz);
> +}
> +
[...]
> case BTF_KIND_ARRAY: {
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 4a711f990904..a78cf8331d49 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -141,6 +141,9 @@ LIBBPF_API int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz
> LIBBPF_API int btf__add_datasec_var_info(struct btf *btf, int var_type_id,
> __u32 offset, __u32 byte_sz);
>
> +/* tag contruction API */
typo: construction, but I'd put it after btf__add_restrict with no comment
> +LIBBPF_API int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id);
> +
> struct btf_dedup_opts {
> unsigned int dedup_table_size;
> bool dont_resolve_fwds;
> @@ -328,6 +331,11 @@ static inline bool btf_is_float(const struct btf_type *t)
> return btf_kind(t) == BTF_KIND_FLOAT;
> }
>
> +static inline bool btf_is_tag(const struct btf_type *t)
> +{
> + return btf_kind(t) == BTF_KIND_TAG;
> +}
> +
> static inline __u8 btf_int_encoding(const struct btf_type *t)
> {
> return BTF_INT_ENCODING(*(__u32 *)(t + 1));
> @@ -396,6 +404,11 @@ btf_var_secinfos(const struct btf_type *t)
> return (struct btf_var_secinfo *)(t + 1);
> }
>
please add `struct btf_tag;` forward reference for those users who are
compiling with old UAPI headers.
> +static inline struct btf_tag *btf_tag(const struct btf_type *t)
> +{
> + return (struct btf_tag *)(t + 1);
> +}
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
[...]
> LIBBPF_0.5.0 {
> global:
> + btf__add_tag;
> bpf_map__initial_value;
> bpf_map__pin_path;
> bpf_map_lookup_and_delete_elem_flags;
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 533b0211f40a..7deb86d9af51 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -69,6 +69,8 @@
> #define BTF_VAR_SECINFO_ENC(type, offset, size) (type), (offset), (size)
> #define BTF_TYPE_FLOAT_ENC(name, sz) \
> BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0), sz)
> +#define BTF_TYPE_TAG_KIND_ENC(name, type) \
following other macro names, it should be BTF_TYPE_TAG_ENC, no?
> + BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_TAG, 1, 0), type), (0)
>
> #ifndef likely
> #define likely(x) __builtin_expect(!!(x), 1)
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG
2021-09-09 5:26 ` Andrii Nakryiko
@ 2021-09-10 16:04 ` Yonghong Song
0 siblings, 0 replies; 31+ messages in thread
From: Yonghong Song @ 2021-09-10 16:04 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On 9/8/21 10:26 PM, Andrii Nakryiko wrote:
> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add BTF_KIND_TAG support for parsing and dedup.
>> Also added sanitization for BTF_KIND_TAG. If BTF_KIND_TAG is not
>> supported in the kernel, sanitize it to INTs.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> tools/lib/bpf/btf.c | 61 +++++++++++++++++++++++++++++++++
>> tools/lib/bpf/btf.h | 13 +++++++
>> tools/lib/bpf/btf_dump.c | 3 ++
>> tools/lib/bpf/libbpf.c | 31 +++++++++++++++--
>> tools/lib/bpf/libbpf.map | 1 +
>> tools/lib/bpf/libbpf_internal.h | 2 ++
>> 6 files changed, 108 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>> index 7cb6ebf1be37..ed02b17aad17 100644
>> --- a/tools/lib/bpf/btf.c
>> +++ b/tools/lib/bpf/btf.c
>> @@ -304,6 +304,8 @@ static int btf_type_size(const struct btf_type *t)
>> return base_size + sizeof(struct btf_var);
>> case BTF_KIND_DATASEC:
>> return base_size + vlen * sizeof(struct btf_var_secinfo);
>> + case BTF_KIND_TAG:
>> + return base_size + sizeof(struct btf_tag);
>> default:
>> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
>> return -EINVAL;
>> @@ -376,6 +378,9 @@ static int btf_bswap_type_rest(struct btf_type *t)
>> v->size = bswap_32(v->size);
>> }
>> return 0;
>> + case BTF_KIND_TAG:
>> + btf_tag(t)->comp_id = bswap_32(btf_tag(t)->comp_id);
>> + return 0;
>> default:
>> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
>> return -EINVAL;
>> @@ -586,6 +591,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
>> case BTF_KIND_CONST:
>> case BTF_KIND_RESTRICT:
>> case BTF_KIND_VAR:
>> + case BTF_KIND_TAG:
>> type_id = t->type;
>> break;
>> case BTF_KIND_ARRAY:
>> @@ -2440,6 +2446,41 @@ int btf__add_datasec_var_info(struct btf *btf, int var_type_id, __u32 offset, __
>> return 0;
>> }
>>
>> +int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id)
>
> Curious about the terminology here. The string recorded in bpf_tag, is
> that a "name" of the tag, or rather a "value" of the tag? We should
> reflect that in argument names for btf__add_tag.
We can use "value" as the argument.
>
> I'll also nitpick on order of arguments. ref_type_id is always
> specified, and it points to the entire type (struct/union/func), while
> comp_id might, optionally, point inside that type. So I think the
> order should be ref_type_id followed by comp_id.
Will switch and then the argument order follows ELF file format order.
>
> Please also add a comment describing inputs (especially the -1 comp_id
> case) and outputs, like all that other btf__add_xxx() APIs.
Will do.
>
>> +{
>> + bool for_ref_type = false;
>> + struct btf_type *t;
>> + int sz, name_off;
>> +
>> + if (!name || !name[0] || comp_id < -1)
>> + return libbpf_err(-EINVAL);
>> +
>> + if (validate_type_id(ref_type_id))
>> + return libbpf_err(-EINVAL);
>> +
>> + if (btf_ensure_modifiable(btf))
>> + return libbpf_err(-ENOMEM);
>> +
>> + sz = sizeof(struct btf_type) + sizeof(struct btf_tag);
>> + t = btf_add_type_mem(btf, sz);
>> + if (!t)
>> + return libbpf_err(-ENOMEM);
>> +
>> + name_off = btf__add_str(btf, name);
>> + if (name_off < 0)
>> + return name_off;
>> +
>> + t->name_off = name_off;
>> + t->type = ref_type_id;
>> +
>> + if (comp_id == -1)
>> + for_ref_type = true;
>> + t->info = btf_type_info(BTF_KIND_TAG, 0, for_ref_type);
>> + ((struct btf_tag *)(t + 1))->comp_id = for_ref_type ? 0 : comp_id;
>
> As I mentioned in the previous patch, it feels cleaner to just have
> this -1 special value and not utilize kflag at all. It will match
> libbpf API as you defined it naturally.
will do.
>
>> +
>> + return btf_commit_type(btf, sz);
>> +}
>> +
>
> [...]
>
>> case BTF_KIND_ARRAY: {
>> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
>> index 4a711f990904..a78cf8331d49 100644
>> --- a/tools/lib/bpf/btf.h
>> +++ b/tools/lib/bpf/btf.h
>> @@ -141,6 +141,9 @@ LIBBPF_API int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz
>> LIBBPF_API int btf__add_datasec_var_info(struct btf *btf, int var_type_id,
>> __u32 offset, __u32 byte_sz);
>>
>> +/* tag contruction API */
>
> typo: construction, but I'd put it after btf__add_restrict with no comment
ack.
>
>> +LIBBPF_API int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id);
>> +
>> struct btf_dedup_opts {
>> unsigned int dedup_table_size;
>> bool dont_resolve_fwds;
>> @@ -328,6 +331,11 @@ static inline bool btf_is_float(const struct btf_type *t)
>> return btf_kind(t) == BTF_KIND_FLOAT;
>> }
>>
>> +static inline bool btf_is_tag(const struct btf_type *t)
>> +{
>> + return btf_kind(t) == BTF_KIND_TAG;
>> +}
>> +
>> static inline __u8 btf_int_encoding(const struct btf_type *t)
>> {
>> return BTF_INT_ENCODING(*(__u32 *)(t + 1));
>> @@ -396,6 +404,11 @@ btf_var_secinfos(const struct btf_type *t)
>> return (struct btf_var_secinfo *)(t + 1);
>> }
>>
>
> please add `struct btf_tag;` forward reference for those users who are
> compiling with old UAPI headers.
okay.
>
>> +static inline struct btf_tag *btf_tag(const struct btf_type *t)
>> +{
>> + return (struct btf_tag *)(t + 1);
>> +}
>> +
>> #ifdef __cplusplus
>> } /* extern "C" */
>> #endif
>
> [...]
>
>> LIBBPF_0.5.0 {
>> global:
>> + btf__add_tag;
>> bpf_map__initial_value;
>> bpf_map__pin_path;
>> bpf_map_lookup_and_delete_elem_flags;
>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
>> index 533b0211f40a..7deb86d9af51 100644
>> --- a/tools/lib/bpf/libbpf_internal.h
>> +++ b/tools/lib/bpf/libbpf_internal.h
>> @@ -69,6 +69,8 @@
>> #define BTF_VAR_SECINFO_ENC(type, offset, size) (type), (offset), (size)
>> #define BTF_TYPE_FLOAT_ENC(name, sz) \
>> BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0), sz)
>> +#define BTF_TYPE_TAG_KIND_ENC(name, type) \
>
> following other macro names, it should be BTF_TYPE_TAG_ENC, no?
This is to differentiate the case from attr to member/func_argument
which is implemented in selftest test_btf.h.
But with the new scheme, we just need BTF_TYPE_TAG_ENC. Will change.
>
>> + BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_TAG, 1, 0), type), (0)
>>
>> #ifndef likely
>> #define likely(x) __builtin_expect(!!(x), 1)
>> --
>> 2.30.2
>>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH bpf-next 4/9] bpftool: add support for BTF_KIND_TAG
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
` (2 preceding siblings ...)
2021-09-07 23:01 ` [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-09 5:28 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 5/9] selftests/bpf: test libbpf API function btf__add_tag() Yonghong Song
` (5 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
added bpftool support to dump BTF_KIND_TAG information.
The new bpftool will be used in later patches to dump
btf in the test bpf program object file.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/bpf/bpftool/btf.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index f7e5ff3586c9..89c17ea62d8e 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -37,6 +37,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
[BTF_KIND_VAR] = "VAR",
[BTF_KIND_DATASEC] = "DATASEC",
[BTF_KIND_FLOAT] = "FLOAT",
+ [BTF_KIND_TAG] = "TAG",
};
struct btf_attach_table {
@@ -347,6 +348,23 @@ static int dump_btf_type(const struct btf *btf, __u32 id,
printf(" size=%u", t->size);
break;
}
+ case BTF_KIND_TAG: {
+ const struct btf_tag *tag = (const void *)(t + 1);
+
+
+ if (json_output) {
+ jsonw_uint_field(w, "type_id", t->type);
+ if (btf_kflag(t))
+ jsonw_int_field(w, "comp_id", -1);
+ else
+ jsonw_uint_field(w, "comp_id", tag->comp_id);
+ } else if (btf_kflag(t)) {
+ printf(" type_id=%u, comp_id=-1", t->type);
+ } else {
+ printf(" type_id=%u, comp_id=%u", t->type, tag->comp_id);
+ }
+ break;
+ }
default:
break;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 4/9] bpftool: add support for BTF_KIND_TAG
2021-09-07 23:01 ` [PATCH bpf-next 4/9] bpftool: " Yonghong Song
@ 2021-09-09 5:28 ` Andrii Nakryiko
2021-09-09 5:33 ` Andrii Nakryiko
2021-09-10 16:04 ` Yonghong Song
0 siblings, 2 replies; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:28 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>
> added bpftool support to dump BTF_KIND_TAG information.
> The new bpftool will be used in later patches to dump
> btf in the test bpf program object file.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> tools/bpf/bpftool/btf.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index f7e5ff3586c9..89c17ea62d8e 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -37,6 +37,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
> [BTF_KIND_VAR] = "VAR",
> [BTF_KIND_DATASEC] = "DATASEC",
> [BTF_KIND_FLOAT] = "FLOAT",
> + [BTF_KIND_TAG] = "TAG",
> };
>
> struct btf_attach_table {
> @@ -347,6 +348,23 @@ static int dump_btf_type(const struct btf *btf, __u32 id,
> printf(" size=%u", t->size);
> break;
> }
> + case BTF_KIND_TAG: {
> + const struct btf_tag *tag = (const void *)(t + 1);
> +
> +
extra empty line?
> + if (json_output) {
> + jsonw_uint_field(w, "type_id", t->type);
> + if (btf_kflag(t))
> + jsonw_int_field(w, "comp_id", -1);
> + else
> + jsonw_uint_field(w, "comp_id", tag->comp_id);
> + } else if (btf_kflag(t)) {
> + printf(" type_id=%u, comp_id=-1", t->type);
> + } else {
> + printf(" type_id=%u, comp_id=%u", t->type, tag->comp_id);
> + }
here not using kflag would be more natural as well ;)
> + break;
> + }
> default:
> break;
> }
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 4/9] bpftool: add support for BTF_KIND_TAG
2021-09-09 5:28 ` Andrii Nakryiko
@ 2021-09-09 5:33 ` Andrii Nakryiko
2021-09-10 16:38 ` Yonghong Song
2021-09-10 16:04 ` Yonghong Song
1 sibling, 1 reply; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:33 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Wed, Sep 8, 2021 at 10:28 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
> >
> > added bpftool support to dump BTF_KIND_TAG information.
> > The new bpftool will be used in later patches to dump
> > btf in the test bpf program object file.
> >
What should be done for `bpftool btf dump file <path> format c` if BTF
contains btf_tag? Should it ignore it silently? Should it error out?
Or should we corrupt output (as will be the case right now, I think)?
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> > ---
> > tools/bpf/bpftool/btf.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> > index f7e5ff3586c9..89c17ea62d8e 100644
> > --- a/tools/bpf/bpftool/btf.c
> > +++ b/tools/bpf/bpftool/btf.c
> > @@ -37,6 +37,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
> > [BTF_KIND_VAR] = "VAR",
> > [BTF_KIND_DATASEC] = "DATASEC",
> > [BTF_KIND_FLOAT] = "FLOAT",
> > + [BTF_KIND_TAG] = "TAG",
> > };
> >
> > struct btf_attach_table {
> > @@ -347,6 +348,23 @@ static int dump_btf_type(const struct btf *btf, __u32 id,
> > printf(" size=%u", t->size);
> > break;
> > }
> > + case BTF_KIND_TAG: {
> > + const struct btf_tag *tag = (const void *)(t + 1);
> > +
> > +
>
> extra empty line?
>
> > + if (json_output) {
> > + jsonw_uint_field(w, "type_id", t->type);
> > + if (btf_kflag(t))
> > + jsonw_int_field(w, "comp_id", -1);
> > + else
> > + jsonw_uint_field(w, "comp_id", tag->comp_id);
> > + } else if (btf_kflag(t)) {
> > + printf(" type_id=%u, comp_id=-1", t->type);
> > + } else {
> > + printf(" type_id=%u, comp_id=%u", t->type, tag->comp_id);
> > + }
>
> here not using kflag would be more natural as well ;)
>
> > + break;
> > + }
> > default:
> > break;
> > }
> > --
> > 2.30.2
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 4/9] bpftool: add support for BTF_KIND_TAG
2021-09-09 5:33 ` Andrii Nakryiko
@ 2021-09-10 16:38 ` Yonghong Song
2021-09-10 18:05 ` Andrii Nakryiko
0 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-10 16:38 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On 9/8/21 10:33 PM, Andrii Nakryiko wrote:
> On Wed, Sep 8, 2021 at 10:28 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>>>
>>> added bpftool support to dump BTF_KIND_TAG information.
>>> The new bpftool will be used in later patches to dump
>>> btf in the test bpf program object file.
>>>
>
> What should be done for `bpftool btf dump file <path> format c` if BTF
> contains btf_tag? Should it ignore it silently? Should it error out?
> Or should we corrupt output (as will be the case right now, I think)?
Currently it is silently ignored. The attribute information is mostly
used in the kernel by verification purpose and the kernel uses its own
btf to check.
Adding such attributes to vmlinux.h, bpf program BTF will contain these
attributes but they may not be used by the kernel verifier at least
for now.
So I think we can delay this as a followup if there is a real need.
>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> ---
>>> tools/bpf/bpftool/btf.c | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>>> index f7e5ff3586c9..89c17ea62d8e 100644
>>> --- a/tools/bpf/bpftool/btf.c
>>> +++ b/tools/bpf/bpftool/btf.c
>>> @@ -37,6 +37,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
>>> [BTF_KIND_VAR] = "VAR",
>>> [BTF_KIND_DATASEC] = "DATASEC",
>>> [BTF_KIND_FLOAT] = "FLOAT",
>>> + [BTF_KIND_TAG] = "TAG",
>>> };
>>>
>>> struct btf_attach_table {
>>> @@ -347,6 +348,23 @@ static int dump_btf_type(const struct btf *btf, __u32 id,
>>> printf(" size=%u", t->size);
>>> break;
>>> }
>>> + case BTF_KIND_TAG: {
>>> + const struct btf_tag *tag = (const void *)(t + 1);
>>> +
>>> +
>>
>> extra empty line?
>>
>>> + if (json_output) {
>>> + jsonw_uint_field(w, "type_id", t->type);
>>> + if (btf_kflag(t))
>>> + jsonw_int_field(w, "comp_id", -1);
>>> + else
>>> + jsonw_uint_field(w, "comp_id", tag->comp_id);
>>> + } else if (btf_kflag(t)) {
>>> + printf(" type_id=%u, comp_id=-1", t->type);
>>> + } else {
>>> + printf(" type_id=%u, comp_id=%u", t->type, tag->comp_id);
>>> + }
>>
>> here not using kflag would be more natural as well ;)
>>
>>> + break;
>>> + }
>>> default:
>>> break;
>>> }
>>> --
>>> 2.30.2
>>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 4/9] bpftool: add support for BTF_KIND_TAG
2021-09-10 16:38 ` Yonghong Song
@ 2021-09-10 18:05 ` Andrii Nakryiko
0 siblings, 0 replies; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-10 18:05 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Fri, Sep 10, 2021 at 9:39 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 9/8/21 10:33 PM, Andrii Nakryiko wrote:
> > On Wed, Sep 8, 2021 at 10:28 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
> >>>
> >>> added bpftool support to dump BTF_KIND_TAG information.
> >>> The new bpftool will be used in later patches to dump
> >>> btf in the test bpf program object file.
> >>>
> >
> > What should be done for `bpftool btf dump file <path> format c` if BTF
> > contains btf_tag? Should it ignore it silently? Should it error out?
> > Or should we corrupt output (as will be the case right now, I think)?
>
> Currently it is silently ignored. The attribute information is mostly
> used in the kernel by verification purpose and the kernel uses its own
> btf to check.
>
> Adding such attributes to vmlinux.h, bpf program BTF will contain these
> attributes but they may not be used by the kernel verifier at least
> for now.
>
> So I think we can delay this as a followup if there is a real need.
Sounds good. Just wanted to confirm that we won't get some libbpf
warning emitted inside vmlinux.h, making it non-compilable.
>
> >
> >>> Signed-off-by: Yonghong Song <yhs@fb.com>
> >>> ---
> >>> tools/bpf/bpftool/btf.c | 18 ++++++++++++++++++
> >>> 1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> >>> index f7e5ff3586c9..89c17ea62d8e 100644
> >>> --- a/tools/bpf/bpftool/btf.c
> >>> +++ b/tools/bpf/bpftool/btf.c
> >>> @@ -37,6 +37,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
> >>> [BTF_KIND_VAR] = "VAR",
> >>> [BTF_KIND_DATASEC] = "DATASEC",
> >>> [BTF_KIND_FLOAT] = "FLOAT",
> >>> + [BTF_KIND_TAG] = "TAG",
> >>> };
> >>>
> >>> struct btf_attach_table {
> >>> @@ -347,6 +348,23 @@ static int dump_btf_type(const struct btf *btf, __u32 id,
> >>> printf(" size=%u", t->size);
> >>> break;
> >>> }
> >>> + case BTF_KIND_TAG: {
> >>> + const struct btf_tag *tag = (const void *)(t + 1);
> >>> +
> >>> +
> >>
> >> extra empty line?
> >>
> >>> + if (json_output) {
> >>> + jsonw_uint_field(w, "type_id", t->type);
> >>> + if (btf_kflag(t))
> >>> + jsonw_int_field(w, "comp_id", -1);
> >>> + else
> >>> + jsonw_uint_field(w, "comp_id", tag->comp_id);
> >>> + } else if (btf_kflag(t)) {
> >>> + printf(" type_id=%u, comp_id=-1", t->type);
> >>> + } else {
> >>> + printf(" type_id=%u, comp_id=%u", t->type, tag->comp_id);
> >>> + }
> >>
> >> here not using kflag would be more natural as well ;)
> >>
> >>> + break;
> >>> + }
> >>> default:
> >>> break;
> >>> }
> >>> --
> >>> 2.30.2
> >>>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH bpf-next 4/9] bpftool: add support for BTF_KIND_TAG
2021-09-09 5:28 ` Andrii Nakryiko
2021-09-09 5:33 ` Andrii Nakryiko
@ 2021-09-10 16:04 ` Yonghong Song
1 sibling, 0 replies; 31+ messages in thread
From: Yonghong Song @ 2021-09-10 16:04 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On 9/8/21 10:28 PM, Andrii Nakryiko wrote:
> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> added bpftool support to dump BTF_KIND_TAG information.
>> The new bpftool will be used in later patches to dump
>> btf in the test bpf program object file.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> tools/bpf/bpftool/btf.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>> index f7e5ff3586c9..89c17ea62d8e 100644
>> --- a/tools/bpf/bpftool/btf.c
>> +++ b/tools/bpf/bpftool/btf.c
>> @@ -37,6 +37,7 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
>> [BTF_KIND_VAR] = "VAR",
>> [BTF_KIND_DATASEC] = "DATASEC",
>> [BTF_KIND_FLOAT] = "FLOAT",
>> + [BTF_KIND_TAG] = "TAG",
>> };
>>
>> struct btf_attach_table {
>> @@ -347,6 +348,23 @@ static int dump_btf_type(const struct btf *btf, __u32 id,
>> printf(" size=%u", t->size);
>> break;
>> }
>> + case BTF_KIND_TAG: {
>> + const struct btf_tag *tag = (const void *)(t + 1);
>> +
>> +
>
> extra empty line?
ack.
>
>> + if (json_output) {
>> + jsonw_uint_field(w, "type_id", t->type);
>> + if (btf_kflag(t))
>> + jsonw_int_field(w, "comp_id", -1);
>> + else
>> + jsonw_uint_field(w, "comp_id", tag->comp_id);
>> + } else if (btf_kflag(t)) {
>> + printf(" type_id=%u, comp_id=-1", t->type);
>> + } else {
>> + printf(" type_id=%u, comp_id=%u", t->type, tag->comp_id);
>> + }
>
> here not using kflag would be more natural as well ;)
definitely.
>
>> + break;
>> + }
>> default:
>> break;
>> }
>> --
>> 2.30.2
>>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH bpf-next 5/9] selftests/bpf: test libbpf API function btf__add_tag()
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
` (3 preceding siblings ...)
2021-09-07 23:01 ` [PATCH bpf-next 4/9] bpftool: " Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-09 5:35 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 6/9] selftests/bpf: add BTF_KIND_TAG unit tests Yonghong Song
` (4 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
Add btf_write tests with btf__add_tag() function.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/testing/selftests/bpf/btf_helpers.c | 7 +++++-
.../selftests/bpf/prog_tests/btf_write.c | 23 +++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/btf_helpers.c b/tools/testing/selftests/bpf/btf_helpers.c
index b692e6ead9b5..20dc8f4cb884 100644
--- a/tools/testing/selftests/bpf/btf_helpers.c
+++ b/tools/testing/selftests/bpf/btf_helpers.c
@@ -24,11 +24,12 @@ static const char * const btf_kind_str_mapping[] = {
[BTF_KIND_VAR] = "VAR",
[BTF_KIND_DATASEC] = "DATASEC",
[BTF_KIND_FLOAT] = "FLOAT",
+ [BTF_KIND_TAG] = "TAG",
};
static const char *btf_kind_str(__u16 kind)
{
- if (kind > BTF_KIND_DATASEC)
+ if (kind > BTF_KIND_TAG)
return "UNKNOWN";
return btf_kind_str_mapping[kind];
}
@@ -177,6 +178,10 @@ int fprintf_btf_type_raw(FILE *out, const struct btf *btf, __u32 id)
case BTF_KIND_FLOAT:
fprintf(out, " size=%u", t->size);
break;
+ case BTF_KIND_TAG:
+ fprintf(out, " type_id=%u, comp_id=%d",
+ t->type, btf_kflag(t) ? -1 : (int)btf_tag(t)->comp_id);
+ break;
default:
break;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_write.c b/tools/testing/selftests/bpf/prog_tests/btf_write.c
index 022c7d89d6f4..d20db7814ed1 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_write.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_write.c
@@ -281,5 +281,28 @@ void test_btf_write() {
"[17] DATASEC 'datasec1' size=12 vlen=1\n"
"\ttype_id=1 offset=4 size=8", "raw_dump");
+ /* TAG */
+ id = btf__add_tag(btf, "tag1", -1, 16);
+ ASSERT_EQ(id, 18, "tag_id");
+ t = btf__type_by_id(btf, 18);
+ ASSERT_STREQ(btf__str_by_offset(btf, t->name_off), "tag1", "tag_name");
+ ASSERT_EQ(btf_kind(t), BTF_KIND_TAG, "tag_kind");
+ ASSERT_EQ(btf_kflag(t), true, "tag_kflag");
+ ASSERT_EQ(t->type, 16, "tag_type");
+ ASSERT_EQ(btf_tag(t)->comp_id, 0, "tag_comp_id");
+ ASSERT_STREQ(btf_type_raw_dump(btf, 18),
+ "[18] TAG 'tag1' type_id=16, comp_id=-1", "raw_dump");
+
+ id = btf__add_tag(btf, "tag2", 1, 14);
+ ASSERT_EQ(id, 19, "tag_id");
+ t = btf__type_by_id(btf, 19);
+ ASSERT_STREQ(btf__str_by_offset(btf, t->name_off), "tag2", "tag_name");
+ ASSERT_EQ(btf_kind(t), BTF_KIND_TAG, "tag_kind");
+ ASSERT_EQ(btf_kflag(t), false, "tag_kflag");
+ ASSERT_EQ(t->type, 14, "tag_type");
+ ASSERT_EQ(btf_tag(t)->comp_id, 1, "tag_comp_id");
+ ASSERT_STREQ(btf_type_raw_dump(btf, 19),
+ "[19] TAG 'tag2' type_id=14, comp_id=1", "raw_dump");
+
btf__free(btf);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 5/9] selftests/bpf: test libbpf API function btf__add_tag()
2021-09-07 23:01 ` [PATCH bpf-next 5/9] selftests/bpf: test libbpf API function btf__add_tag() Yonghong Song
@ 2021-09-09 5:35 ` Andrii Nakryiko
2021-09-10 16:39 ` Yonghong Song
0 siblings, 1 reply; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:35 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>
> Add btf_write tests with btf__add_tag() function.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> tools/testing/selftests/bpf/btf_helpers.c | 7 +++++-
> .../selftests/bpf/prog_tests/btf_write.c | 23 +++++++++++++++++++
> 2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/btf_helpers.c b/tools/testing/selftests/bpf/btf_helpers.c
> index b692e6ead9b5..20dc8f4cb884 100644
> --- a/tools/testing/selftests/bpf/btf_helpers.c
> +++ b/tools/testing/selftests/bpf/btf_helpers.c
> @@ -24,11 +24,12 @@ static const char * const btf_kind_str_mapping[] = {
> [BTF_KIND_VAR] = "VAR",
> [BTF_KIND_DATASEC] = "DATASEC",
> [BTF_KIND_FLOAT] = "FLOAT",
> + [BTF_KIND_TAG] = "TAG",
> };
>
> static const char *btf_kind_str(__u16 kind)
> {
> - if (kind > BTF_KIND_DATASEC)
> + if (kind > BTF_KIND_TAG)
> return "UNKNOWN";
> return btf_kind_str_mapping[kind];
> }
> @@ -177,6 +178,10 @@ int fprintf_btf_type_raw(FILE *out, const struct btf *btf, __u32 id)
> case BTF_KIND_FLOAT:
> fprintf(out, " size=%u", t->size);
> break;
> + case BTF_KIND_TAG:
> + fprintf(out, " type_id=%u, comp_id=%d",
seems like we use space as a separator, please remove comma for consistency
> + t->type, btf_kflag(t) ? -1 : (int)btf_tag(t)->comp_id);
> + break;
> default:
> break;
> }
[...]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 5/9] selftests/bpf: test libbpf API function btf__add_tag()
2021-09-09 5:35 ` Andrii Nakryiko
@ 2021-09-10 16:39 ` Yonghong Song
0 siblings, 0 replies; 31+ messages in thread
From: Yonghong Song @ 2021-09-10 16:39 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On 9/8/21 10:35 PM, Andrii Nakryiko wrote:
> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add btf_write tests with btf__add_tag() function.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> tools/testing/selftests/bpf/btf_helpers.c | 7 +++++-
>> .../selftests/bpf/prog_tests/btf_write.c | 23 +++++++++++++++++++
>> 2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/btf_helpers.c b/tools/testing/selftests/bpf/btf_helpers.c
>> index b692e6ead9b5..20dc8f4cb884 100644
>> --- a/tools/testing/selftests/bpf/btf_helpers.c
>> +++ b/tools/testing/selftests/bpf/btf_helpers.c
>> @@ -24,11 +24,12 @@ static const char * const btf_kind_str_mapping[] = {
>> [BTF_KIND_VAR] = "VAR",
>> [BTF_KIND_DATASEC] = "DATASEC",
>> [BTF_KIND_FLOAT] = "FLOAT",
>> + [BTF_KIND_TAG] = "TAG",
>> };
>>
>> static const char *btf_kind_str(__u16 kind)
>> {
>> - if (kind > BTF_KIND_DATASEC)
>> + if (kind > BTF_KIND_TAG)
>> return "UNKNOWN";
>> return btf_kind_str_mapping[kind];
>> }
>> @@ -177,6 +178,10 @@ int fprintf_btf_type_raw(FILE *out, const struct btf *btf, __u32 id)
>> case BTF_KIND_FLOAT:
>> fprintf(out, " size=%u", t->size);
>> break;
>> + case BTF_KIND_TAG:
>> + fprintf(out, " type_id=%u, comp_id=%d",
>
> seems like we use space as a separator, please remove comma for consistency
ack
>
>> + t->type, btf_kflag(t) ? -1 : (int)btf_tag(t)->comp_id);
>> + break;
>> default:
>> break;
>> }
>
> [...]
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH bpf-next 6/9] selftests/bpf: add BTF_KIND_TAG unit tests
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
` (4 preceding siblings ...)
2021-09-07 23:01 ` [PATCH bpf-next 5/9] selftests/bpf: test libbpf API function btf__add_tag() Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 7/9] selftests/bpf: test BTF_KIND_TAG for deduplication Yonghong Song
` (3 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
Test good and bad variants of BTF_KIND_TAG encoding.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/testing/selftests/bpf/prog_tests/btf.c | 223 +++++++++++++++++++
tools/testing/selftests/bpf/test_btf.h | 6 +
2 files changed, 229 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index 649f87382c8d..5e07a2f61486 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -3661,6 +3661,227 @@ static struct btf_raw_test raw_tests[] = {
.err_str = "Invalid type_size",
},
+{
+ .descr = "tag test #1, struct/member, well-formed",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_STRUCT_ENC(0, 2, 8), /* [2] */
+ BTF_MEMBER_ENC(NAME_TBD, 1, 0),
+ BTF_MEMBER_ENC(NAME_TBD, 1, 32),
+ BTF_TAG_KIND_ENC(NAME_TBD, 2),
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 2, 0),
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 2, 1),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0m1\0m2\0tag1\0tag2\0tag3"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 8,
+ .key_type_id = 1,
+ .value_type_id = 2,
+ .max_entries = 1,
+},
+{
+ .descr = "tag test #2, union/member, well-formed",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_STRUCT_ENC(NAME_TBD, 2, 4), /* [2] */
+ BTF_MEMBER_ENC(NAME_TBD, 1, 0),
+ BTF_MEMBER_ENC(NAME_TBD, 1, 0),
+ BTF_TAG_KIND_ENC(NAME_TBD, 2),
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 2, 0),
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 2, 1),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0t\0m1\0m2\0tag1\0tag2\0tag3"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 2,
+ .max_entries = 1,
+},
+{
+ .descr = "tag test #3, variable, well-formed",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_VAR_ENC(NAME_TBD, 1, 0), /* [2] */
+ BTF_VAR_ENC(NAME_TBD, 1, 1), /* [3] */
+ BTF_TAG_KIND_ENC(NAME_TBD, 2),
+ BTF_TAG_KIND_ENC(NAME_TBD, 3),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0local\0global\0tag1\0tag2"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+},
+{
+ .descr = "tag test #4, func/parameter, well-formed",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_FUNC_PROTO_ENC(0, 2), /* [2] */
+ BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+ BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+ BTF_FUNC_ENC(NAME_TBD, 2), /* [3] */
+ BTF_TAG_KIND_ENC(NAME_TBD, 3),
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 3, 0),
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 3, 1),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0arg1\0arg2\0f\0tag1\0tag2\0tag3"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+},
+{
+ .descr = "tag test #5, invalid name",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_VAR_ENC(NAME_TBD, 1, 0), /* [2] */
+ BTF_TAG_KIND_ENC(0, 2),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0local\0tag"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+ .btf_load_err = true,
+ .err_str = "Invalid name",
+},
+{
+ .descr = "tag test #6, invalid target type",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_TAG_KIND_ENC(NAME_TBD, 1),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0tag1"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+ .btf_load_err = true,
+ .err_str = "Invalid type",
+},
+{
+ .descr = "tag test #7, invalid vlen",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_VAR_ENC(NAME_TBD, 1, 0), /* [2] */
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_TAG, 1, 1), 2), (0),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0local\0tag1"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+ .btf_load_err = true,
+ .err_str = "vlen != 0",
+},
+{
+ .descr = "tag test #8, kflag/comp_id mismatch ",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_VAR_ENC(NAME_TBD, 1, 0), /* [2] */
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_TAG, 1, 0), 2), (1),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0local\0tag1"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+ .btf_load_err = true,
+ .err_str = "kflag/comp_id mismatch",
+},
+{
+ .descr = "tag test #9, invalid next_type",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_VAR_ENC(NAME_TBD, 1, 0), /* [2] */
+ BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_TAG, 0, 0), 2), (0),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0local\0tag"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+ .btf_load_err = true,
+ .err_str = "Invalid next_type",
+},
+{
+ .descr = "tag test #10, struct member, invalid comp_id",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_STRUCT_ENC(0, 2, 8), /* [2] */
+ BTF_MEMBER_ENC(NAME_TBD, 1, 0),
+ BTF_MEMBER_ENC(NAME_TBD, 1, 32),
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 2, 2),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0m1\0m2\0tag"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 8,
+ .key_type_id = 1,
+ .value_type_id = 2,
+ .max_entries = 1,
+ .btf_load_err = true,
+ .err_str = "Invalid comp_id",
+},
+{
+ .descr = "tag test #11, func parameter, invalid comp_id",
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_FUNC_PROTO_ENC(0, 2), /* [2] */
+ BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+ BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1),
+ BTF_FUNC_ENC(NAME_TBD, 2), /* [3] */
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 3, 2),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0arg1\0arg2\0f\0tag"),
+ .map_type = BPF_MAP_TYPE_ARRAY,
+ .map_name = "tag_type_check_btf",
+ .key_size = sizeof(int),
+ .value_size = 4,
+ .key_type_id = 1,
+ .value_type_id = 1,
+ .max_entries = 1,
+ .btf_load_err = true,
+ .err_str = "Invalid comp_id",
+},
+
}; /* struct btf_raw_test raw_tests[] */
static const char *get_next_str(const char *start, const char *end)
@@ -6801,6 +7022,8 @@ static int btf_type_size(const struct btf_type *t)
return base_size + sizeof(struct btf_var);
case BTF_KIND_DATASEC:
return base_size + vlen * sizeof(struct btf_var_secinfo);
+ case BTF_KIND_TAG:
+ return base_size + sizeof(struct btf_tag);
default:
fprintf(stderr, "Unsupported BTF_KIND:%u\n", kind);
return -EINVAL;
diff --git a/tools/testing/selftests/bpf/test_btf.h b/tools/testing/selftests/bpf/test_btf.h
index e2394eea4b7f..d0dcf5a9a8f1 100644
--- a/tools/testing/selftests/bpf/test_btf.h
+++ b/tools/testing/selftests/bpf/test_btf.h
@@ -69,4 +69,10 @@
#define BTF_TYPE_FLOAT_ENC(name, sz) \
BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0), sz)
+#define BTF_TAG_KIND_ENC(name, type) \
+ BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_TAG, 1, 0), type), (0)
+
+#define BTF_TAG_COMP_ID_ENC(name, type, comp_id) \
+ BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_TAG, 0, 0), type), (comp_id)
+
#endif /* _TEST_BTF_H */
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH bpf-next 7/9] selftests/bpf: test BTF_KIND_TAG for deduplication
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
` (5 preceding siblings ...)
2021-09-07 23:01 ` [PATCH bpf-next 6/9] selftests/bpf: add BTF_KIND_TAG unit tests Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 8/9] selftests/bpf: add a test with a bpf program with btf_tag attributes Yonghong Song
` (2 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
Add unit tests for BTF_KIND_TAG deduplication for
- struct and struct member
- variable
- func and func argument
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/testing/selftests/bpf/prog_tests/btf.c | 91 ++++++++++++++++----
1 file changed, 74 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index 5e07a2f61486..91e2709f2cac 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -6642,27 +6642,33 @@ const struct btf_dedup_test dedup_tests[] = {
BTF_MEMBER_ENC(NAME_NTH(4), 5, 64), /* const int *a; */
BTF_MEMBER_ENC(NAME_NTH(5), 2, 128), /* int b[16]; */
BTF_MEMBER_ENC(NAME_NTH(6), 1, 640), /* int c; */
- BTF_MEMBER_ENC(NAME_NTH(8), 13, 672), /* float d; */
+ BTF_MEMBER_ENC(NAME_NTH(8), 15, 672), /* float d; */
/* ptr -> [3] struct s */
BTF_PTR_ENC(3), /* [4] */
/* ptr -> [6] const int */
BTF_PTR_ENC(6), /* [5] */
/* const -> [1] int */
BTF_CONST_ENC(1), /* [6] */
+ /* tag -> [3] struct s */
+ BTF_TAG_KIND_ENC(NAME_NTH(2), 3), /* [7] */
+ /* tag -> [3] struct s, member 1 */
+ BTF_TAG_COMP_ID_ENC(NAME_NTH(2), 3, 1), /* [8] */
/* full copy of the above */
- BTF_TYPE_INT_ENC(NAME_NTH(1), BTF_INT_SIGNED, 0, 32, 4), /* [7] */
- BTF_TYPE_ARRAY_ENC(7, 7, 16), /* [8] */
- BTF_STRUCT_ENC(NAME_NTH(2), 5, 88), /* [9] */
- BTF_MEMBER_ENC(NAME_NTH(3), 10, 0),
- BTF_MEMBER_ENC(NAME_NTH(4), 11, 64),
- BTF_MEMBER_ENC(NAME_NTH(5), 8, 128),
- BTF_MEMBER_ENC(NAME_NTH(6), 7, 640),
- BTF_MEMBER_ENC(NAME_NTH(8), 13, 672),
- BTF_PTR_ENC(9), /* [10] */
- BTF_PTR_ENC(12), /* [11] */
- BTF_CONST_ENC(7), /* [12] */
- BTF_TYPE_FLOAT_ENC(NAME_NTH(7), 4), /* [13] */
+ BTF_TYPE_INT_ENC(NAME_NTH(1), BTF_INT_SIGNED, 0, 32, 4), /* [9] */
+ BTF_TYPE_ARRAY_ENC(9, 9, 16), /* [10] */
+ BTF_STRUCT_ENC(NAME_NTH(2), 5, 88), /* [11] */
+ BTF_MEMBER_ENC(NAME_NTH(3), 12, 0),
+ BTF_MEMBER_ENC(NAME_NTH(4), 13, 64),
+ BTF_MEMBER_ENC(NAME_NTH(5), 10, 128),
+ BTF_MEMBER_ENC(NAME_NTH(6), 9, 640),
+ BTF_MEMBER_ENC(NAME_NTH(8), 15, 672),
+ BTF_PTR_ENC(11), /* [12] */
+ BTF_PTR_ENC(14), /* [13] */
+ BTF_CONST_ENC(9), /* [14] */
+ BTF_TYPE_FLOAT_ENC(NAME_NTH(7), 4), /* [15] */
+ BTF_TAG_KIND_ENC(NAME_NTH(2), 11), /* [16] */
+ BTF_TAG_COMP_ID_ENC(NAME_NTH(2), 11, 1), /* [17] */
BTF_END_RAW,
},
BTF_STR_SEC("\0int\0s\0next\0a\0b\0c\0float\0d"),
@@ -6679,14 +6685,16 @@ const struct btf_dedup_test dedup_tests[] = {
BTF_MEMBER_ENC(NAME_NTH(1), 5, 64), /* const int *a; */
BTF_MEMBER_ENC(NAME_NTH(2), 2, 128), /* int b[16]; */
BTF_MEMBER_ENC(NAME_NTH(3), 1, 640), /* int c; */
- BTF_MEMBER_ENC(NAME_NTH(4), 7, 672), /* float d; */
+ BTF_MEMBER_ENC(NAME_NTH(4), 9, 672), /* float d; */
/* ptr -> [3] struct s */
BTF_PTR_ENC(3), /* [4] */
/* ptr -> [6] const int */
BTF_PTR_ENC(6), /* [5] */
/* const -> [1] int */
BTF_CONST_ENC(1), /* [6] */
- BTF_TYPE_FLOAT_ENC(NAME_NTH(7), 4), /* [7] */
+ BTF_TAG_KIND_ENC(NAME_NTH(2), 3), /* [7] */
+ BTF_TAG_COMP_ID_ENC(NAME_NTH(2), 3, 1), /* [8] */
+ BTF_TYPE_FLOAT_ENC(NAME_NTH(7), 4), /* [9] */
BTF_END_RAW,
},
BTF_STR_SEC("\0a\0b\0c\0d\0int\0float\0next\0s"),
@@ -6811,9 +6819,11 @@ const struct btf_dedup_test dedup_tests[] = {
BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 8),
BTF_FUNC_ENC(NAME_TBD, 12), /* [13] func */
BTF_TYPE_FLOAT_ENC(NAME_TBD, 2), /* [14] float */
+ BTF_TAG_KIND_ENC(NAME_TBD, 13), /* [15] tag */
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 13, 1), /* [16] tag */
BTF_END_RAW,
},
- BTF_STR_SEC("\0A\0B\0C\0D\0E\0F\0G\0H\0I\0J\0K\0L\0M\0N"),
+ BTF_STR_SEC("\0A\0B\0C\0D\0E\0F\0G\0H\0I\0J\0K\0L\0M\0N\0O\0P"),
},
.expect = {
.raw_types = {
@@ -6837,9 +6847,11 @@ const struct btf_dedup_test dedup_tests[] = {
BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 8),
BTF_FUNC_ENC(NAME_TBD, 12), /* [13] func */
BTF_TYPE_FLOAT_ENC(NAME_TBD, 2), /* [14] float */
+ BTF_TAG_KIND_ENC(NAME_TBD, 13), /* [15] tag */
+ BTF_TAG_COMP_ID_ENC(NAME_TBD, 13, 1), /* [16] tag */
BTF_END_RAW,
},
- BTF_STR_SEC("\0A\0B\0C\0D\0E\0F\0G\0H\0I\0J\0K\0L\0M\0N"),
+ BTF_STR_SEC("\0A\0B\0C\0D\0E\0F\0G\0H\0I\0J\0K\0L\0M\0N\0O\0P"),
},
.opts = {
.dont_resolve_fwds = false,
@@ -6988,6 +7000,51 @@ const struct btf_dedup_test dedup_tests[] = {
.dedup_table_size = 1
},
},
+{
+ .descr = "dedup: func/func_arg/var tags",
+ .input = {
+ .raw_types = {
+ /* int */
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ /* static int t */
+ BTF_VAR_ENC(NAME_NTH(1), 1, 0), /* [2] */
+ /* void f(int a1, int a2) */
+ BTF_FUNC_PROTO_ENC(0, 2), /* [3] */
+ BTF_FUNC_PROTO_ARG_ENC(NAME_NTH(2), 1),
+ BTF_FUNC_PROTO_ARG_ENC(NAME_NTH(3), 1),
+ BTF_FUNC_ENC(NAME_NTH(4), 2), /* [4] */
+ /* tag -> t */
+ BTF_TAG_KIND_ENC(NAME_NTH(5), 2), /* [5] */
+ BTF_TAG_KIND_ENC(NAME_NTH(5), 2), /* [6] */
+ /* tag -> func */
+ BTF_TAG_KIND_ENC(NAME_NTH(5), 4), /* [7] */
+ BTF_TAG_KIND_ENC(NAME_NTH(5), 4), /* [8] */
+ /* tag -> func arg a1 */
+ BTF_TAG_COMP_ID_ENC(NAME_NTH(5), 4, 1), /* [9] */
+ BTF_TAG_COMP_ID_ENC(NAME_NTH(5), 4, 1), /* [10] */
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0t\0a1\0a2\0f\0tag"),
+ },
+ .expect = {
+ .raw_types = {
+ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */
+ BTF_VAR_ENC(NAME_NTH(1), 1, 0), /* [2] */
+ BTF_FUNC_PROTO_ENC(0, 2), /* [3] */
+ BTF_FUNC_PROTO_ARG_ENC(NAME_NTH(2), 1),
+ BTF_FUNC_PROTO_ARG_ENC(NAME_NTH(3), 1),
+ BTF_FUNC_ENC(NAME_NTH(4), 2), /* [4] */
+ BTF_TAG_KIND_ENC(NAME_NTH(5), 2), /* [5] */
+ BTF_TAG_KIND_ENC(NAME_NTH(5), 4), /* [6] */
+ BTF_TAG_COMP_ID_ENC(NAME_NTH(5), 4, 1), /* [7] */
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0t\0a1\0a2\0f\0tag"),
+ },
+ .opts = {
+ .dont_resolve_fwds = false,
+ },
+},
};
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH bpf-next 8/9] selftests/bpf: add a test with a bpf program with btf_tag attributes
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
` (6 preceding siblings ...)
2021-09-07 23:01 ` [PATCH bpf-next 7/9] selftests/bpf: test BTF_KIND_TAG for deduplication Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-09 5:39 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG Yonghong Song
2021-09-09 22:45 ` [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Jose E. Marchesi
9 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
Add a bpf program with btf_tag attributes. The program is
loaded successfully with the kernel. With the command
bpftool btf dump file ./tag.o
the following dump shows that tags are properly encoded:
[8] STRUCT 'key_t' size=12 vlen=3
'a' type_id=2 bits_offset=0
'b' type_id=2 bits_offset=32
'c' type_id=2 bits_offset=64
[9] TAG 'tag1' type_id=8, comp_id=-1
[10] TAG 'tag2' type_id=8, comp_id=-1
[11] TAG 'tag1' type_id=8, comp_id=1
[12] TAG 'tag2' type_id=8, comp_id=1
...
[21] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
'x' type_id=2
[22] FUNC 'foo' type_id=21 linkage=static
[23] TAG 'tag1' type_id=22, comp_id=0
[24] TAG 'tag2' type_id=22, comp_id=0
[25] TAG 'tag1' type_id=22, comp_id=-1
[26] TAG 'tag2' type_id=22, comp_id=-1
...
[29] VAR 'total' type_id=27, linkage=global
[30] TAG 'tag1' type_id=29, comp_id=-1
[31] TAG 'tag2' type_id=29, comp_id=-1
If an old clang compiler, which does not support btf_tag attribute,
is used, these btf_tag attributes will be silently ignored.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
.../selftests/bpf/prog_tests/btf_tag.c | 14 +++++++
tools/testing/selftests/bpf/progs/tag.c | 39 +++++++++++++++++++
2 files changed, 53 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/btf_tag.c
create mode 100644 tools/testing/selftests/bpf/progs/tag.c
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_tag.c b/tools/testing/selftests/bpf/prog_tests/btf_tag.c
new file mode 100644
index 000000000000..f939527ede77
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/btf_tag.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include <test_progs.h>
+#include "tag.skel.h"
+
+void test_btf_tag(void)
+{
+ struct tag *skel;
+
+ skel = tag__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "btf_tag"))
+ return;
+ tag__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/tag.c b/tools/testing/selftests/bpf/progs/tag.c
new file mode 100644
index 000000000000..17f88c58a6c5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tag.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#define __tag1 __attribute__((btf_tag("tag1")))
+#define __tag2 __attribute__((btf_tag("tag2")))
+
+struct key_t {
+ int a;
+ int b __tag1 __tag2;
+ int c;
+} __tag1 __tag2;
+
+struct {
+ __uint(type, BPF_MAP_TYPE_HASH);
+ __uint(max_entries, 3);
+ __type(key, struct key_t);
+ __type(value, __u64);
+} hashmap1 SEC(".maps");
+
+__u32 total __tag1 __tag2 = 0;
+
+static __noinline int foo(int x __tag1 __tag2) __tag1 __tag2
+{
+ struct key_t key;
+ __u64 val = 1;
+
+ key.a = key.b = key.c = x;
+ bpf_map_update_elem(&hashmap1, &key, &val, 0);
+ return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(sub, int x)
+{
+ return foo(x);
+}
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 8/9] selftests/bpf: add a test with a bpf program with btf_tag attributes
2021-09-07 23:01 ` [PATCH bpf-next 8/9] selftests/bpf: add a test with a bpf program with btf_tag attributes Yonghong Song
@ 2021-09-09 5:39 ` Andrii Nakryiko
0 siblings, 0 replies; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:39 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>
> Add a bpf program with btf_tag attributes. The program is
> loaded successfully with the kernel. With the command
> bpftool btf dump file ./tag.o
> the following dump shows that tags are properly encoded:
> [8] STRUCT 'key_t' size=12 vlen=3
> 'a' type_id=2 bits_offset=0
> 'b' type_id=2 bits_offset=32
> 'c' type_id=2 bits_offset=64
> [9] TAG 'tag1' type_id=8, comp_id=-1
> [10] TAG 'tag2' type_id=8, comp_id=-1
> [11] TAG 'tag1' type_id=8, comp_id=1
> [12] TAG 'tag2' type_id=8, comp_id=1
> ...
> [21] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
> 'x' type_id=2
> [22] FUNC 'foo' type_id=21 linkage=static
> [23] TAG 'tag1' type_id=22, comp_id=0
> [24] TAG 'tag2' type_id=22, comp_id=0
> [25] TAG 'tag1' type_id=22, comp_id=-1
> [26] TAG 'tag2' type_id=22, comp_id=-1
> ...
> [29] VAR 'total' type_id=27, linkage=global
> [30] TAG 'tag1' type_id=29, comp_id=-1
> [31] TAG 'tag2' type_id=29, comp_id=-1
>
> If an old clang compiler, which does not support btf_tag attribute,
> is used, these btf_tag attributes will be silently ignored.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
LGTM.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> .../selftests/bpf/prog_tests/btf_tag.c | 14 +++++++
> tools/testing/selftests/bpf/progs/tag.c | 39 +++++++++++++++++++
> 2 files changed, 53 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/btf_tag.c
> create mode 100644 tools/testing/selftests/bpf/progs/tag.c
>
[...]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
` (7 preceding siblings ...)
2021-09-07 23:01 ` [PATCH bpf-next 8/9] selftests/bpf: add a test with a bpf program with btf_tag attributes Yonghong Song
@ 2021-09-07 23:01 ` Yonghong Song
2021-09-09 5:42 ` Andrii Nakryiko
2021-09-09 22:45 ` [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Jose E. Marchesi
9 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-07 23:01 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team
Add BTF_KIND_TAG documentation in btf.rst.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
Documentation/bpf/btf.rst | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
index 846354cd2d69..aff032686057 100644
--- a/Documentation/bpf/btf.rst
+++ b/Documentation/bpf/btf.rst
@@ -85,6 +85,7 @@ sequentially and type id is assigned to each recognized type starting from id
#define BTF_KIND_VAR 14 /* Variable */
#define BTF_KIND_DATASEC 15 /* Section */
#define BTF_KIND_FLOAT 16 /* Floating point */
+ #define BTF_KIND_TAG 17 /* Tag */
Note that the type section encodes debug info, not just pure types.
``BTF_KIND_FUNC`` is not a type, and it represents a defined subprogram.
@@ -99,14 +100,14 @@ Each type contains the following common data::
* bits 24-28: kind (e.g. int, ptr, array...etc)
* bits 29-30: unused
* bit 31: kind_flag, currently used by
- * struct, union and fwd
+ * struct, union, fwd and tag
*/
__u32 info;
/* "size" is used by INT, ENUM, STRUCT and UNION.
* "size" tells the size of the type it is describing.
*
* "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
- * FUNC and FUNC_PROTO.
+ * FUNC, FUNC_PROTO and TAG.
* "type" is a type_id referring to another type.
*/
union {
@@ -465,6 +466,31 @@ map definition.
No additional type data follow ``btf_type``.
+2.2.17 BTF_KIND_TAG
+~~~~~~~~~~~~~~~~~~~
+
+``struct btf_type`` encoding requirement:
+ * ``name_off``: offset to a non-empty string
+ * ``info.kind_flag``: 0 for tagging ``type``, 1 for tagging member/argument of the ``type``
+ * ``info.kind``: BTF_KIND_TAG
+ * ``info.vlen``: 0
+ * ``type``: ``struct``, ``union``, ``func`` or ``var``
+
+``btf_type`` is followed by ``struct btf_tag``.::
+
+ struct btf_tag {
+ __u32 comp_id;
+ };
+
+The ``name_off`` encodes btf_tag attribute string.
+If ``info.kind_flag`` is 1, the attribute is attached to the ``type``.
+If ``info.kind_flag`` is 0, the attribute is attached to either a
+``struct``/``union`` member or a ``func`` argument.
+Hence the ``type`` should be ``struct``, ``union`` or
+``func``, and ``btf_tag.comp_id``, starting from 0,
+indicates which member or argument is attached with
+the attribute.
+
3. BTF Kernel API
*****************
--
2.30.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG
2021-09-07 23:01 ` [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG Yonghong Song
@ 2021-09-09 5:42 ` Andrii Nakryiko
2021-09-10 16:40 ` Yonghong Song
0 siblings, 1 reply; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-09 5:42 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>
> Add BTF_KIND_TAG documentation in btf.rst.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> Documentation/bpf/btf.rst | 30 ++++++++++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
[...]
> +2.2.17 BTF_KIND_TAG
> +~~~~~~~~~~~~~~~~~~~
> +
> +``struct btf_type`` encoding requirement:
> + * ``name_off``: offset to a non-empty string
> + * ``info.kind_flag``: 0 for tagging ``type``, 1 for tagging member/argument of the ``type``
> + * ``info.kind``: BTF_KIND_TAG
> + * ``info.vlen``: 0
> + * ``type``: ``struct``, ``union``, ``func`` or ``var``
> +
> +``btf_type`` is followed by ``struct btf_tag``.::
> +
> + struct btf_tag {
> + __u32 comp_id;
> + };
> +
> +The ``name_off`` encodes btf_tag attribute string.
> +If ``info.kind_flag`` is 1, the attribute is attached to the ``type``.
This contradicts "info.kind_flag" description above
> +If ``info.kind_flag`` is 0, the attribute is attached to either a
> +``struct``/``union`` member or a ``func`` argument.
> +Hence the ``type`` should be ``struct``, ``union`` or
> +``func``, and ``btf_tag.comp_id``, starting from 0,
> +indicates which member or argument is attached with
> +the attribute.
Does the kernel validate this restriction for the VAR target type?
I.e., if we have kind_flag == 0 (member of type), we should disallow
VAR, right?
> +
> 3. BTF Kernel API
> *****************
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG
2021-09-09 5:42 ` Andrii Nakryiko
@ 2021-09-10 16:40 ` Yonghong Song
2021-09-10 18:05 ` Andrii Nakryiko
0 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-10 16:40 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On 9/8/21 10:42 PM, Andrii Nakryiko wrote:
> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add BTF_KIND_TAG documentation in btf.rst.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> Documentation/bpf/btf.rst | 30 ++++++++++++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>
> [...]
>
>> +2.2.17 BTF_KIND_TAG
>> +~~~~~~~~~~~~~~~~~~~
>> +
>> +``struct btf_type`` encoding requirement:
>> + * ``name_off``: offset to a non-empty string
>> + * ``info.kind_flag``: 0 for tagging ``type``, 1 for tagging member/argument of the ``type``
>> + * ``info.kind``: BTF_KIND_TAG
>> + * ``info.vlen``: 0
>> + * ``type``: ``struct``, ``union``, ``func`` or ``var``
>> +
>> +``btf_type`` is followed by ``struct btf_tag``.::
>> +
>> + struct btf_tag {
>> + __u32 comp_id;
>> + };
>> +
>> +The ``name_off`` encodes btf_tag attribute string.
>> +If ``info.kind_flag`` is 1, the attribute is attached to the ``type``.
>
> This contradicts "info.kind_flag" description above
will remove info.kind_flag stuff in the next revision.
>
>> +If ``info.kind_flag`` is 0, the attribute is attached to either a
>> +``struct``/``union`` member or a ``func`` argument.
>> +Hence the ``type`` should be ``struct``, ``union`` or
>> +``func``, and ``btf_tag.comp_id``, starting from 0,
>> +indicates which member or argument is attached with
>> +the attribute.
>
> Does the kernel validate this restriction for the VAR target type?
> I.e., if we have kind_flag == 0 (member of type), we should disallow
> VAR, right?
Yes, I even has a selftest for that.
>
>> +
>> 3. BTF Kernel API
>> *****************
>>
>> --
>> 2.30.2
>>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG
2021-09-10 16:40 ` Yonghong Song
@ 2021-09-10 18:05 ` Andrii Nakryiko
0 siblings, 0 replies; 31+ messages in thread
From: Andrii Nakryiko @ 2021-09-10 18:05 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team
On Fri, Sep 10, 2021 at 9:40 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 9/8/21 10:42 PM, Andrii Nakryiko wrote:
> > On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
> >>
> >> Add BTF_KIND_TAG documentation in btf.rst.
> >>
> >> Signed-off-by: Yonghong Song <yhs@fb.com>
> >> ---
> >> Documentation/bpf/btf.rst | 30 ++++++++++++++++++++++++++++--
> >> 1 file changed, 28 insertions(+), 2 deletions(-)
> >>
> >
> > [...]
> >
> >> +2.2.17 BTF_KIND_TAG
> >> +~~~~~~~~~~~~~~~~~~~
> >> +
> >> +``struct btf_type`` encoding requirement:
> >> + * ``name_off``: offset to a non-empty string
> >> + * ``info.kind_flag``: 0 for tagging ``type``, 1 for tagging member/argument of the ``type``
> >> + * ``info.kind``: BTF_KIND_TAG
> >> + * ``info.vlen``: 0
> >> + * ``type``: ``struct``, ``union``, ``func`` or ``var``
> >> +
> >> +``btf_type`` is followed by ``struct btf_tag``.::
> >> +
> >> + struct btf_tag {
> >> + __u32 comp_id;
> >> + };
> >> +
> >> +The ``name_off`` encodes btf_tag attribute string.
> >> +If ``info.kind_flag`` is 1, the attribute is attached to the ``type``.
> >
> > This contradicts "info.kind_flag" description above
>
> will remove info.kind_flag stuff in the next revision.
>
> >
> >> +If ``info.kind_flag`` is 0, the attribute is attached to either a
> >> +``struct``/``union`` member or a ``func`` argument.
> >> +Hence the ``type`` should be ``struct``, ``union`` or
> >> +``func``, and ``btf_tag.comp_id``, starting from 0,
> >> +indicates which member or argument is attached with
> >> +the attribute.
> >
> > Does the kernel validate this restriction for the VAR target type?
> > I.e., if we have kind_flag == 0 (member of type), we should disallow
> > VAR, right?
>
> Yes, I even has a selftest for that.
Great, must have missed it on initial review pass.
>
> >
> >> +
> >> 3. BTF Kernel API
> >> *****************
> >>
> >> --
> >> 2.30.2
> >>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
` (8 preceding siblings ...)
2021-09-07 23:01 ` [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG Yonghong Song
@ 2021-09-09 22:45 ` Jose E. Marchesi
2021-09-09 23:30 ` Yonghong Song
9 siblings, 1 reply; 31+ messages in thread
From: Jose E. Marchesi @ 2021-09-09 22:45 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, david.faust
Hi Yonghong.
> LLVM14 added support for a new C attribute ([1])
> __attribute__((btf_tag("arbitrary_str")))
> This attribute will be emitted to dwarf ([2]) and pahole
> will convert it to BTF. Or for bpf target, this
> attribute will be emitted to BTF directly ([3]).
> The attribute is intended to provide additional
> information for
> - struct/union type or struct/union member
> - static/global variables
> - static/global function or function parameter.
>
> This new attribute can be used to add attributes
> to kernel codes, e.g., pre- or post- conditions,
> allow/deny info, or any other info in which only
> the kernel is interested. Such attributes will
> be processed by clang frontend and emitted to
> dwarf, converting to BTF by pahole. Ultimiately
> the verifier can use these information for
> verification purpose.
>
> The new attribute can also be used for bpf
> programs, e.g., tagging with __user attributes
> for function parameters, specifying global
> function preconditions, etc. Such information
> may help verifier to detect user program
> bugs.
>
> After this series, pahole dwarf->btf converter
> will be enhanced to support new llvm tag
> for btf_tag attribute. With pahole support,
> we will then try to add a few real use case,
> e.g., __user/__rcu tagging, allow/deny list,
> some kernel function precondition, etc,
> in the kernel.
We are looking into implementing this in the GCC BPF port.
Supporting the new C attribute in BPF programs as a target-specific
attribute, and the new BTF kind, is straightforward enough.
However, I am afraid it will be difficult to upstream to GCC support for
a target-independent C attribute called `btf_tag' that emits a
LLVM-specific DWARF tag. Even if we proposed to use a GCC-specific
DWARF tag like DW_TAG_GNU_annotation using the same number, or better a
compiler neutral tag like DW_TAG_annotation or DW_TAG_BPF_annotation,
adding such an attribute for all targets would still likely to be much
controversial...
Would you be open to explore other, more generic, ways to convey these
annotations to pahole, something that could be easily supported by GCC,
and potentially other C compilers?
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG
2021-09-09 22:45 ` [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Jose E. Marchesi
@ 2021-09-09 23:30 ` Yonghong Song
2021-09-10 2:19 ` Jose E. Marchesi
0 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-09 23:30 UTC (permalink / raw)
To: Jose E. Marchesi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, david.faust
On 9/9/21 3:45 PM, Jose E. Marchesi wrote:
>
> Hi Yonghong.
>
>> LLVM14 added support for a new C attribute ([1])
>> __attribute__((btf_tag("arbitrary_str")))
>> This attribute will be emitted to dwarf ([2]) and pahole
>> will convert it to BTF. Or for bpf target, this
>> attribute will be emitted to BTF directly ([3]).
>> The attribute is intended to provide additional
>> information for
>> - struct/union type or struct/union member
>> - static/global variables
>> - static/global function or function parameter.
>>
>> This new attribute can be used to add attributes
>> to kernel codes, e.g., pre- or post- conditions,
>> allow/deny info, or any other info in which only
>> the kernel is interested. Such attributes will
>> be processed by clang frontend and emitted to
>> dwarf, converting to BTF by pahole. Ultimiately
>> the verifier can use these information for
>> verification purpose.
>>
>> The new attribute can also be used for bpf
>> programs, e.g., tagging with __user attributes
>> for function parameters, specifying global
>> function preconditions, etc. Such information
>> may help verifier to detect user program
>> bugs.
>>
>> After this series, pahole dwarf->btf converter
>> will be enhanced to support new llvm tag
>> for btf_tag attribute. With pahole support,
>> we will then try to add a few real use case,
>> e.g., __user/__rcu tagging, allow/deny list,
>> some kernel function precondition, etc,
>> in the kernel.
>
> We are looking into implementing this in the GCC BPF port.
Hi, Jose, thanks for your reply. It would be great if the
btf_tag can be implemented in gcc.
>
> Supporting the new C attribute in BPF programs as a target-specific
> attribute, and the new BTF kind, is straightforward enough.
>
> However, I am afraid it will be difficult to upstream to GCC support for
> a target-independent C attribute called `btf_tag' that emits a
> LLVM-specific DWARF tag. Even if we proposed to use a GCC-specific
Are you concerned with the name? The btf_tag name cames from the
discussion in
https://lore.kernel.org/bpf/CAADnVQJa=b=hoMGU213wMxyZzycPEKjAPFArKNatbVe4FvzVUA@mail.gmail.com/
as llvm guys want this attribute to be explicitly referring to bpf echo
system because we didn't implement for C++, and we didn't try to
annotate everywhere. Since its main purpose is to eventually encode in
btf (for different architectures), so we settled with btf_tag instead of
bpf_tag.
But if you have suggestion to change to a different name which can
be acceptable by both gcc and llvm community, I am okay with that.
> DWARF tag like DW_TAG_GNU_annotation using the same number, or better a
> compiler neutral tag like DW_TAG_annotation or DW_TAG_BPF_annotation,
> adding such an attribute for all targets would still likely to be much
> controversial...
This is okay too. If gcc settles with DW_TAG_GNU_annotation with a
different number (not conflict with existing other llvm tag numbers),
I think llvm can change to have the same name and number since we are
still in the release.
>
> Would you be open to explore other, more generic, ways to convey these
> annotations to pahole, something that could be easily supported by GCC,
> and potentially other C compilers?
Could you share your proposal in detail? I think some kind of difference
might be okay if it is handled by pahole and invisible to users,
although it would be good if pahole only needs to handle single
interface w.r.t. btf_tag support.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG
2021-09-09 23:30 ` Yonghong Song
@ 2021-09-10 2:19 ` Jose E. Marchesi
2021-09-10 7:04 ` Yonghong Song
0 siblings, 1 reply; 31+ messages in thread
From: Jose E. Marchesi @ 2021-09-10 2:19 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, david.faust
> On 9/9/21 3:45 PM, Jose E. Marchesi wrote:
>> Hi Yonghong.
>>
>>> LLVM14 added support for a new C attribute ([1])
>>> __attribute__((btf_tag("arbitrary_str")))
>>> This attribute will be emitted to dwarf ([2]) and pahole
>>> will convert it to BTF. Or for bpf target, this
>>> attribute will be emitted to BTF directly ([3]).
>>> The attribute is intended to provide additional
>>> information for
>>> - struct/union type or struct/union member
>>> - static/global variables
>>> - static/global function or function parameter.
>>>
>>> This new attribute can be used to add attributes
>>> to kernel codes, e.g., pre- or post- conditions,
>>> allow/deny info, or any other info in which only
>>> the kernel is interested. Such attributes will
>>> be processed by clang frontend and emitted to
>>> dwarf, converting to BTF by pahole. Ultimiately
>>> the verifier can use these information for
>>> verification purpose.
>>>
>>> The new attribute can also be used for bpf
>>> programs, e.g., tagging with __user attributes
>>> for function parameters, specifying global
>>> function preconditions, etc. Such information
>>> may help verifier to detect user program
>>> bugs.
>>>
>>> After this series, pahole dwarf->btf converter
>>> will be enhanced to support new llvm tag
>>> for btf_tag attribute. With pahole support,
>>> we will then try to add a few real use case,
>>> e.g., __user/__rcu tagging, allow/deny list,
>>> some kernel function precondition, etc,
>>> in the kernel.
>> We are looking into implementing this in the GCC BPF port.
>
> Hi, Jose, thanks for your reply. It would be great if the
> btf_tag can be implemented in gcc.
>
>> Supporting the new C attribute in BPF programs as a target-specific
>> attribute, and the new BTF kind, is straightforward enough.
>> However, I am afraid it will be difficult to upstream to GCC support
>> for
>> a target-independent C attribute called `btf_tag' that emits a
>> LLVM-specific DWARF tag. Even if we proposed to use a GCC-specific
>
> Are you concerned with the name? The btf_tag name cames from the
> discussion in
> https://lore.kernel.org/bpf/CAADnVQJa=b=hoMGU213wMxyZzycPEKjAPFArKNatbVe4FvzVUA@mail.gmail.com/
> as llvm guys want this attribute to be explicitly referring to bpf echo
> system because we didn't implement for C++, and we didn't try to
> annotate everywhere. Since its main purpose is to eventually encode in
> btf (for different architectures), so we settled with btf_tag instead of
> bpf_tag.
>
> But if you have suggestion to change to a different name which can
> be acceptable by both gcc and llvm community, I am okay with that.
I think the name of the attribute is very fine when BTF is generated
directly, like when compiling BPF programs. My concern is that the
connection `btf_tag () -> DWARF -> kernel/pahole -> BTF' may be seen as
too indirect and application-specific (the kernel) for a general-purpose
compiler attribute.
>> DWARF tag like DW_TAG_GNU_annotation using the same number, or better a
>> compiler neutral tag like DW_TAG_annotation or DW_TAG_BPF_annotation,
>> adding such an attribute for all targets would still likely to be much
>> controversial...
>
> This is okay too. If gcc settles with DW_TAG_GNU_annotation with a
> different number (not conflict with existing other llvm tag numbers),
> I think llvm can change to have the same name and number since we are
> still in the release.
Thanks, that is very nice and appreciated :) I don't think the
particular number used to encode the tag matters much, provided it
doesn't collide with any existing one of course...
However, there may be a way to entirely avoid creating a new DWARF
tag... see below.
>> Would you be open to explore other, more generic, ways to convey
>> these
>> annotations to pahole, something that could be easily supported by GCC,
>> and potentially other C compilers?
>
> Could you share your proposal in detail? I think some kind of difference
> might be okay if it is handled by pahole and invisible to users,
> although it would be good if pahole only needs to handle single
> interface w.r.t. btf_tag support.
GCC can currently generate BTF for any target, not just BPF. For
example, you can compile an object foo.o with both DWARF and BTF with:
$ gcc -c -gdwarf -gbtf foo.c
or with just BTF with:
$ gcc -c -gbtf foo.c
Binutils (ld) also supports full type deduplication for CTF, which is
very similar to BTF. We use it to build kernels in-house with CTF
enabled for dtrace. It is certainly possible to add support to ld to
also merge and deduplicate BTF sections... it is similar enough to CTF
to (hopefully) not require much work, and we were already considering
doing it anyway for other purposes.
So the proposal would be:
For GCC, we can implement the btf_tag for any target, but impacting only
the BTF output as the name implies. No effect in DWARF. Once ld is
able to merge and deduplicate BTF, it shall then be possible to build
the kernel and obtain the BTF for it without the aid of pahole, just
building it with -gdwarf -gbtf and linking normally. (We know this works
with CTF.)
For LLVM, nothing would have to be done in the short term: just use the
DWARF DIE + pahole approach already implemented. But in the medium term
LLVM could be made to 1) support emitting BTF for any target (not sure
how difficult would that be, maybe it already can do that?) and 2) to
support the -gbtf command-line option.
Then the generation of BTF for the kernel could be done in the same way
(same build rules) with both compilers, and there would be no need for
conveying the extra tags (nor any future extensions to aid the verifier
on the kernel side) to pahole via DWARF. Pure BTF all the way up (or
down) without diversion to DWARF :)
Does this make sense? WDYT?
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG
2021-09-10 2:19 ` Jose E. Marchesi
@ 2021-09-10 7:04 ` Yonghong Song
2021-09-10 8:31 ` Jose E. Marchesi
0 siblings, 1 reply; 31+ messages in thread
From: Yonghong Song @ 2021-09-10 7:04 UTC (permalink / raw)
To: Jose E. Marchesi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, david.faust
On 9/9/21 7:19 PM, Jose E. Marchesi wrote:
>
>> On 9/9/21 3:45 PM, Jose E. Marchesi wrote:
>>> Hi Yonghong.
>>>
>>>> LLVM14 added support for a new C attribute ([1])
>>>> __attribute__((btf_tag("arbitrary_str")))
>>>> This attribute will be emitted to dwarf ([2]) and pahole
>>>> will convert it to BTF. Or for bpf target, this
>>>> attribute will be emitted to BTF directly ([3]).
>>>> The attribute is intended to provide additional
>>>> information for
>>>> - struct/union type or struct/union member
>>>> - static/global variables
>>>> - static/global function or function parameter.
>>>>
>>>> This new attribute can be used to add attributes
>>>> to kernel codes, e.g., pre- or post- conditions,
>>>> allow/deny info, or any other info in which only
>>>> the kernel is interested. Such attributes will
>>>> be processed by clang frontend and emitted to
>>>> dwarf, converting to BTF by pahole. Ultimiately
>>>> the verifier can use these information for
>>>> verification purpose.
>>>>
>>>> The new attribute can also be used for bpf
>>>> programs, e.g., tagging with __user attributes
>>>> for function parameters, specifying global
>>>> function preconditions, etc. Such information
>>>> may help verifier to detect user program
>>>> bugs.
>>>>
>>>> After this series, pahole dwarf->btf converter
>>>> will be enhanced to support new llvm tag
>>>> for btf_tag attribute. With pahole support,
>>>> we will then try to add a few real use case,
>>>> e.g., __user/__rcu tagging, allow/deny list,
>>>> some kernel function precondition, etc,
>>>> in the kernel.
>>> We are looking into implementing this in the GCC BPF port.
>>
>> Hi, Jose, thanks for your reply. It would be great if the
>> btf_tag can be implemented in gcc.
>>
>>> Supporting the new C attribute in BPF programs as a target-specific
>>> attribute, and the new BTF kind, is straightforward enough.
>>> However, I am afraid it will be difficult to upstream to GCC support
>>> for
>>> a target-independent C attribute called `btf_tag' that emits a
>>> LLVM-specific DWARF tag. Even if we proposed to use a GCC-specific
>>
>> Are you concerned with the name? The btf_tag name cames from the
>> discussion in
>> https://lore.kernel.org/bpf/CAADnVQJa=b=hoMGU213wMxyZzycPEKjAPFArKNatbVe4FvzVUA@mail.gmail.com/
>> as llvm guys want this attribute to be explicitly referring to bpf echo
>> system because we didn't implement for C++, and we didn't try to
>> annotate everywhere. Since its main purpose is to eventually encode in
>> btf (for different architectures), so we settled with btf_tag instead of
>> bpf_tag.
>>
>> But if you have suggestion to change to a different name which can
>> be acceptable by both gcc and llvm community, I am okay with that.
>
> I think the name of the attribute is very fine when BTF is generated
> directly, like when compiling BPF programs. My concern is that the
> connection `btf_tag () -> DWARF -> kernel/pahole -> BTF' may be seen as
> too indirect and application-specific (the kernel) for a general-purpose
> compiler attribute.
For llvm, btf_tag implies implementation scope as it *only covers* btf
use cases. There are some other use cases which may utilize the same
IR/dwarf implementation, but they may use a flag to control or different
attribute. And this has been agreed upon with llvm community, so we
should be okay here.
>
>>> DWARF tag like DW_TAG_GNU_annotation using the same number, or better a
>>> compiler neutral tag like DW_TAG_annotation or DW_TAG_BPF_annotation,
>>> adding such an attribute for all targets would still likely to be much
>>> controversial...
>>
>> This is okay too. If gcc settles with DW_TAG_GNU_annotation with a
>> different number (not conflict with existing other llvm tag numbers),
>> I think llvm can change to have the same name and number since we are
>> still in the release.
>
> Thanks, that is very nice and appreciated :) I don't think the
> particular number used to encode the tag matters much, provided it
> doesn't collide with any existing one of course...
>
> However, there may be a way to entirely avoid creating a new DWARF
> tag... see below.
>
>>> Would you be open to explore other, more generic, ways to convey
>>> these
>>> annotations to pahole, something that could be easily supported by GCC,
>>> and potentially other C compilers?
>>
>> Could you share your proposal in detail? I think some kind of difference
>> might be okay if it is handled by pahole and invisible to users,
>> although it would be good if pahole only needs to handle single
>> interface w.r.t. btf_tag support.
>
> GCC can currently generate BTF for any target, not just BPF. For
> example, you can compile an object foo.o with both DWARF and BTF with:
>
> $ gcc -c -gdwarf -gbtf foo.c
>
> or with just BTF with:
>
> $ gcc -c -gbtf foo.c
>
> Binutils (ld) also supports full type deduplication for CTF, which is
> very similar to BTF. We use it to build kernels in-house with CTF
> enabled for dtrace. It is certainly possible to add support to ld to
> also merge and deduplicate BTF sections... it is similar enough to CTF
> to (hopefully) not require much work, and we were already considering
> doing it anyway for other purposes.
>
> So the proposal would be:
>
> For GCC, we can implement the btf_tag for any target, but impacting only
> the BTF output as the name implies. No effect in DWARF. Once ld is
> able to merge and deduplicate BTF, it shall then be possible to build
> the kernel and obtain the BTF for it without the aid of pahole, just
> building it with -gdwarf -gbtf and linking normally. (We know this works
> with CTF.)
This should be okay.
>
> For LLVM, nothing would have to be done in the short term: just use the
> DWARF DIE + pahole approach already implemented. But in the medium term
> LLVM could be made to 1) support emitting BTF for any target (not sure
> how difficult would that be, maybe it already can do that?) and 2) to
> support the -gbtf command-line option.
>
> Then the generation of BTF for the kernel could be done in the same way
> (same build rules) with both compilers, and there would be no need for
> conveying the extra tags (nor any future extensions to aid the verifier
> on the kernel side) to pahole via DWARF. Pure BTF all the way up (or
> down) without diversion to DWARF :)
>
> Does this make sense? WDYT?
During discussion to implement btf_tag attribute, I actually have a
prototype to emit BTF with non-BPF targets in llvm.
see https://reviews.llvm.org/D103549
But since we get a simpler solution to emit the info to llvm, so we went
there. We will keep this in mind, it is totally possible in the future
we may start to directly generate BTF from llvm for all architectures.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG
2021-09-10 7:04 ` Yonghong Song
@ 2021-09-10 8:31 ` Jose E. Marchesi
0 siblings, 0 replies; 31+ messages in thread
From: Jose E. Marchesi @ 2021-09-10 8:31 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, david.faust
> On 9/9/21 7:19 PM, Jose E. Marchesi wrote:
>>
>>> On 9/9/21 3:45 PM, Jose E. Marchesi wrote:
>>>> Hi Yonghong.
>>>>
>>>>> LLVM14 added support for a new C attribute ([1])
>>>>> __attribute__((btf_tag("arbitrary_str")))
>>>>> This attribute will be emitted to dwarf ([2]) and pahole
>>>>> will convert it to BTF. Or for bpf target, this
>>>>> attribute will be emitted to BTF directly ([3]).
>>>>> The attribute is intended to provide additional
>>>>> information for
>>>>> - struct/union type or struct/union member
>>>>> - static/global variables
>>>>> - static/global function or function parameter.
>>>>>
>>>>> This new attribute can be used to add attributes
>>>>> to kernel codes, e.g., pre- or post- conditions,
>>>>> allow/deny info, or any other info in which only
>>>>> the kernel is interested. Such attributes will
>>>>> be processed by clang frontend and emitted to
>>>>> dwarf, converting to BTF by pahole. Ultimiately
>>>>> the verifier can use these information for
>>>>> verification purpose.
>>>>>
>>>>> The new attribute can also be used for bpf
>>>>> programs, e.g., tagging with __user attributes
>>>>> for function parameters, specifying global
>>>>> function preconditions, etc. Such information
>>>>> may help verifier to detect user program
>>>>> bugs.
>>>>>
>>>>> After this series, pahole dwarf->btf converter
>>>>> will be enhanced to support new llvm tag
>>>>> for btf_tag attribute. With pahole support,
>>>>> we will then try to add a few real use case,
>>>>> e.g., __user/__rcu tagging, allow/deny list,
>>>>> some kernel function precondition, etc,
>>>>> in the kernel.
>>>> We are looking into implementing this in the GCC BPF port.
>>>
>>> Hi, Jose, thanks for your reply. It would be great if the
>>> btf_tag can be implemented in gcc.
>>>
>>>> Supporting the new C attribute in BPF programs as a target-specific
>>>> attribute, and the new BTF kind, is straightforward enough.
>>>> However, I am afraid it will be difficult to upstream to GCC support
>>>> for
>>>> a target-independent C attribute called `btf_tag' that emits a
>>>> LLVM-specific DWARF tag. Even if we proposed to use a GCC-specific
>>>
>>> Are you concerned with the name? The btf_tag name cames from the
>>> discussion in
>>> https://lore.kernel.org/bpf/CAADnVQJa=b=hoMGU213wMxyZzycPEKjAPFArKNatbVe4FvzVUA@mail.gmail.com/
>>> as llvm guys want this attribute to be explicitly referring to bpf echo
>>> system because we didn't implement for C++, and we didn't try to
>>> annotate everywhere. Since its main purpose is to eventually encode in
>>> btf (for different architectures), so we settled with btf_tag instead of
>>> bpf_tag.
>>>
>>> But if you have suggestion to change to a different name which can
>>> be acceptable by both gcc and llvm community, I am okay with that.
>> I think the name of the attribute is very fine when BTF is generated
>> directly, like when compiling BPF programs. My concern is that the
>> connection `btf_tag () -> DWARF -> kernel/pahole -> BTF' may be seen as
>> too indirect and application-specific (the kernel) for a general-purpose
>> compiler attribute.
>
> For llvm, btf_tag implies implementation scope as it *only covers* btf
> use cases. There are some other use cases which may utilize the same
> IR/dwarf implementation, but they may use a flag to control or different
> attribute. And this has been agreed upon with llvm community, so we
> should be okay here.
>
>>>> DWARF tag like DW_TAG_GNU_annotation using the same number, or better a
>>>> compiler neutral tag like DW_TAG_annotation or DW_TAG_BPF_annotation,
>>>> adding such an attribute for all targets would still likely to be much
>>>> controversial...
>>>
>>> This is okay too. If gcc settles with DW_TAG_GNU_annotation with a
>>> different number (not conflict with existing other llvm tag numbers),
>>> I think llvm can change to have the same name and number since we are
>>> still in the release.
>> Thanks, that is very nice and appreciated :) I don't think the
>> particular number used to encode the tag matters much, provided it
>> doesn't collide with any existing one of course...
>> However, there may be a way to entirely avoid creating a new DWARF
>> tag... see below.
>>
>>>> Would you be open to explore other, more generic, ways to convey
>>>> these
>>>> annotations to pahole, something that could be easily supported by GCC,
>>>> and potentially other C compilers?
>>>
>>> Could you share your proposal in detail? I think some kind of difference
>>> might be okay if it is handled by pahole and invisible to users,
>>> although it would be good if pahole only needs to handle single
>>> interface w.r.t. btf_tag support.
>> GCC can currently generate BTF for any target, not just BPF. For
>> example, you can compile an object foo.o with both DWARF and BTF with:
>> $ gcc -c -gdwarf -gbtf foo.c
>> or with just BTF with:
>> $ gcc -c -gbtf foo.c
>> Binutils (ld) also supports full type deduplication for CTF, which
>> is
>> very similar to BTF. We use it to build kernels in-house with CTF
>> enabled for dtrace. It is certainly possible to add support to ld to
>> also merge and deduplicate BTF sections... it is similar enough to CTF
>> to (hopefully) not require much work, and we were already considering
>> doing it anyway for other purposes.
>> So the proposal would be:
>> For GCC, we can implement the btf_tag for any target, but impacting
>> only
>> the BTF output as the name implies. No effect in DWARF. Once ld is
>> able to merge and deduplicate BTF, it shall then be possible to build
>> the kernel and obtain the BTF for it without the aid of pahole, just
>> building it with -gdwarf -gbtf and linking normally. (We know this works
>> with CTF.)
>
> This should be okay.
Super. We will work in this direction.
>> For LLVM, nothing would have to be done in the short term: just use
>> the
>> DWARF DIE + pahole approach already implemented. But in the medium term
>> LLVM could be made to 1) support emitting BTF for any target (not sure
>> how difficult would that be, maybe it already can do that?) and 2) to
>> support the -gbtf command-line option.
>> Then the generation of BTF for the kernel could be done in the same
>> way
>> (same build rules) with both compilers, and there would be no need for
>> conveying the extra tags (nor any future extensions to aid the verifier
>> on the kernel side) to pahole via DWARF. Pure BTF all the way up (or
>> down) without diversion to DWARF :)
>> Does this make sense? WDYT?
>
> During discussion to implement btf_tag attribute, I actually have a
> prototype to emit BTF with non-BPF targets in llvm.
> see https://reviews.llvm.org/D103549
>
> But since we get a simpler solution to emit the info to llvm, so we went
> there. We will keep this in mind, it is totally possible in the future
> we may start to directly generate BTF from llvm for all architectures.
Nice :)
Thanks.
^ permalink raw reply [flat|nested] 31+ messages in thread