From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 1/9] bpf: support for new btf kind BTF_KIND_TAG
Date: Fri, 10 Sep 2021 08:55:43 -0700 [thread overview]
Message-ID: <9f58f7d3-e517-81ad-2bc8-07efa8689f32@fb.com> (raw)
In-Reply-To: <CAEf4BzYUhFZf_Kt+uQ1k4N1k_H3uJd2A9-FqSF9HbcfvLYUO4Q@mail.gmail.com>
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,
>> +};
>> +
>
> [...]
>
next prev parent reply other threads:[~2021-09-10 15:56 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
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-09 5:09 ` Andrii Nakryiko
2021-09-10 15:55 ` Yonghong Song [this message]
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
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
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:38 ` Yonghong Song
2021-09-10 18:05 ` Andrii Nakryiko
2021-09-10 16:04 ` Yonghong Song
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
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 ` [PATCH bpf-next 7/9] selftests/bpf: test BTF_KIND_TAG for deduplication 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
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 5:42 ` Andrii Nakryiko
2021-09-10 16:40 ` Yonghong Song
2021-09-10 18:05 ` 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
2021-09-09 23:30 ` Yonghong Song
2021-09-10 2:19 ` Jose E. Marchesi
2021-09-10 7:04 ` Yonghong Song
2021-09-10 8:31 ` Jose E. Marchesi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9f58f7d3-e517-81ad-2bc8-07efa8689f32@fb.com \
--to=yhs@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox