* [PATCH bpf-next v2 00/13] bpf: add btf func info support @ 2018-10-17 7:23 Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 01/13] bpf: btf: Break up btf_type_is_void() Yonghong Song ` (4 more replies) 0 siblings, 5 replies; 37+ messages in thread From: Yonghong Song @ 2018-10-17 7:23 UTC (permalink / raw) To: ast, kafai, daniel, netdev; +Cc: kernel-team The BTF support was added to kernel by Commit 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)"), which introduced .BTF section into ELF file and is primarily used for map pretty print. pahole is used to convert dwarf to BTF for ELF files. This patch added func info support to the kernel so we can get better ksym's for bpf function calls. Basically, pairs of bpf function calls and their corresponding types are passed to the kernel. Extracting function names from the types, the kernel is able to construct a ksym for each function call with embedded function name. LLVM patch https://reviews.llvm.org/D53261 can generate func info, encoded in .BTF.ext ELF section. In addition, it also added support for FUNC and FUNC_PROTO types, which are also supported through this patch set. The following is an example to show FUNC and FUNC_PROTO difference, compiled with the above LLVM patch with Debug mode. -bash-4.2$ cat test.c int foo(int (*bar)(int)) { return bar(5); } -bash-4.2$ clang -target bpf -g -O2 -mllvm -debug-only=btf -c test.c Type Table: [1] FUNC name_off=1 info=0x0c000001 size/type=2 param_type=3 [2] INT name_off=11 info=0x01000000 size/type=4 desc=0x01000020 [3] PTR name_off=0 info=0x02000000 size/type=4 [4] FUNC_PROTO name_off=0 info=0x0d000001 size/type=2 param_type=2 String Table: 0 : 1 : foo 5 : .text 11 : int 15 : test.c 22 : int foo(int (*bar)(int)) { return bar(5); } FuncInfo Table: sec_name_off=5 insn_offset=<Omitted> type_id=1 ... In the above, type and string tables are in .BTF section and the func info in .BTF.ext. The "<Omitted>" is the insn offset which is not available during the dump time but resolved during later compilation process. Following the format specification at Patch #9 and examine the raw data in .BTF.ext section, we have FuncInfo Table: sec_name_off=5 insn_offset=0 type_id=1 The (insn_offset, type_id) can be passed to the kernel so the kernel can find the func name and use it in the ksym. Below is a demonstration from Patch #13. $ bpftool prog dump jited id 1 int _dummy_tracepoint(struct dummy_tracepoint_args * ): bpf_prog_b07ccb89267cf242__dummy_tracepoint: 0: push %rbp 1: mov %rsp,%rbp ...... 3c: add $0x28,%rbp 40: leaveq 41: retq int test_long_fname_1(struct dummy_tracepoint_args * ): bpf_prog_2dcecc18072623fc_test_long_fname_1: 0: push %rbp 1: mov %rsp,%rbp ...... 3a: add $0x28,%rbp 3e: leaveq 3f: retq int test_long_fname_2(struct dummy_tracepoint_args * ): bpf_prog_89d64e4abf0f0126_test_long_fname_2: 0: push %rbp 1: mov %rsp,%rbp ...... 80: add $0x28,%rbp 84: leaveq 85: retq For the patchset, Patch #1 refactors the code to break up btf_type_is_void(). Patch #2 introduces new BTF types BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO. Patch #3 syncs btf.h header to tools directory. Patch #4 adds btf func/func_proto self tests in test_btf. Patch #5 adds kernel interface to load func_info to kernel and pass func_info to userspace. Patch #6 syncs bpf.h header to tools directory. Patch #7 adds news btf/func_info related fields in libbpf program load function. Patch #8 extends selftest test_btf to test load/retrieve func_type info. Patch #9 adds .BTF.ext func info support. Patch #10 changes Makefile to avoid using pahole if llvm is capable of generating BTF sections. Patch #11 refactors to have btf_get_from_id() in libbpf for reuse. Patch #12 enhance test_btf file testing to test func info. Patch #13 adds bpftool support for func signature dump. Yonghong Song (13): bpf: btf: Break up btf_type_is_void() bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO tools/bpf: sync kernel btf.h header tools/bpf: add btf func/func_proto unit tests in selftest test_btf bpf: get better bpf_prog ksyms based on btf func type_id tools/bpf: sync kernel uapi bpf.h header to tools directory tools/bpf: add new fields for program load in lib/bpf tools/bpf: extends test_btf to test load/retrieve func_type info tools/bpf: add support to read .BTF.ext sections tools/bpf: do not use pahole if clang/llvm can generate BTF sections tools/bpf: refactor to implement btf_get_from_id() in lib/bpf tools/bpf: enhance test_btf file testing to test func info tools/bpf: bpftool: add support for jited func types Changelogs: v1 -> v2: . Added missing sign-off . Limited the func_name/struct_member_name length for validity test . Removed/changed several verifier messages . Modified several commit messages to remove line_off reference include/linux/bpf.h | 2 + include/linux/bpf_verifier.h | 1 + include/linux/btf.h | 2 + include/uapi/linux/bpf.h | 11 + include/uapi/linux/btf.h | 9 +- kernel/bpf/btf.c | 325 +++++++++-- kernel/bpf/core.c | 10 + kernel/bpf/syscall.c | 83 ++- kernel/bpf/verifier.c | 46 ++ samples/bpf/Makefile | 8 + tools/bpf/bpftool/btf_dumper.c | 96 ++++ tools/bpf/bpftool/main.h | 2 + tools/bpf/bpftool/map.c | 68 +-- tools/bpf/bpftool/prog.c | 54 ++ tools/include/uapi/linux/bpf.h | 11 + tools/include/uapi/linux/btf.h | 9 +- tools/lib/bpf/bpf.c | 3 + tools/lib/bpf/bpf.h | 3 + tools/lib/bpf/btf.c | 305 ++++++++++ tools/lib/bpf/btf.h | 33 ++ tools/lib/bpf/libbpf.c | 53 +- tools/testing/selftests/bpf/Makefile | 8 + tools/testing/selftests/bpf/test_btf.c | 566 ++++++++++++++++++- tools/testing/selftests/bpf/test_btf_haskv.c | 16 +- tools/testing/selftests/bpf/test_btf_nokv.c | 16 +- 25 files changed, 1611 insertions(+), 129 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH bpf-next v2 01/13] bpf: btf: Break up btf_type_is_void() 2018-10-17 7:23 [PATCH bpf-next v2 00/13] bpf: add btf func info support Yonghong Song @ 2018-10-17 7:23 ` Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO Yonghong Song ` (3 subsequent siblings) 4 siblings, 0 replies; 37+ messages in thread From: Yonghong Song @ 2018-10-17 7:23 UTC (permalink / raw) To: ast, kafai, daniel, netdev; +Cc: kernel-team This patch breaks up btf_type_is_void() into btf_type_is_void() and btf_type_is_fwd(). It also adds btf_type_nosize() to better describe it is testing a type has nosize info. Signed-off-by: Martin KaFai Lau <kafai@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/btf.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 378cef70341c..be406d8906ce 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -306,15 +306,22 @@ static bool btf_type_is_modifier(const struct btf_type *t) static bool btf_type_is_void(const struct btf_type *t) { - /* void => no type and size info. - * Hence, FWD is also treated as void. - */ - return t == &btf_void || BTF_INFO_KIND(t->info) == BTF_KIND_FWD; + return t == &btf_void; +} + +static bool btf_type_is_fwd(const struct btf_type *t) +{ + return BTF_INFO_KIND(t->info) == BTF_KIND_FWD; +} + +static bool btf_type_nosize(const struct btf_type *t) +{ + return btf_type_is_void(t) || btf_type_is_fwd(t); } -static bool btf_type_is_void_or_null(const struct btf_type *t) +static bool btf_type_nosize_or_null(const struct btf_type *t) { - return !t || btf_type_is_void(t); + return !t || btf_type_nosize(t); } /* union is only a special case of struct: @@ -826,7 +833,7 @@ const struct btf_type *btf_type_id_size(const struct btf *btf, u32 size = 0; size_type = btf_type_by_id(btf, size_type_id); - if (btf_type_is_void_or_null(size_type)) + if (btf_type_nosize_or_null(size_type)) return NULL; if (btf_type_has_size(size_type)) { @@ -842,7 +849,7 @@ const struct btf_type *btf_type_id_size(const struct btf *btf, size = btf->resolved_sizes[size_type_id]; size_type_id = btf->resolved_ids[size_type_id]; size_type = btf_type_by_id(btf, size_type_id); - if (btf_type_is_void(size_type)) + if (btf_type_nosize_or_null(size_type)) return NULL; } @@ -1164,7 +1171,7 @@ static int btf_modifier_resolve(struct btf_verifier_env *env, } /* "typedef void new_void", "const void"...etc */ - if (btf_type_is_void(next_type)) + if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type)) goto resolved; if (!env_type_is_resolve_sink(env, next_type) && @@ -1178,7 +1185,7 @@ static int btf_modifier_resolve(struct btf_verifier_env *env, * pretty print). */ if (!btf_type_id_size(btf, &next_type_id, &next_type_size) && - !btf_type_is_void(btf_type_id_resolve(btf, &next_type_id))) { + !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) { btf_verifier_log_type(env, v->t, "Invalid type_id"); return -EINVAL; } @@ -1205,7 +1212,7 @@ static int btf_ptr_resolve(struct btf_verifier_env *env, } /* "void *" */ - if (btf_type_is_void(next_type)) + if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type)) goto resolved; if (!env_type_is_resolve_sink(env, next_type) && @@ -1235,7 +1242,7 @@ static int btf_ptr_resolve(struct btf_verifier_env *env, } if (!btf_type_id_size(btf, &next_type_id, &next_type_size) && - !btf_type_is_void(btf_type_id_resolve(btf, &next_type_id))) { + !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) { btf_verifier_log_type(env, v->t, "Invalid type_id"); return -EINVAL; } @@ -1396,7 +1403,7 @@ static int btf_array_resolve(struct btf_verifier_env *env, /* Check array->index_type */ index_type_id = array->index_type; index_type = btf_type_by_id(btf, index_type_id); - if (btf_type_is_void_or_null(index_type)) { + if (btf_type_nosize_or_null(index_type)) { btf_verifier_log_type(env, v->t, "Invalid index"); return -EINVAL; } @@ -1415,7 +1422,7 @@ static int btf_array_resolve(struct btf_verifier_env *env, /* Check array->type */ elem_type_id = array->type; elem_type = btf_type_by_id(btf, elem_type_id); - if (btf_type_is_void_or_null(elem_type)) { + if (btf_type_nosize_or_null(elem_type)) { btf_verifier_log_type(env, v->t, "Invalid elem"); return -EINVAL; @@ -1615,7 +1622,7 @@ static int btf_struct_resolve(struct btf_verifier_env *env, const struct btf_type *member_type = btf_type_by_id(env->btf, member_type_id); - if (btf_type_is_void_or_null(member_type)) { + if (btf_type_nosize_or_null(member_type)) { btf_verifier_log_member(env, v->t, member, "Invalid member"); return -EINVAL; -- 2.17.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-17 7:23 [PATCH bpf-next v2 00/13] bpf: add btf func info support Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 01/13] bpf: btf: Break up btf_type_is_void() Yonghong Song @ 2018-10-17 7:23 ` Yonghong Song 2018-10-17 16:13 ` Edward Cree 2018-10-17 7:23 ` [PATCH bpf-next v2 03/13] tools/bpf: sync kernel btf.h header Yonghong Song ` (2 subsequent siblings) 4 siblings, 1 reply; 37+ messages in thread From: Yonghong Song @ 2018-10-17 7:23 UTC (permalink / raw) To: ast, kafai, daniel, netdev; +Cc: kernel-team This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO support to the type section. BTF_KIND_FUNC_PROTO is used to specify the type of a function pointer. With this, BTF has a complete set of C types (except float). BTF_KIND_FUNC is used to specify the signature of a defined subprogram. BTF_KIND_FUNC_PROTO can be referenced by another type, e.g., a pointer type, and BTF_KIND_FUNC type cannot be referenced by another type. For both BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO types, the func return type is in t->type (where t is a "struct btf_type" object). The func args are an array of u32s immediately following object "t". As a concrete example, for the C program below, $ cat test.c int foo(int (*bar)(int)) { return bar(5); } with LLVM patch https://reviews.llvm.org/D53261 in Debug mode, we have $ clang -target bpf -g -O2 -mllvm -debug-only=btf -c test.c Type Table: [1] FUNC NameOff=1 Info=0x0c000001 Size/Type=2 ParamType=3 [2] INT NameOff=11 Info=0x01000000 Size/Type=4 Desc=0x01000020 [3] PTR NameOff=0 Info=0x02000000 Size/Type=4 [4] FUNC_PROTO NameOff=0 Info=0x0d000001 Size/Type=2 ParamType=2 String Table: 0 : 1 : foo 5 : .text 11 : int 15 : test.c 22 : int foo(int (*bar)(int)) { return bar(5); } FuncInfo Table: SecNameOff=5 InsnOffset=<Omitted> TypeId=1 ... (Eventually we shall have bpftool to dump btf information like the above.) Function "foo" has a FUNC type (type_id = 1). The parameter of "foo" has type_id 3 which is PTR->FUNC_PROTO, where FUNC_PROTO refers to function pointer "bar". In FuncInfo Table, for section .text, the function, with to-be-determined offset (marked as <Omitted>), has type_id=1 which refers to a FUNC type. This way, the function signature is available to both kernel and user space. Here, the insn offset is not available during the dump time as relocation is resolved pretty late in the compilation process. Signed-off-by: Martin KaFai Lau <kafai@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- include/uapi/linux/btf.h | 9 +- kernel/bpf/btf.c | 280 ++++++++++++++++++++++++++++++++++----- 2 files changed, 253 insertions(+), 36 deletions(-) diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h index 972265f32871..63f8500e6f34 100644 --- a/include/uapi/linux/btf.h +++ b/include/uapi/linux/btf.h @@ -40,7 +40,8 @@ struct btf_type { /* "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 and RESTRICT. + * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, + * FUNC and FUNC_PROTO. * "type" is a type_id referring to another type. */ union { @@ -64,8 +65,10 @@ struct btf_type { #define BTF_KIND_VOLATILE 9 /* Volatile */ #define BTF_KIND_CONST 10 /* Const */ #define BTF_KIND_RESTRICT 11 /* Restrict */ -#define BTF_KIND_MAX 11 -#define NR_BTF_KINDS 12 +#define BTF_KIND_FUNC 12 /* Function */ +#define BTF_KIND_FUNC_PROTO 13 /* Function Prototype */ +#define BTF_KIND_MAX 13 +#define NR_BTF_KINDS 14 /* For some specific BTF_KIND, "struct btf_type" is immediately * followed by extra data. diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index be406d8906ce..763f8e06bc91 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -5,6 +5,7 @@ #include <uapi/linux/types.h> #include <linux/seq_file.h> #include <linux/compiler.h> +#include <linux/ctype.h> #include <linux/errno.h> #include <linux/slab.h> #include <linux/anon_inodes.h> @@ -259,6 +260,8 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = { [BTF_KIND_VOLATILE] = "VOLATILE", [BTF_KIND_CONST] = "CONST", [BTF_KIND_RESTRICT] = "RESTRICT", + [BTF_KIND_FUNC] = "FUNC", + [BTF_KIND_FUNC_PROTO] = "FUNC_PROTO", }; struct btf_kind_operations { @@ -281,6 +284,9 @@ struct btf_kind_operations { static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS]; static struct btf_type btf_void; +static int btf_resolve(struct btf_verifier_env *env, + const struct btf_type *t, u32 type_id); + static bool btf_type_is_modifier(const struct btf_type *t) { /* Some of them is not strictly a C modifier @@ -314,9 +320,20 @@ static bool btf_type_is_fwd(const struct btf_type *t) return BTF_INFO_KIND(t->info) == BTF_KIND_FWD; } +static bool btf_type_is_func(const struct btf_type *t) +{ + return BTF_INFO_KIND(t->info) == BTF_KIND_FUNC; +} + +static bool btf_type_is_func_proto(const struct btf_type *t) +{ + return BTF_INFO_KIND(t->info) == BTF_KIND_FUNC_PROTO; +} + static bool btf_type_nosize(const struct btf_type *t) { - return btf_type_is_void(t) || btf_type_is_fwd(t); + return btf_type_is_void(t) || btf_type_is_fwd(t) || + btf_type_is_func(t) || btf_type_is_func_proto(t); } static bool btf_type_nosize_or_null(const struct btf_type *t) @@ -433,6 +450,27 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset) offset < btf->hdr.str_len; } +static bool btf_name_valid_identifier(const struct btf *btf, u32 offset) +{ + /* offset must be valid */ + const char *src = &btf->strings[offset]; + const char *src_limit; + + if (!isalpha(*src) && *src != '_') + return false; + + /* set a limit on identifier length */ + src_limit = src + KSYM_NAME_LEN; + src++; + while (*src && src < src_limit) { + if (!isalnum(*src) && *src != '_') + return false; + src++; + } + + return !*src; +} + static const char *btf_name_by_offset(const struct btf *btf, u32 offset) { if (!offset) @@ -747,7 +785,9 @@ static bool env_type_is_resolve_sink(const struct btf_verifier_env *env, /* int, enum or void is a sink */ return !btf_type_needs_resolve(next_type); case RESOLVE_PTR: - /* int, enum, void, struct or array is a sink for ptr */ + /* int, enum, void, struct, array or func_ptoto is a sink + * for ptr + */ return !btf_type_is_modifier(next_type) && !btf_type_is_ptr(next_type); case RESOLVE_STRUCT_OR_ARRAY: @@ -1170,6 +1210,14 @@ static int btf_modifier_resolve(struct btf_verifier_env *env, return -EINVAL; } + if (btf_type_is_func_proto(next_type)) { + if (env->resolve_mode == RESOLVE_PTR) + goto resolved; + + btf_verifier_log_type(env, v->t, "Invalid type_id"); + return -EINVAL; + } + /* "typedef void new_void", "const void"...etc */ if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type)) goto resolved; @@ -1185,7 +1233,8 @@ static int btf_modifier_resolve(struct btf_verifier_env *env, * pretty print). */ if (!btf_type_id_size(btf, &next_type_id, &next_type_size) && - !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) { + (!btf_type_nosize(btf_type_id_resolve(btf, &next_type_id)) || + btf_type_is_func(next_type))) { btf_verifier_log_type(env, v->t, "Invalid type_id"); return -EINVAL; } @@ -1212,7 +1261,8 @@ static int btf_ptr_resolve(struct btf_verifier_env *env, } /* "void *" */ - if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type)) + if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type) || + btf_type_is_func_proto(next_type)) goto resolved; if (!env_type_is_resolve_sink(env, next_type) && @@ -1242,7 +1292,8 @@ static int btf_ptr_resolve(struct btf_verifier_env *env, } if (!btf_type_id_size(btf, &next_type_id, &next_type_size) && - !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) { + (!btf_type_nosize(btf_type_id_resolve(btf, &next_type_id)) || + btf_type_is_func(next_type))) { btf_verifier_log_type(env, v->t, "Invalid type_id"); return -EINVAL; } @@ -1550,6 +1601,14 @@ static s32 btf_struct_check_meta(struct btf_verifier_env *env, return -EINVAL; } + if (member->name_off && + !btf_name_valid_identifier(btf, member->name_off)) { + btf_verifier_log_member(env, t, member, + "Invalid member name:%s", + btf_name_by_offset(btf, member->name_off)); + return -EINVAL; + } + /* A member cannot be in type void */ if (!member->type || !BTF_TYPE_ID_VALID(member->type)) { btf_verifier_log_member(env, t, member, @@ -1787,6 +1846,142 @@ static struct btf_kind_operations enum_ops = { .seq_show = btf_enum_seq_show, }; +static s32 btf_func_check_meta(struct btf_verifier_env *env, + const struct btf_type *t, + u32 meta_left) +{ + u32 meta_needed = btf_type_vlen(t) * sizeof(u32); + + if (meta_left < meta_needed) { + btf_verifier_log_basic(env, t, + "meta_left:%u meta_needed:%u", + meta_left, meta_needed); + return -EINVAL; + } + + btf_verifier_log_type(env, t, NULL); + + return meta_needed; +} + +static void btf_func_log(struct btf_verifier_env *env, + const struct btf_type *t) +{ + u16 nr_args = btf_type_vlen(t), i; + u32 *args = (u32 *)(t + 1); + + btf_verifier_log(env, "return=%u args=(", t->type); + if (!nr_args) { + btf_verifier_log(env, "void"); + goto done; + } + + if (nr_args == 1 && !args[0]) { + /* Only one vararg */ + btf_verifier_log(env, "vararg"); + goto done; + } + + btf_verifier_log(env, "%u", args[0]); + for (i = 1; i < nr_args - 1; i++) + btf_verifier_log(env, ", %u", args[i]); + if (nr_args > 1) { + u32 last_arg = args[nr_args - 1]; + + if (last_arg) + btf_verifier_log(env, ", %u", last_arg); + else + btf_verifier_log(env, ", vararg"); + } + +done: + btf_verifier_log(env, ")"); +} + +static int btf_func_check(struct btf_verifier_env *env, + const struct btf_type *t, + u32 type_id) +{ + u16 nr_args = btf_type_vlen(t), i; + const struct btf *btf = env->btf; + const struct btf_type *ret_type; + u32 *args = (u32 *)(t + 1); + u32 ret_type_id; + int err; + + /* Check func return type which could be "void" (t->type == 0) */ + ret_type_id = t->type; + if (ret_type_id) { + /* return type is not "void" */ + ret_type = btf_type_by_id(btf, ret_type_id); + if (btf_type_needs_resolve(ret_type) && + !env_type_is_resolved(env, ret_type_id)) { + err = btf_resolve(env, ret_type, ret_type_id); + if (err) + return err; + } + + /* Ensure the return type is a type that has a size */ + if (!btf_type_id_size(btf, &ret_type_id, NULL)) { + btf_verifier_log_type(env, t, "Invalid return type"); + return -EINVAL; + } + } + + if (!nr_args) + return 0; + + /* Last func arg type_id could be 0 if it is a vararg */ + if (!args[nr_args - 1]) + nr_args--; + + err = 0; + for (i = 0; i < nr_args; i++) { + const struct btf_type *arg_type; + u32 arg_type_id; + + arg_type_id = args[i]; + arg_type = btf_type_by_id(btf, arg_type_id); + if (!arg_type) { + btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1); + err = -EINVAL; + break; + } + + if (btf_type_needs_resolve(arg_type) && + !env_type_is_resolved(env, arg_type_id)) { + err = btf_resolve(env, arg_type, arg_type_id); + if (err) + break; + } + + if (!btf_type_id_size(btf, &arg_type_id, NULL)) { + btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1); + err = -EINVAL; + break; + } + } + + return err; +} + +static struct btf_kind_operations func_ops = { + .check_meta = btf_func_check_meta, + .resolve = btf_df_resolve, + /* + * BPF_KIND_FUNC_PROTO cannot be directly referred by + * a struct's member. + * + * It should be a funciton pointer instead. + * (i.e. struct's member -> BTF_KIND_PTR -> BTF_KIND_FUNC_PROTO) + * + * Hence, there is no btf_func_check_member(). + */ + .check_member = btf_df_check_member, + .log_details = btf_func_log, + .seq_show = btf_df_seq_show, +}; + static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = { [BTF_KIND_INT] = &int_ops, [BTF_KIND_PTR] = &ptr_ops, @@ -1799,6 +1994,8 @@ static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = { [BTF_KIND_VOLATILE] = &modifier_ops, [BTF_KIND_CONST] = &modifier_ops, [BTF_KIND_RESTRICT] = &modifier_ops, + [BTF_KIND_FUNC] = &func_ops, + [BTF_KIND_FUNC_PROTO] = &func_ops, }; static s32 btf_check_meta(struct btf_verifier_env *env, @@ -1870,30 +2067,6 @@ static int btf_check_all_metas(struct btf_verifier_env *env) return 0; } -static int btf_resolve(struct btf_verifier_env *env, - const struct btf_type *t, u32 type_id) -{ - const struct resolve_vertex *v; - int err = 0; - - env->resolve_mode = RESOLVE_TBD; - env_stack_push(env, t, type_id); - while (!err && (v = env_stack_peak(env))) { - env->log_type_id = v->type_id; - err = btf_type_ops(v->t)->resolve(env, v); - } - - env->log_type_id = type_id; - if (err == -E2BIG) - btf_verifier_log_type(env, t, - "Exceeded max resolving depth:%u", - MAX_RESOLVE_DEPTH); - else if (err == -EEXIST) - btf_verifier_log_type(env, t, "Loop detected"); - - return err; -} - static bool btf_resolve_valid(struct btf_verifier_env *env, const struct btf_type *t, u32 type_id) @@ -1927,6 +2100,39 @@ static bool btf_resolve_valid(struct btf_verifier_env *env, return false; } +static int btf_resolve(struct btf_verifier_env *env, + const struct btf_type *t, u32 type_id) +{ + u32 save_log_type_id = env->log_type_id; + const struct resolve_vertex *v; + int err = 0; + + env->resolve_mode = RESOLVE_TBD; + env_stack_push(env, t, type_id); + while (!err && (v = env_stack_peak(env))) { + env->log_type_id = v->type_id; + err = btf_type_ops(v->t)->resolve(env, v); + } + + env->log_type_id = type_id; + if (err == -E2BIG) { + btf_verifier_log_type(env, t, + "Exceeded max resolving depth:%u", + MAX_RESOLVE_DEPTH); + } else if (err == -EEXIST) { + btf_verifier_log_type(env, t, "Loop detected"); + } + + /* Final sanity check */ + if (!err && !btf_resolve_valid(env, t, type_id)) { + btf_verifier_log_type(env, t, "Invalid resolve state"); + err = -EINVAL; + } + + env->log_type_id = save_log_type_id; + return err; +} + static int btf_check_all_types(struct btf_verifier_env *env) { struct btf *btf = env->btf; @@ -1949,10 +2155,18 @@ static int btf_check_all_types(struct btf_verifier_env *env) return err; } - if (btf_type_needs_resolve(t) && - !btf_resolve_valid(env, t, type_id)) { - btf_verifier_log_type(env, t, "Invalid resolve state"); - return -EINVAL; + if (btf_type_is_func(t) || btf_type_is_func_proto(t)) { + if (btf_type_is_func(t) && + (!btf_name_offset_valid(btf, t->name_off) || + !btf_name_valid_identifier(btf, t->name_off))) { + btf_verifier_log_type(env, t, + "Invalid type_id"); + return -EINVAL; + } + + err = btf_func_check(env, t, type_id); + if (err) + return err; } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-17 7:23 ` [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO Yonghong Song @ 2018-10-17 16:13 ` Edward Cree 2018-10-17 17:25 ` Yonghong Song 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-10-17 16:13 UTC (permalink / raw) To: Yonghong Song, ast, kafai, daniel, netdev; +Cc: kernel-team On 17/10/18 08:23, Yonghong Song wrote: > This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO > support to the type section. BTF_KIND_FUNC_PROTO is used > to specify the type of a function pointer. With this, > BTF has a complete set of C types (except float). > > BTF_KIND_FUNC is used to specify the signature of a > defined subprogram. BTF_KIND_FUNC_PROTO can be referenced > by another type, e.g., a pointer type, and BTF_KIND_FUNC > type cannot be referenced by another type. Why are distinct kinds created for these? A function body is a value of function type, and since there's no way (in C) to declare a variable of function type (only pointer-to- function), any declaration of function type must necessarily be a BTF_KIND_FUNC, whereas any other reference to a function type (e.g. a declaration of type pointer to function type) must, as you state above, be a BTF_KIND_FUNC_PROTO. In fact, you can tell the difference just from name_off, since a (C-legal) BTF_KIND_FUNC_PROTO will always be anonymous (as the pointee of a pointer type), while a BTF_KIND_FUNC will have the name of the subprogram. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-17 16:13 ` Edward Cree @ 2018-10-17 17:25 ` Yonghong Song 2018-10-17 17:50 ` Martin Lau 0 siblings, 1 reply; 37+ messages in thread From: Yonghong Song @ 2018-10-17 17:25 UTC (permalink / raw) To: Edward Cree, Alexei Starovoitov, Martin Lau, daniel, netdev; +Cc: Kernel Team On 10/17/18 9:13 AM, Edward Cree wrote: > On 17/10/18 08:23, Yonghong Song wrote: >> This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO >> support to the type section. BTF_KIND_FUNC_PROTO is used >> to specify the type of a function pointer. With this, >> BTF has a complete set of C types (except float). >> >> BTF_KIND_FUNC is used to specify the signature of a >> defined subprogram. BTF_KIND_FUNC_PROTO can be referenced >> by another type, e.g., a pointer type, and BTF_KIND_FUNC >> type cannot be referenced by another type. > Why are distinct kinds created for these? A function body is > a value of function type, and since there's no way (in C) to > declare a variable of function type (only pointer-to- > function), any declaration of function type must necessarily > be a BTF_KIND_FUNC, whereas any other reference to a function > type (e.g. a declaration of type pointer to function type) > must, as you state above, be a BTF_KIND_FUNC_PROTO. > In fact, you can tell the difference just from name_off, since > a (C-legal) BTF_KIND_FUNC_PROTO will always be anonymous (as > the pointee of a pointer type), while a BTF_KIND_FUNC will > have the name of the subprogram. What you stated is true, BTF_KIND_FUNC_PROTO corresponds to dwarf subroutine tag which has no name while BTF_KIND_FUNC must have a valid name. The original design is to have both since they are corresponding to different dwarf constructs. Martin, what do you think? > > -Ed > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-17 17:25 ` Yonghong Song @ 2018-10-17 17:50 ` Martin Lau 2018-10-18 16:47 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Martin Lau @ 2018-10-17 17:50 UTC (permalink / raw) To: Yonghong Song Cc: Edward Cree, Alexei Starovoitov, daniel, netdev, Kernel Team On Wed, Oct 17, 2018 at 10:25:21AM -0700, Yonghong Song wrote: > > > On 10/17/18 9:13 AM, Edward Cree wrote: > > On 17/10/18 08:23, Yonghong Song wrote: > >> This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO > >> support to the type section. BTF_KIND_FUNC_PROTO is used > >> to specify the type of a function pointer. With this, > >> BTF has a complete set of C types (except float). > >> > >> BTF_KIND_FUNC is used to specify the signature of a > >> defined subprogram. BTF_KIND_FUNC_PROTO can be referenced > >> by another type, e.g., a pointer type, and BTF_KIND_FUNC > >> type cannot be referenced by another type. > > Why are distinct kinds created for these? A function body is > > a value of function type, and since there's no way (in C) to > > declare a variable of function type (only pointer-to- > > function), any declaration of function type must necessarily > > be a BTF_KIND_FUNC, whereas any other reference to a function > > type (e.g. a declaration of type pointer to function type) > > must, as you state above, be a BTF_KIND_FUNC_PROTO. > > In fact, you can tell the difference just from name_off, since > > a (C-legal) BTF_KIND_FUNC_PROTO will always be anonymous (as > > the pointee of a pointer type), while a BTF_KIND_FUNC will > > have the name of the subprogram. > > What you stated is true, BTF_KIND_FUNC_PROTO corresponds to > dwarf subroutine tag which has no name while BTF_KIND_FUNC > must have a valid name. The original design is to have both > since they are corresponding to different dwarf constructs. > > Martin, what do you think? I prefer to have separate kinds. We need a way to distinguish them. For example, the BTF verifier is checking it. Having two kinds is cleaner instead of resorting to other hints from 'struct btf_type'. We don't lack of bits for kind. > > > > > -Ed > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-17 17:50 ` Martin Lau @ 2018-10-18 16:47 ` Edward Cree 2018-10-18 18:12 ` Martin Lau 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-10-18 16:47 UTC (permalink / raw) To: Martin Lau, Yonghong Song; +Cc: Alexei Starovoitov, daniel, netdev, Kernel Team On 17/10/18 18:50, Martin Lau wrote: > On Wed, Oct 17, 2018 at 10:25:21AM -0700, Yonghong Song wrote: >> What you stated is true, BTF_KIND_FUNC_PROTO corresponds to >> dwarf subroutine tag which has no name while BTF_KIND_FUNC >> must have a valid name. The original design is to have both >> since they are corresponding to different dwarf constructs. >> >> Martin, what do you think? > I prefer to have separate kinds. We need a way to distinguish them. > For example, the BTF verifier is checking it. Having two kinds is > cleaner instead of resorting to other hints from 'struct btf_type'. > We don't lack of bits for kind. But my point is that (a) they can be distinguished by how they are used, and (b) the _only_ difference between them is how they are used. In this C code: int a = 5; int foo(int x) { return a; } int *b = &a; int (*bar)(int) = &foo; foo and *bar are _the same type_, just as a and *b are. It's just that C has a slightly odd way of writing int foo(int) = lambda x: a; and foo itself is implicitly sorta-const. What am I missing? -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-18 16:47 ` Edward Cree @ 2018-10-18 18:12 ` Martin Lau 2018-10-18 19:41 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Martin Lau @ 2018-10-18 18:12 UTC (permalink / raw) To: Edward Cree Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Thu, Oct 18, 2018 at 05:47:11PM +0100, Edward Cree wrote: > On 17/10/18 18:50, Martin Lau wrote: > > On Wed, Oct 17, 2018 at 10:25:21AM -0700, Yonghong Song wrote: > >> What you stated is true, BTF_KIND_FUNC_PROTO corresponds to > >> dwarf subroutine tag which has no name while BTF_KIND_FUNC > >> must have a valid name. The original design is to have both > >> since they are corresponding to different dwarf constructs. > >> > >> Martin, what do you think? > > I prefer to have separate kinds. We need a way to distinguish them. > > For example, the BTF verifier is checking it. Having two kinds is > > cleaner instead of resorting to other hints from 'struct btf_type'. > > We don't lack of bits for kind. > But my point is that (a) they can be distinguished by how they are > used, and (b) the _only_ difference between them is how they are > used. In this C code: > int a = 5; > int foo(int x) { return a; } > int *b = &a; > int (*bar)(int) = &foo; > foo and *bar are _the same type_, just as a and *b are. It's just > that C has a slightly odd way of writing > int foo(int) = lambda x: a; > and foo itself is implicitly sorta-const. > What am I missing? The BTF verification and bpf_prog_load() has to treat them differently. Are you suggesting they can be treated the same for the kernel verification purpose? or The concern is, having two kind, BTF_KIND_FUNC_PROTO and BTF_KIND_FUNC, is confusing because they have almost the same BTF metadata? >From the BTF perspective, they are different because they are allowed to contain different information. For example, only "foo" can have func_info in patch 5 (the to-be-added line_info can only belong to "foo" also). This patch 2 is also doing checks on func_proto. e.g. in btf_ptr_resolve(). The early idea is to have "foo_a", "foo_b", "foo_c"... in a separate BTF section called "func" section (on top of the current "type" and "string" section). By doing this, only one BTF_KIND_FUNC would be needed as you suggested because "foo" could be clearly distinguished from "bar" by observing they belong to different BTF sections. The down side is the "func" section will have its own id space separated from the current "type" section id space and it is less ideal for debugging purpose. Hence, "foo" is contained in the "type" section also and added FUNC_PROTO to distinguish them. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-18 18:12 ` Martin Lau @ 2018-10-18 19:41 ` Edward Cree 2018-10-18 21:19 ` Martin Lau 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-10-18 19:41 UTC (permalink / raw) To: Martin Lau; +Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On 18/10/18 19:12, Martin Lau wrote: > The BTF verification and bpf_prog_load() has to treat > them differently. > > Are you suggesting they can be treated the same for > the kernel verification purpose? > > or > > The concern is, having two kind, BTF_KIND_FUNC_PROTO and > BTF_KIND_FUNC, is confusing because they have almost the > same BTF metadata? > > From the BTF perspective, they are different because > they are allowed to contain different information. > For example, only "foo" can have func_info in patch 5 > (the to-be-added line_info can only belong to "foo" also). Perhaps I'm not properly understanding how BTF is to be used. But from my perspective, 'foo' is not a type; it's an instance of a type; i.e. it's a value of type int ()(int). Then by a kind of metonymy, we are including a BTF record that defines the type 'foo' as int()(int), in order to state that this is the type _of_ foo. Similarly, we are including a BTF record defining 'bar' as the type int (*)(int), thus stating that the type _of_ bar is int (*)(int). So everything that's special about 'foo' relates to the fact that it is a named, rather than anonymous, type, which is being used to indicate that there is an actual object 'foo' of this type. And this is *identical* to how the BTF record "name 'a', type int" declares an object a, the BTF record "name 'b', type int *" declares an object b, and the anonymous int type referenced by b's btf_type.type does not declare an object. So either types and objects are represented the same way in BTF, or not; it's inconsistent to make a distinction between them for functions but not for other types. I know C thinks functions are special, but that's a wart in C that maybe makes sense in its context but not elsewhere. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-18 19:41 ` Edward Cree @ 2018-10-18 21:19 ` Martin Lau 2018-10-19 17:04 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Martin Lau @ 2018-10-18 21:19 UTC (permalink / raw) To: Edward Cree Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Thu, Oct 18, 2018 at 08:41:52PM +0100, Edward Cree wrote: > On 18/10/18 19:12, Martin Lau wrote: > > The BTF verification and bpf_prog_load() has to treat > > them differently. > > > > Are you suggesting they can be treated the same for > > the kernel verification purpose? > > > > or > > > > The concern is, having two kind, BTF_KIND_FUNC_PROTO and > > BTF_KIND_FUNC, is confusing because they have almost the > > same BTF metadata? > > > > From the BTF perspective, they are different because > > they are allowed to contain different information. > > For example, only "foo" can have func_info in patch 5 > > (the to-be-added line_info can only belong to "foo" also). > Perhaps I'm not properly understanding how BTF is to be used. > But from my perspective, 'foo' is not a type; it's an instance > of a type; i.e. it's a value of type int ()(int). Then by a > kind of metonymy, we are including a BTF record that defines > the type 'foo' as int()(int), in order to state that this is > the type _of_ foo. Similarly, we are including a BTF record > defining 'bar' as the type int (*)(int), thus stating that > the type _of_ bar is int (*)(int). > So everything that's special about 'foo' relates to the fact > that it is a named, rather than anonymous, type, which is > being used to indicate that there is an actual object 'foo' > of this type. > And this is *identical* to how the BTF record "name 'a', type > int" declares an object a, the BTF record "name 'b', type > int *" declares an object b, and the anonymous int type > referenced by b's btf_type.type does not declare an object. > > So either types and objects are represented the same way in > BTF, or not; it's inconsistent to make a distinction between > them for functions but not for other types. > > I know C thinks functions are special, but that's a wart in C > that maybe makes sense in its context but not elsewhere. As I have mentioned earlier, it is also special to the kernel because the BTF verifier and bpf_prog_load() need to do different checks for FUNC and FUNC_PROTO to ensure they are sane. First, we need to agree that the kernel needs to verify them differently. and then we can proceed to discuss how to distinguish them. We picked the current way to avoid adding a new BTF function section and keep it strict forward to distinguish them w/o relying on other hints from 'struct btf_type'. Are you suggesting another way of doing it? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-18 21:19 ` Martin Lau @ 2018-10-19 17:04 ` Edward Cree 2018-10-19 19:36 ` Martin Lau 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-10-19 17:04 UTC (permalink / raw) To: Martin Lau; +Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On 18/10/18 22:19, Martin Lau wrote: > As I have mentioned earlier, it is also special to > the kernel because the BTF verifier and bpf_prog_load() > need to do different checks for FUNC and FUNC_PROTO to > ensure they are sane. > > First, we need to agree that the kernel needs to verify > them differently. > > and then we can proceed to discuss how to distinguish them. > We picked the current way to avoid adding a > new BTF function section and keep it > strict forward to distinguish them w/o relying > on other hints from 'struct btf_type'. > > Are you suggesting another way of doing it? But you *do* have such a new section. The patch comment talks about a 'FuncInfo Table' which appears to map (section, insn_idx) to type_id. (I think this gets added in .BTF.ext per patch 9?) So when you're looking at a FUNC type because you looked up a type_id from that table, you know it's the signature of a subprogram, and you're checking it as such. Whereas, if you were doing something with some other type and it referenced a FUNC type (e.g., in the patch comment's example, you're checking foo's first argument against the type bar) in its type_id, you know you're using it as a formal type (a FUNC_ PROTO in your parlance) and not as a subprogram. The context in which you are using a type entry tells you which kind it is. And the verifier can and should be smart enough to know what it's doing in this way. And it's entirely reasonable for the same type entry to get used for both those cases; in my example, you'd have a FUNC type for int foo(int), referenced both by the func_info entry for foo and by the PTR type for bar. And if you had another subprogram int baz(int), its func_info entry could reference the same type_id, because the (reference to the) name of the function should live in the func_info, not in the type. What you are proposing seems to be saying "if we have this particular special btf_kind, then this BTF entry doesn't just define a type, it declares a subprogram of that type". Oh, and with the name of the type as the subprogram name. Which just creates blurry confusion as to whether BTF entries define types or declare objects; IMNSHO the correct approach is for objects to be declared elsewhere and to reference BTF types by their type_id. Which is what the func_info table in patch 9 appears to do. (It also rather bothers me the way we are using special type names to associate maps with their k/v types, rather than extending the records in the maps section to include type_ids referencing them. It's the same kind of weird implicitness, and if I'd spotted it when it was going in I'd've nacked it, but I suppose it's ABI now and too late to change.) -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-19 17:04 ` Edward Cree @ 2018-10-19 19:36 ` Martin Lau 2018-10-19 20:35 ` Martin Lau 2018-10-19 21:26 ` Edward Cree 0 siblings, 2 replies; 37+ messages in thread From: Martin Lau @ 2018-10-19 19:36 UTC (permalink / raw) To: Edward Cree Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote: > On 18/10/18 22:19, Martin Lau wrote: > > As I have mentioned earlier, it is also special to > > the kernel because the BTF verifier and bpf_prog_load() > > need to do different checks for FUNC and FUNC_PROTO to > > ensure they are sane. > > > > First, we need to agree that the kernel needs to verify > > them differently. > > > > and then we can proceed to discuss how to distinguish them. > > We picked the current way to avoid adding a > > new BTF function section and keep it > > strict forward to distinguish them w/o relying > > on other hints from 'struct btf_type'. > > > > Are you suggesting another way of doing it? > But you *do* have such a new section. > The patch comment talks about a 'FuncInfo Table' which appears to Note that the new section, which contains the FuncInfo Table, is in a new ELF section ".BTF.ext" instead of the ".BTF". It is not in the ".BTF" section because it is only useful during bpf_prog_load(). I was meaning a new function section within ".BTF". > map (section, insn_idx) to type_id. (I think this gets added in > .BTF.ext per patch 9?) So when you're looking at a FUNC type > because you looked up a type_id from that table, you know it's > the signature of a subprogram, and you're checking it as such. > Whereas, if you were doing something with some other type and it > referenced a FUNC type (e.g., in the patch comment's example, > you're checking foo's first argument against the type bar) in > its type_id, you know you're using it as a formal type (a FUNC_ > PROTO in your parlance) and not as a subprogram. > The context in which you are using a type entry tells you which > kind it is. And the verifier can and should be smart enough to > know what it's doing in this way. > > And it's entirely reasonable for the same type entry to get used > for both those cases; in my example, you'd have a FUNC type for > int foo(int), referenced both by the func_info entry for foo > and by the PTR type for bar. And if you had another subprogram > int baz(int), its func_info entry could reference the same > type_id, because the (reference to the) name of the function > should live in the func_info, not in the type. IIUC, I think what you are suggesting here is to use (type_id, name) to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}", "int foo3(int) {}" where type_id here is referring to the same DW_TAG_subroutine_type, and only define that _one_ DW_TAG_subroutine_type in the BTF "type" section. That will require more manipulation/type-merging in the dwarf2btf process and it could get quite complicated. Note that CTF is also fully spelling out the return type and arg types for each DW_TAG_subprogram in a separate function section (still within the same ELF section). The only difference here is they are merged into the type section and FUNC_PROTO is added. If the concern is having both FUNC and FUNC_PROTO is confusing, we could go back to the CTF way which adds a new function section in ".BTF" and it is only for DW_TAG_subprogram. BTF_KIND_FUNC_PROTO is then no longer necessary. Some of new BTF verifier checkings may actually go away also. The down side is there will be two id spaces. > > What you are proposing seems to be saying "if we have this > particular special btf_kind, then this BTF entry doesn't just > define a type, it declares a subprogram of that type". Oh, > and with the name of the type as the subprogram name. Which > just creates blurry confusion as to whether BTF entries define > types or declare objects; IMNSHO the correct approach is for > objects to be declared elsewhere and to reference BTF types by > their type_id. > Which is what the func_info table in patch 9 appears to do. > > (It also rather bothers me the way we are using special type > names to associate maps with their k/v types, rather than > extending the records in the maps section to include type_ids > referencing them. It's the same kind of weird implicitness, > and if I'd spotted it when it was going in I'd've nacked it, > but I suppose it's ABI now and too late to change.) > > -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-19 19:36 ` Martin Lau @ 2018-10-19 20:35 ` Martin Lau 2018-10-19 21:26 ` Edward Cree 1 sibling, 0 replies; 37+ messages in thread From: Martin Lau @ 2018-10-19 20:35 UTC (permalink / raw) To: Edward Cree Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Fri, Oct 19, 2018 at 12:36:49PM -0700, Martin Lau wrote: > On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote: > > On 18/10/18 22:19, Martin Lau wrote: > > > As I have mentioned earlier, it is also special to > > > the kernel because the BTF verifier and bpf_prog_load() > > > need to do different checks for FUNC and FUNC_PROTO to > > > ensure they are sane. > > > > > > First, we need to agree that the kernel needs to verify > > > them differently. > > > > > > and then we can proceed to discuss how to distinguish them. > > > We picked the current way to avoid adding a > > > new BTF function section and keep it > > > strict forward to distinguish them w/o relying > > > on other hints from 'struct btf_type'. > > > > > > Are you suggesting another way of doing it? > > But you *do* have such a new section. > > The patch comment talks about a 'FuncInfo Table' which appears to > Note that the new section, which contains the FuncInfo Table, > is in a new ELF section ".BTF.ext" instead of the ".BTF". > It is not in the ".BTF" section because it is only useful during > bpf_prog_load(). > > I was meaning a new function section within ".BTF". > > > map (section, insn_idx) to type_id. (I think this gets added in > > .BTF.ext per patch 9?) So when you're looking at a FUNC type > > because you looked up a type_id from that table, you know it's > > the signature of a subprogram, and you're checking it as such. > > Whereas, if you were doing something with some other type and it > > referenced a FUNC type (e.g., in the patch comment's example, > > you're checking foo's first argument against the type bar) in > > its type_id, you know you're using it as a formal type (a FUNC_ > > PROTO in your parlance) and not as a subprogram. > > The context in which you are using a type entry tells you which > > kind it is. And the verifier can and should be smart enough to > > know what it's doing in this way. > > > > And it's entirely reasonable for the same type entry to get used > > for both those cases; in my example, you'd have a FUNC type for > > int foo(int), referenced both by the func_info entry for foo > > and by the PTR type for bar. And if you had another subprogram > > int baz(int), its func_info entry could reference the same > > type_id, because the (reference to the) name of the function > > should live in the func_info, not in the type. > IIUC, I think what you are suggesting here is to use (type_id, name) > to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}", > "int foo3(int) {}" where type_id here is referring to the same > DW_TAG_subroutine_type, and only define that _one_ > DW_TAG_subroutine_type in the BTF "type" section. > > That will require more manipulation/type-merging in the dwarf2btf > process and it could get quite complicated. > > Note that CTF is also fully spelling out the return type > and arg types for each DW_TAG_subprogram in a separate > function section (still within the same ELF section). > The only difference here is they are merged into the type > section and FUNC_PROTO is added. > > If the concern is having both FUNC and FUNC_PROTO is confusing, > we could go back to the CTF way which adds a new function section > in ".BTF" and it is only for DW_TAG_subprogram. > BTF_KIND_FUNC_PROTO is then no longer necessary. > Some of new BTF verifier checkings may actually go away also. > The down side is there will be two id spaces. Discussed a bit offline with folks about the two id spaces situation and it is not good for debugging purpose. If we must get rid of FUNC_PROTO, it is better to use the name_off==0 check instead of adding a new function section. We will go for this path in the next respin. > > > > > What you are proposing seems to be saying "if we have this > > particular special btf_kind, then this BTF entry doesn't just > > define a type, it declares a subprogram of that type". Oh, > > and with the name of the type as the subprogram name. Which > > just creates blurry confusion as to whether BTF entries define > > types or declare objects; IMNSHO the correct approach is for > > objects to be declared elsewhere and to reference BTF types by > > their type_id. > > Which is what the func_info table in patch 9 appears to do. > > > > (It also rather bothers me the way we are using special type > > names to associate maps with their k/v types, rather than > > extending the records in the maps section to include type_ids > > referencing them. It's the same kind of weird implicitness, > > and if I'd spotted it when it was going in I'd've nacked it, > > but I suppose it's ABI now and too late to change.) > > > > -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-19 19:36 ` Martin Lau 2018-10-19 20:35 ` Martin Lau @ 2018-10-19 21:26 ` Edward Cree 2018-10-19 23:27 ` Martin Lau 1 sibling, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-10-19 21:26 UTC (permalink / raw) To: Martin Lau; +Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On 19/10/18 20:36, Martin Lau wrote: > On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote: >> But you *do* have such a new section. >> The patch comment talks about a 'FuncInfo Table' which appears to > Note that the new section, which contains the FuncInfo Table, > is in a new ELF section ".BTF.ext" instead of the ".BTF". > It is not in the ".BTF" section because it is only useful during > bpf_prog_load(). I thought it was because it needed to be munged by the loader/linker? > IIUC, I think what you are suggesting here is to use (type_id, name) > to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}", > "int foo3(int) {}" where type_id here is referring to the same > DW_TAG_subroutine_type, and only define that _one_ > DW_TAG_subroutine_type in the BTF "type" section. Yes, something like that. > If the concern is having both FUNC and FUNC_PROTO is confusing, The concern is that you're conflating different entities (types and instances); FUNC_PROTO is just a symptom/canary of that. > we could go back to the CTF way which adds a new function section > in ".BTF" and it is only for DW_TAG_subprogram. > BTF_KIND_FUNC_PROTO is then no longer necessary. > Some of new BTF verifier checkings may actually go away also. > The down side is there will be two id spaces. Two id spaces... one for types and the other for subprograms. These are different things, so why would you _want_ them to share an id space? I don't, for instance, see any situation in which you'd want some other record to have a field that could reference either. And the 'subprogram id' doesn't have to be just for subprograms; it could be for instances generally — like I've been saying, a variable declaration is to an object type what a subprogram is to a function type, just with a few complications like "subprograms can only appear at file scope, not nested in other functions" and "variables of function type are immutable". (I'm assuming that at some point we're going to want to be able to have BTF information for e.g. variables stored on a subprogram's stack, if only for stuff like single-stepping in a debugger in userspace with some sort of mock. At that point, the variable has to have its own record — you can't just have some sort of magic type record because e.g. "struct foo bar;" has two names, one for the type and one for the variable.) > Discussed a bit offline with folks about the two id spaces > situation and it is not good for debugging purpose. Could you unpack this a bit more? -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-19 21:26 ` Edward Cree @ 2018-10-19 23:27 ` Martin Lau 2018-11-01 21:08 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Martin Lau @ 2018-10-19 23:27 UTC (permalink / raw) To: Edward Cree Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Fri, Oct 19, 2018 at 10:26:53PM +0100, Edward Cree wrote: > On 19/10/18 20:36, Martin Lau wrote: > > On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote: > >> But you *do* have such a new section. > >> The patch comment talks about a 'FuncInfo Table' which appears to > > Note that the new section, which contains the FuncInfo Table, > > is in a new ELF section ".BTF.ext" instead of the ".BTF". > > It is not in the ".BTF" section because it is only useful during > > bpf_prog_load(). > I thought it was because it needed to be munged by the loader/linker? > > > IIUC, I think what you are suggesting here is to use (type_id, name) > > to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}", > > "int foo3(int) {}" where type_id here is referring to the same > > DW_TAG_subroutine_type, and only define that _one_ > > DW_TAG_subroutine_type in the BTF "type" section. > Yes, something like that. > > > If the concern is having both FUNC and FUNC_PROTO is confusing, > The concern is that you're conflating different entities (types > and instances); FUNC_PROTO is just a symptom/canary of that. > > > we could go back to the CTF way which adds a new function section > > in ".BTF" and it is only for DW_TAG_subprogram. > > BTF_KIND_FUNC_PROTO is then no longer necessary. > > Some of new BTF verifier checkings may actually go away also. > > The down side is there will be two id spaces. > Two id spaces... one for types and the other for subprograms. > These are different things, so why would you _want_ them to share > an id space? I don't, for instance, see any situation in which > you'd want some other record to have a field that could reference > either. > And the 'subprogram id' doesn't have to be just for subprograms; > it could be for instances generally — like I've been saying, a > variable declaration is to an object type what a subprogram is to > a function type, just with a few complications like "subprograms > can only appear at file scope, not nested in other functions" and > "variables of function type are immutable". > (I'm assuming that at some point we're going to want to be able to > have BTF information for e.g. variables stored on a subprogram's > stack, if only for stuff like single-stepping in a debugger in > userspace with some sort of mock. At that point, the variable > has to have its own record — you can't just have some sort of > magic type record because e.g. "struct foo bar;" has two names, > one for the type and one for the variable.) > btf_type is not exactly a C type. btf_type is a debug-info. Each btf_type carries specific debug information. Name is part of the debug-info/btf_type. If something carries different debug-info, it is another btf_type. Like struct, the member's names of struct is part of the btf_type. A struct with the same member's types but different member's names is a different btf_type. The same go for function. The function with different function names and arg names is a different btf_type. > > Discussed a bit offline with folks about the two id spaces > > situation and it is not good for debugging purpose. > Could you unpack this a bit more? Having two id spaces for debug-info is confusing. They are all debug-info at the end. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-10-19 23:27 ` Martin Lau @ 2018-11-01 21:08 ` Edward Cree 2018-11-06 6:29 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-11-01 21:08 UTC (permalink / raw) To: Martin Lau; +Cc: Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team I've spent a bit more time thinking about / sleeping on this, and I still think there's a major disagreement here. Basically it seems like I'm saying "the design of BTF is wrong" and you're saying "but it's the design" (with the possible implication — I'm not entirely sure — of "but that's what DWARF does"). So let's back away from the details about FUNC/PROTO, and talk in more general terms about what a BTF record means. There are two classes of things we might want to put in debug-info: * There exists a type T * I have an instance X (variable, subprogram, etc.) of type T Both of these may need to reference other types, and have the same space of possible things T could be, but there the similarity ends: they are semantically different things. Indeed, the only reason for any record of the first class is to define types referenced by records of the second class. Some concrete examples of records of the second class are: 1) I have a map named "foo" with key-type T1 and value-type T2 2) I have a subprogram named "bar" with prototype T3 3) I am using stack slot fp-8 to store a value of type T4 4) I am casting ctx+8 to a pointer type T5 before dereferencing it Currently we have (1) and this patch series adds (2), both done through records that look like they are just defining a type (i.e. the first class of record) but have 'magic' semantics (in the case of (1), special names of the form ____btf_map_foo. How anyone thought that was a clean and tasteful design is beyond me.) What IMHO the design *should* be, is that we have a 'types' subsection that *only* contains records of the first class, and then other subsections to hold records of the second class that reference records of the first class by ID. So for (1) you'd have either additional fields in struct bpf_map_def (we've extended that several times before, after all), or you'd have a maps table in .BTF that links map names ("foo", not "____btf_map_foo"!) with type IDs for its key and value: struct btf_map_record { __u32 name_off; /* name of map */ __u32 key_type_id; /* index in "types" table */ __u32 value_type_id; /* ditto */ } (Note the absence of any meaningless struct type as created by BPF_ANNOTATE_KV_PAIR. That kind of source-level hack should be converted by the compiler's BTF output module into something less magic, rather than baked into the format definition.) Then for (2) you'd have a functions table in .BTF that links subprog names, start offsets, and signatures/prototypes: struct btf_func_record { __u32 name_off; /* name of function */ __u16 subprog_secn; /* section index in which func appears */ __u16 subprog_start; /* offset in section of func entry point */ __u32 type_id; /* index in "types" table of func signature */ } I believe this is a much cleaner design, which will be easier to extend in the future to add things like (3) and (4) for source-line-level debug information. I also believe that if someone had written documentation describing the original design, semantics of the various BTF records, etc., it would have been immediately obvious that the design was needlessly confusing and ad-hoc. On 20/10/18 00:27, Martin Lau wrote: > Like struct, the member's names of struct is part of the btf_type. > A struct with the same member's types but different member's names > is a different btf_type. Yes, but that's not what I'm talking about. I'm talking about structs with the same member names, but with different names of the structs. As in the following C snippet: struct foo { int i; }; int main(void) { struct foo x; struct foo y; x.i = 0 y.i = x.i; return y.i; } We have one type 'struct foo' (name "foo"), but two _instances_ of that type (names "x", "y"). We *cannot* use a single BTF record to express both "x" and its type, because its type has a name of its own ("foo") and there is only room in struct btf_type for one name. Thus we must have one record for the instance "x" and another record for the type "foo", with the former referencing the latter. > Having two id spaces for debug-info is confusing. They are > all debug-info at the end. But they have different semantics! Just because you have a term, "debug-info", that's defined to cover both, doesn't mean that they are the same thing. You might as well say that passport numbers and telephone numbers should be drawn from the same numbering space, because they're both "personal information", and never mind that one identifies a person and the other identifies a telephone. It's having the _same_ id space for entities that are almost, but not quite, entirely unlike each other that's confusing. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-01 21:08 ` Edward Cree @ 2018-11-06 6:29 ` Alexei Starovoitov 2018-11-06 18:52 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-06 6:29 UTC (permalink / raw) To: Edward Cree Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Thu, Nov 01, 2018 at 09:08:37PM +0000, Edward Cree wrote: > I've spent a bit more time thinking about / sleeping on this, and I > still think there's a major disagreement here. Basically it seems > like I'm saying "the design of BTF is wrong" and you're saying "but > it's the design" (with the possible implication — I'm not entirely > sure — of "but that's what DWARF does"). > So let's back away from the details about FUNC/PROTO, and talk in > more general terms about what a BTF record means. > There are two classes of things we might want to put in debug-info: > * There exists a type T > * I have an instance X (variable, subprogram, etc.) of type T > Both of these may need to reference other types, and have the same > space of possible things T could be, but there the similarity ends: > they are semantically different things. > Indeed, the only reason for any record of the first class is to > define types referenced by records of the second class. Some > concrete examples of records of the second class are: > 1) I have a map named "foo" with key-type T1 and value-type T2 > 2) I have a subprogram named "bar" with prototype T3 > 3) I am using stack slot fp-8 to store a value of type T4 > 4) I am casting ctx+8 to a pointer type T5 before dereferencing it > Currently we have (1) and this patch series adds (2), both done > through records that look like they are just defining a type (i.e. > the first class of record) but have 'magic' semantics (in the case > of (1), special names of the form ____btf_map_foo. How anyone > thought that was a clean and tasteful design is beyond me.) > What IMHO the design *should* be, is that we have a 'types' > subsection that *only* contains records of the first class, and > then other subsections to hold records of the second class that > reference records of the first class by ID. Such pure type approach wouldn't be practical. BTF is not pure type information. BTF is everything that verifier needs to know to make safety decisions that bpf instruction set doesn't have. For example two anonymous structs: struct { int a; int b; } var1; struct { int c; int d; } var2; from C point of view have the same type, but from BTF point of view they are different. Names of the fields are essential part of the BTF because the purpose of BTF is to provide information about bpf objects for debugging and safety reasons. Similarly int (*) (void *src, void *dst, int len); and int (*) (void *dst, void *src, int len); are the same from C and compiler point of view, but they are different in BTF, because names carry information that needs to be preserved. Same goes for function declarations. The function name and argument names are part of the 'type description'. We shouldn't be using word 'type' in pure form otherwise it will cause confusion like this thread demonstrated. Beyond prog names expressed in BTF we're adding global variables support. They will be expressed in BTF as a new KIND. Think of all global variables in a single .c file as fields of anonymous structure. They have offsets from beginning of .bss, sizes, further references into btf_type_ids and most importantly names. Another thing we're working on is spin_lock support. There we also have to rely on BTF to make sure that the certain bytes of map's value or cgroup local storage that belong to spin_lock will be masked for lookup/update calls. typedef u32 bpf_spin_lock; will be recognized by the verifier by its name. May be we will introduce new KIND_ for spin_lock too and convert name into KIND in btf writer. That is TBD. The main point that names of types, variables, functions has to be expressed in BTF as one coherent graph of information. Splitting pure types into one section, variables into another, functions into yet another is not practical, since the same modifiers (like const or volatile) need to be applied to variables and functions. At the end all sections will have the same style of encoding, hence no need to duplicate the encoding three times and instead it's cleaner to encode all of them BTF-style via different KINDs. vmlinux's BTF will include all global variables and functions, so that tracing scripts can reference to particular variable or function argument by name making kernel debuging easier and less error prone. Consider that right now typical bcc's trace.py script looks like: trace.py -I linux/skbuff.h -I net/sock.h \ 'skb_recv_datagram(struct sock *sk, unsigned int flags, int noblock, int *err) \ "sk_recv_q:%llx next:%llx prev:%llx" \ &sk->sk_receive_queue,sk->sk_receive_queue.next,sk->sk_receive_queue.prev' where func declaration is copy pasted from the C source and there are no guarantees that argument names and their order match what vmlinux actually has. With fully named BTF of vmlinux the tracing scripts will become more robust. Note that BTF_KIND_FUNC is pretty much the same as BTF_KIND_STRUCT with an addition of return type. Kernel specific things like per-cpu attribute of the variable will be BTF KIND too. This is information that tooling and kernel needs and current BTF graph design is perfectly suited to carry such info. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-06 6:29 ` Alexei Starovoitov @ 2018-11-06 18:52 ` Edward Cree 2018-11-06 21:56 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-11-06 18:52 UTC (permalink / raw) To: Alexei Starovoitov Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On 06/11/18 06:29, Alexei Starovoitov wrote: > BTF is not pure type information. BTF is everything that verifier needs > to know to make safety decisions that bpf instruction set doesn't have. Yes, I'm not disputing that and never have. I'm just saying that it will be much cleaner and better if it's internally organised differently. > Splitting pure types into one section, variables into another, > functions into yet another is not practical, since the same > modifiers (like const or volatile) need to be applied to > variables and functions. At the end all sections will have > the same style of encoding, hence no need to duplicate > the encoding three times and instead it's cleaner to encode > all of them BTF-style via different KINDs. This shows that you've misunderstood what I'm proposing, probably I explained it poorly so I'll try again. I'm not suggesting that the 'functions' and 'variables' sections would have _type_ records in them, only that they would reference records in the 'types' section. So if for instance we have int foo(int x) { int quux; /* ... */ } int bar(int y) { /* ... */ } in the source, then in the 'types' section we would have 1 INT 32bits encoding=signed offset=0 2 FUNC args=(name="x" type=1,), ret=1 3 FUNC args=(name="y" type=1,), ret=1 while in the 'functions' section we would have 1 "foo" type=2 start_insn_idx=23 insn_count=19 (... maybe other info too...) 2 "bar" type=3 start_insn_idx=42 insn_count=5 and in the 'variables' section we might have 1 "quux" type=1 where=stack func=1 offset=-8 Thus the graph of types lives entirely in the 'types' section, but things-that-are-not-types don't. I'm not making a distinction between "pure types" and (somehow) impure types; I'm making a distinction between types (with all their impurities) and *instances* of those types. Note that these 'sections' may all really be regions of the '.BTF' ELF section, if that makes the implementation easier. Also, the 'functions' and 'variables' sections _won't_ have the same style of encoding as the 'types', because they're storing entirely different data and in fact don't need variable record sizes. And note that in this case any const or volatile qualifiers happen _in the 'types' section_, because they're just another way of deriving a type, and the records in other sections that might want them will just point at a BTF_KIND_CONST or BTF_KIND_VOLATILE record in the 'types' section. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-06 18:52 ` Edward Cree @ 2018-11-06 21:56 ` Alexei Starovoitov 2018-11-06 22:58 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-06 21:56 UTC (permalink / raw) To: Edward Cree Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Tue, Nov 06, 2018 at 06:52:11PM +0000, Edward Cree wrote: > On 06/11/18 06:29, Alexei Starovoitov wrote: > > BTF is not pure type information. BTF is everything that verifier needs > > to know to make safety decisions that bpf instruction set doesn't have. > Yes, I'm not disputing that and never have. > I'm just saying that it will be much cleaner and better if it's > internally organised differently. > > > Splitting pure types into one section, variables into another, > > functions into yet another is not practical, since the same > > modifiers (like const or volatile) need to be applied to > > variables and functions. At the end all sections will have > > the same style of encoding, hence no need to duplicate > > the encoding three times and instead it's cleaner to encode > > all of them BTF-style via different KINDs. > This shows that you've misunderstood what I'm proposing, probably > I explained it poorly so I'll try again. > I'm not suggesting that the 'functions' and 'variables' sections > would have _type_ records in them, only that they would reference > records in the 'types' section. So if for instance we have > int foo(int x) { int quux; /* ... */ } > int bar(int y) { /* ... */ } > in the source, then in the 'types' section we would have > 1 INT 32bits encoding=signed offset=0 > 2 FUNC args=(name="x" type=1,), ret=1 > 3 FUNC args=(name="y" type=1,), ret=1 > while in the 'functions' section we would have > 1 "foo" type=2 start_insn_idx=23 insn_count=19 (... maybe other info too...) > 2 "bar" type=3 start_insn_idx=42 insn_count=5 that looks very weird to me. Why split func name from argument names? The 'function name' as seen by the BTF may not be the symbol name as seen in elf file. Like C++ mangles names for elf. If/when we have C++ front-end for BPF the BTF func name will be 'class::method' string. While elf symbol name will still be mangled string in the elf file. btw libbpf processes that elf symbol name for bpf prog already and passes it to the kernel as bpf_attr.prog_name. BTF's func name should be the one seen in the source. Whatever that source code might be. There are C, bpftrace, p4 and python frontends. These languages should be free to put into BTF KIND_FUNC name that makes sense from the language point of view. Ideally it's C-like name, so when bpftool prints that BTF it would be meaningful. btw we've been thinking to teach libbpf to generate BTF KIND_FUNC on the fly based on elf symbol name (when real BTF is missing in the elf), but decided not to go that route for now. > and in the 'variables' section we might have > 1 "quux" type=1 where=stack func=1 offset=-8 that doesn't work. stack slots can be reused by compiler. variable can be in the register too. right now we're not planning to have an equivalent of such dwarf debug info in BTF, since -O2 is mandatory and with optimizations variable tracking (gcc term) is not effective. Instead we will annotate every load/store with btf type id. Meaning no plans to tackle local variables. The global variables for given .c file will look like single KIND_STRUCT (which is variable length) and when libbpf will learn to 'link' multiple .o the BTF deduplication work (which we're doing for vmlinux) will apply as-is to combine multiple .o together. > > Thus the graph of types lives entirely in the 'types' section, but > things-that-are-not-types don't. I'm not making a distinction > between "pure types" and (somehow) impure types; I'm making a > distinction between types (with all their impurities) and > *instances* of those types. > Note that these 'sections' may all really be regions of the '.BTF' > ELF section, if that makes the implementation easier. Also, the > 'functions' and 'variables' sections _won't_ have the same style > of encoding as the 'types', because they're storing entirely > different data and in fact don't need variable record sizes. yes we do see these things differently. To us function name is the debug info that fits well into BTF description. Whereas you see the function name part of function declaration as something 'entirely different'. I don't quite get that point. In C elf symbol name and in-source func name are the same, which is probably causing this terminology confusion. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-06 21:56 ` Alexei Starovoitov @ 2018-11-06 22:58 ` Edward Cree 2018-11-07 0:59 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-11-06 22:58 UTC (permalink / raw) To: Alexei Starovoitov Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On 06/11/18 21:56, Alexei Starovoitov wrote: > that looks very weird to me. Why split func name from argument names? > The 'function name' as seen by the BTF may not be the symbol name > as seen in elf file. The symbol name will be in the symbol table, which is not the same thing as the functions table in BTF that I'm proposing. (They do look a little similar as I included an insn_idx for functions that partially duplicates the offset given in the symbol table. But that's necessary precisely for the reason you mention, that the function name != the symbol name in general.) "Splitting" func name from argument names is partly to potentially save space — if we'd had "int bar(int x)" instead then 'bar' could share its type record with 'foo'. And partly just because the name of the function itself is no more part of its type than the name of an integer variable is part of the integer's type. (Whereas names of parameters are like names of struct members: while they are not part of the 'pure type' from a language perspective, they are part of the type from the perspective of debugging, which is why they belong in the BTF type record.) > There are C, bpftrace, p4 and python frontends. These languages > should be free to put into BTF KIND_FUNC name that makes sense > from the language point of view. I'm paying attention to BTF because I'm adding support for it into my ebpf_asm. Don't you think I *know* that frontends for BPF are more than just C? >> and in the 'variables' section we might have >> 1 "quux" type=1 where=stack func=1 offset=-8 > that doesn't work. stack slots can be reused by compiler. And who says that there can't be multiple records pointing to the same stack slot with different types & names? > Instead we will annotate every load/store with btf type id. That's certainly more useful; but I think most useful of all is to have *both* (though the stack slot types should be optional). > The global variables for given .c file will look like single KIND_STRUCT That's exactly the kind of superficially-clever but nasty hack that results from the continued insistence on conflating types and instances (objects). In the long run it will make maintenance harder, and frustrate new features owing to the need to find new hacks to shoehorn them into the same model. Instead there should be entries for the globals in something like the variables table I mentioned, 2 "fred" type=1 where=global func=0 offset=8 in which 'func' is unused and 'offset' gives offset in .bss. 'where' might also include indication of whether it's static. Then for linkage you can extend this with index of which file it came from. But maybe discussing global variables is a bit premature as eBPF doesn't have any such thing yet. > yes we do see these things differently. > To us function name is the debug info that fits well into BTF description. > Whereas you see the function name part of function declaration > as something 'entirely different'. I'm not saying that the function name is 'entirely different' to the rest of the type. (Though I do think it doesn't belong in the type, that's a weaker and contingent point.) I'm saying that the *function* is entirely different to its *type*. It's a category error to conflate them: f: x ↦ x + 1 is a function. int → int is a type, and specifically the type of the object named "f". (And the nature of mathematical notation for functions happens to put the name 'x' in the former, whereas we are putting the parameter name in the latter, but that's irrelevant.) Similarly, "1" is an integer, but "integer" is a type, and is not itself an integer, while "1" is not a type. They are at different meta-levels. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-06 22:58 ` Edward Cree @ 2018-11-07 0:59 ` Alexei Starovoitov 2018-11-07 19:29 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-07 0:59 UTC (permalink / raw) To: Edward Cree Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Tue, Nov 06, 2018 at 10:58:42PM +0000, Edward Cree wrote: > share its type record with 'foo'. And partly just because the > name of the function itself is no more part of its type than the > name of an integer variable is part of the integer's type. correct. function name is not part of its type. function name is part of BTF that provide debug info about the function. Function name and function argument names are part of the same debug info. Splitting them makes no sense. > (Whereas names of parameters are like names of struct members: > while they are not part of the 'pure type' from a language > perspective, they are part of the type from the perspective of > debugging, which is why they belong in the BTF type record.) struct name and struct field names live in the same BTF record. Similarly function name and function argument names should be in the same BTF record, so we can reuse most of the BTF validation and BTF parsing logic by doing so. The minor difference between KIND_STRUCT and KIND_FUNC is an addition of return type_id. Everything else is common. imo that speaks for itself that it's a correct path forward. > > There are C, bpftrace, p4 and python frontends. These languages > > should be free to put into BTF KIND_FUNC name that makes sense > > from the language point of view. > I'm paying attention to BTF because I'm adding support for it into > my ebpf_asm. Don't you think I *know* that frontends for BPF are > more than just C? assembler is not a high level language. I believe it's a proper trade-off to make C easier to use in expense of some ugliness in your ebpf_asm. > > The global variables for given .c file will look like single KIND_STRUCT > That's exactly the kind of superficially-clever but nasty hack > that results from the continued insistence on conflating types > and instances (objects). In the long run it will make > maintenance harder, and frustrate new features owing to the need > to find new hacks to shoehorn them into the same model. Let's keep 'nasty hack' claims out of this discussion. I find the current BTF design and KIND_FUNC addition to be elegant and appropriate. > Instead there should be entries for the globals in something like > the variables table I mentioned, > 2 "fred" type=1 where=global func=0 offset=8 > in which 'func' is unused and 'offset' gives offset in .bss. > 'where' might also include indication of whether it's static. 'static' like boolean flag? That won't help introspection. To properly describe 'static' functions more information is necessary. I don't like to invent new formats. BTF is extensible description of any debug info. I prefer to keep all debug info in one place and in one common format. > I'm saying that the *function* is entirely different to its > *type*. It's a category error to conflate them: > f: x -> x + 1 > is a function. BTF does not describe function. BTF describes debug info about function. BPF program is the function. BTF is not *type* only format. It's debug info format. Trying to make BTF into type only is not going to work. It's already more than type only as I showed earlier. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-07 0:59 ` Alexei Starovoitov @ 2018-11-07 19:29 ` Edward Cree 2018-11-07 21:49 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-11-07 19:29 UTC (permalink / raw) To: Alexei Starovoitov Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On 07/11/18 00:59, Alexei Starovoitov wrote: > Function name and function argument names are part of the same debug info. > Splitting them makes no sense. ... except where combining them involves creating pain elsewhere. Sure the function name *could* go in the type record, but there still needs to be a separate function record in a functions table (because types are not instances), and that being the case the latter _may as well_ be where the name lives so that multiple functions (and pointers to them) can all share the same type record when the param names etc. all match. > struct name and struct field names live in the same BTF record. No, the struct _type_ name lives in the same record as the field names, but an _instance_ name doesn't. I.e. in struct foo {int x;} bar; the BTF type record holds the names 'foo' and 'x', but not 'bar' because that's not the name of the _type_. Indeed there isn't room in the record for both 'foo' and 'bar' because there's only one name_off field for the type. And I argue that the name of a function is more like 'bar' than 'foo' here, not least from the point of view of which C namespace they occupy. > Similarly function name and function argument names should be > in the same BTF record, so we can reuse most of the BTF validation > and BTF parsing logic by doing so. I think it's incredibly short-sighted to focus on 'what can most easily be done with the existing implementation' when designing a file format which is intended to have 'long legs'. > assembler is not a high level language. I never said it was. > I believe it's a proper trade-off to make C easier to use > in expense of some ugliness in your ebpf_asm. Please respond to the arguments I make, rather than unrelated arguments that you might imagine me making. Asm is merely a cause of my present interest in BTF, it is not the lens through which I see the whole thing. > Let's keep 'nasty hack' claims out of this discussion. > I find the current BTF design and KIND_FUNC addition to be elegant > and appropriate. Whereas I don't, and I don't feel like my core criticisms have been addressed _at all_. The only answer I get to "BTF should store type and instance information in separate records" is "it's a debuginfo", no indication of why that's a meaningful noun let alone why it implies they should be conflated in the format. And please explain what's "elegant" about how map types are currently handled. > BTF is not *type* only format. It's debug info format. > Trying to make BTF into type only is not going to work. > It's already more than type only as I showed earlier. Again, as I have *repeatedly* said, I am not trying to remove non-type information from BTF. I am just trying to organise BTF to consist of separate _parts_ for types and instances, rather than forcing both into the same Procrustean bed. (I don't feel like we're making progress in understanding one another here; maybe we should have a telephone discussion? Sadly I'm not going to Plumbers, else that would be the perfect place to thrash this out.) -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-07 19:29 ` Edward Cree @ 2018-11-07 21:49 ` Alexei Starovoitov 2018-11-08 17:58 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-07 21:49 UTC (permalink / raw) To: Edward Cree Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Wed, Nov 07, 2018 at 07:29:31PM +0000, Edward Cree wrote: > Whereas I don't, and I don't feel like my core criticisms have > been addressed _at all_. The only answer I get to "BTF should > store type and instance information in separate records" is > "it's a debuginfo", ... > I am just trying to organise > BTF to consist of separate _parts_ for types and instances, > rather than forcing both into the same Procrustean bed. BTF does not have and should not have instances. BTF is debug info only. This is not negotiable. Loading BTF into the kernel does not create an instance of anything. The kernel verifies BTF and BTF blob sits there waiting to be pointed at by the actual instance. Today these instances are maps and programs. A single program can have multiple instances of the functions which debug info is described in BTF. Multiple prograrms (with multiple functions) can point to the same BTF blob description. Consider that in single C file there will be multliple functions, many maps, global variables (in the future) they will be created by individual prog_load/map_create syscalls. Before that the _single_ blob of BTF data for the whole C file will be loaded, since functions, maps share most of the type and name information. In elf files the instances of functions have names. These names go into symbol table of the elf. Dynamic linker is using these symbols to connect different instances. In BPF world we use IDs to connect instances. There is btd if, map id, prog id. You can argue that bpf_prog_load has prog_name attribute. So the instance of the program sort of have name already, but this name is for debug only. It doesn't have to be unique and the kernel is not relying on the name to connect instances. Example: 1: int foo(struct __sk_buff *skb, char arg2) 2: { 3: return skb->len; 4: } Line 1 goes into BTF to describe the function (with names and types). Line 1 also goes into line_info .BTF.ext elf section as a string. Line 3 and 4 go into line_info as well as strings. llvm compiles it into something: .text .globl foo .type foo, @function foo: r0 = *(u32 *)(r1 + 0) exit .section .BTF .byte X .byte Y .section .BTF.ext .byte X .byte Y That 'foo' name goes into symbol table and together with the body form the future instance of the function. libbpf takes 'foo' from symbol table of elf and passes it as bpf_attr.prog_name to the kernel along with bpf instructions to create an instance. The name 'foo' in symbol table doesn't have to match with BTF 'foo' name and '.*foo.*' substring in line_info. If you decide to use the sequence: """ .globl foo .type foo, @function foo: "" in your ebpf_asm to parse and store into BTF as a function name I would strongly argue against, since it would be incorrectly conflating the instance of the function with debug purpose of BTF. Right now we do not have a concrete proposal for assembler text of BTF. When LLVM emits BTF into .s it emits them as raw bytes. So I'm looking forward to your ideas how to describe BTF in .s Note such .s must have freedom to describe 'int bar(struct __sk_buff *a1, char a2)' as debug info while having '.globl foo; foo:' as symbol name. Your other 'criticism' was about libbpf's bpf_map_find_btf_info() and ____btf_map_* hack. Yes. It is a hack and I'm open to change it if there are better suggestions. It's a convention between libbpf and program writers that produce elf. It's not a kernel abi. Nothing to do with BTF and this instance vs debug info discussion. > (I don't feel like we're making progress in understanding one > another here; maybe we should have a telephone discussion? > Sadly I'm not going to Plumbers, else that would be the > perfect place to thrash this out.) Happy to jump on the call to explain it again. 10:30am pacific time works for me tomorrow. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-07 21:49 ` Alexei Starovoitov @ 2018-11-08 17:58 ` Edward Cree 2018-11-08 18:21 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-11-08 17:58 UTC (permalink / raw) To: Alexei Starovoitov Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On 07/11/18 21:49, Alexei Starovoitov wrote: > On Wed, Nov 07, 2018 at 07:29:31PM +0000, Edward Cree wrote: >> Whereas I don't, and I don't feel like my core criticisms have >> been addressed _at all_. The only answer I get to "BTF should >> store type and instance information in separate records" is >> "it's a debuginfo", > ... >> I am just trying to organise >> BTF to consist of separate _parts_ for types and instances, >> rather than forcing both into the same Procrustean bed. > BTF does not have and should not have instances. > BTF is debug info only. > This is not negotiable. I'm not saying the instances go in BTF, I'm saying that debug info *about* instances goes in BTF (it already does, as you keep saying BTF is "not just pure types"), and that that ought to be distinguished within the format from debug info about types. > So I'm looking forward to your ideas how to describe BTF in .s > Note such .s must have freedom to describe 'int bar(struct __sk_buff *a1, char a2)' > as debug info while having '.globl foo; foo:' as symbol name. I've pushed out a branch with what I have; see https://github.com/solarflarecom/ebpf_asm/tree/btfdoc (with some examples in dropper.s and documentation in the README). In particular note that right now the BTF section is entirely decoupled from the .text, so indeed there is nothing right now tying function names to symbol names. I do not yet have anything generating FuncInfo (or LineInfo) tables, but when I do that will remain decoupled. > Your other 'criticism' was about libbpf's bpf_map_find_btf_info() > and ____btf_map_* hack. Yes. It is a hack and I'm open to change it > if there are better suggestions. It's a convention between > libbpf and program writers that produce elf. It's not a kernel abi. > Nothing to do with BTF and this instance vs debug info discussion. It's everything to do with it: it's defining a type with a magic name (____btf_map_foo) when what we really want to do is declare an instance (the map 'foo'). And it may not be a kernel ABI, but it's a part of the file format you're defining (whether that's just a 'convention' or something more), and if you want the BTF ecosystem to be more than just an llvm monoculture then the format needs to be properly specified so that others can work with it. > Happy to jump on the call to explain it again. > 10:30am pacific time works for me tomorrow. That works for me (that's in ~30 minutes from now if I've converted correctly.) Please email me offlist with the phone number to call. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-08 17:58 ` Edward Cree @ 2018-11-08 18:21 ` Alexei Starovoitov 2018-11-08 19:42 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-08 18:21 UTC (permalink / raw) To: Edward Cree Cc: Martin Lau, Yonghong Song, Alexei Starovoitov, daniel, netdev, Kernel Team On Thu, Nov 08, 2018 at 05:58:56PM +0000, Edward Cree wrote: > > > Happy to jump on the call to explain it again. > > 10:30am pacific time works for me tomorrow. > That works for me (that's in ~30 minutes from now if I've converted > correctly.) Please email me offlist with the phone number to call. no offlist. public link for anyone to join: https://bluejeans.com/867080076/ I have hard cutoff at 11am though. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-08 18:21 ` Alexei Starovoitov @ 2018-11-08 19:42 ` Alexei Starovoitov 2018-11-08 22:56 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-08 19:42 UTC (permalink / raw) To: Edward Cree Cc: Martin KaFai Lau, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Network Development, Kernel Team On Thu, Nov 8, 2018 at 10:21 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Nov 08, 2018 at 05:58:56PM +0000, Edward Cree wrote: > > > > > Happy to jump on the call to explain it again. > > > 10:30am pacific time works for me tomorrow. > > That works for me (that's in ~30 minutes from now if I've converted > > correctly.) Please email me offlist with the phone number to call. > > no offlist. public link for anyone to join: > https://bluejeans.com/867080076/ > > I have hard cutoff at 11am though. same link let's continue at 1pm PST. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-08 19:42 ` Alexei Starovoitov @ 2018-11-08 22:56 ` Edward Cree 2018-11-09 1:26 ` Yonghong Song 2018-11-09 4:35 ` Alexei Starovoitov 0 siblings, 2 replies; 37+ messages in thread From: Edward Cree @ 2018-11-08 22:56 UTC (permalink / raw) To: Alexei Starovoitov Cc: Martin KaFai Lau, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Network Development, Kernel Team On 08/11/18 19:42, Alexei Starovoitov wrote: > same link let's continue at 1pm PST. So, one thing we didn't really get onto was maps, and you mentioned that it wasn't really clear what I was proposing there. What I have in mind comes in two parts: 1) map type. A new BTF_KIND_MAP with metadata 'key_type', 'value_type' (both are type_ids referencing other BTF type records), describing the type "map from key_type to value_type". 2) record in the 'instances' table. This would have a name_off (the name of the map), a type_id (pointing at a BTF_KIND_MAP in the 'types' table), and potentially also some indication of what symbol (from section 'maps') refers to this map. This is pretty much the exact same metadata that a function in the 'instances' table has, the only differences being (a) function's type_id points at a BTF_KIND_FUNC record (b) function's symbol indication refers from .text section (c) in future functions may be nested inside other functions, whereas AIUI a map can't live inside a function. (But a variable, which is the other thing that would want to go in an 'instances' table, can.) So the 'instances' table record structure looks like struct btf_instance { __u32 type_id; /* Type of object declared. An index into type section */ __u32 name_off; /* Name of object. An offset into string section */ __u32 parent; /* Containing object if any (else 0). An index into instance section */ }; and we extend the BTF header: struct btf_header { __u16 magic; __u8 version; __u8 flags; __u32 hdr_len; /* All offsets are in bytes relative to the end of this header */ __u32 type_off; /* offset of type section */ __u32 type_len; /* length of type section */ __u32 str_off; /* offset of string section */ __u32 str_len; /* length of string section */ __u32 inst_off; /* offset of instance section */ __u32 inst_len; /* length of instance section */ }; Then in the .BTF.ext section, we have both struct bpf_func_info { __u32 prog_symbol; /* Index of symbol giving address of subprog */ __u32 inst_id; /* Index into instance section */ } struct bpf_map_info { { __u32 map_symbol; /* Index of symbol creating this map */ __u32 inst_id; /* Index into instance section */ } (either living in different subsections, or in a single table with the addition of a kind field, or in a single table relying on the ultimately referenced type to distinguish funcs from maps). Note that the name (in btf_instance) of a map or function need not match the name of the corresponding symbol; we use the .BTF.ext section to tie together btf_instance IDs and symbol IDs. Then in the case of functions (subprogs), the prog_symbol can be looked up in the ELF symbol table to find the address (== insn_offset) of the subprog, as well as the section containing it (since that might not be .text). Similarly in the case of maps the BTF info about the map is connected with the info in the maps section. Now when the loader has munged this, what it passes to the kernel might not have map_symbol, but instead map_fd. Instead of prog_symbol it will have whatever identifies the subprog in the blob of stuff it feeds to the kernel (so probably insn_offset). All this would of course require a bit more compiler support than the current BPF_ANNOTATE_KV_PAIR, since that just causes the existing BTF machinery to declare a specially constructed struct type. At the C level you could still have BPF_ANNOTATE_KV_PAIR and the '____bpf_map_foo' name, but then the compiler would recognise that and convert it into an instance record by looking up the name 'foo' in its "maps" section. That way the special ____bpf_map_* handling (which ties map names to symbol names, also) would be entirely compiler-internal and not 'leak out' into the definition of the format. Frontends for other languages which do possess a native map type (e.g. Python dict) might have other ways of indicating the key/value type of a map at source level (e.g. PEP 484) and could directly generate the appropriate BTF_KIND_MAP and bpf_map_info records rather than (as they would with the current design) having to encode the information as a struct ____bpf_map_foo type-definition. While I realise the desire to concentrate on one topic at once, I think this question of maps should be discussed in tomorrow's call, since it is when we start having other kinds of instances besides functions that the advantages of my design become apparent, unifying the process of 'declaration' of functions, maps, and (eventually) variables while separating them all from the process of 'definition' of the types of all three. Thank you for your continued patience with me. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-08 22:56 ` Edward Cree @ 2018-11-09 1:26 ` Yonghong Song 2018-11-09 4:35 ` Alexei Starovoitov 1 sibling, 0 replies; 37+ messages in thread From: Yonghong Song @ 2018-11-09 1:26 UTC (permalink / raw) To: Edward Cree, Alexei Starovoitov Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann, Network Development, Kernel Team On 11/8/18 2:56 PM, Edward Cree wrote: > On 08/11/18 19:42, Alexei Starovoitov wrote: >> same link let's continue at 1pm PST. > So, one thing we didn't really get onto was maps, and you mentioned that it > wasn't really clear what I was proposing there. > What I have in mind comes in two parts: > 1) map type. A new BTF_KIND_MAP with metadata 'key_type', 'value_type' > (both are type_ids referencing other BTF type records), describing the > type "map from key_type to value_type". > 2) record in the 'instances' table. This would have a name_off (the > name of the map), a type_id (pointing at a BTF_KIND_MAP in the 'types' > table), and potentially also some indication of what symbol (from > section 'maps') refers to this map. This is pretty much the exact > same metadata that a function in the 'instances' table has, the only > differences being > (a) function's type_id points at a BTF_KIND_FUNC record > (b) function's symbol indication refers from .text section > (c) in future functions may be nested inside other functions, whereas > AIUI a map can't live inside a function. (But a variable, which is > the other thing that would want to go in an 'instances' table, can.) > So the 'instances' table record structure looks like > > struct btf_instance { > __u32 type_id; /* Type of object declared. An index into type section */ > __u32 name_off; /* Name of object. An offset into string section */ > __u32 parent; /* Containing object if any (else 0). An index into instance section */ > }; > > and we extend the BTF header: > > struct btf_header { > __u16 magic; > __u8 version; > __u8 flags; > __u32 hdr_len; > > /* All offsets are in bytes relative to the end of this header */ > __u32 type_off; /* offset of type section */ > __u32 type_len; /* length of type section */ > __u32 str_off; /* offset of string section */ > __u32 str_len; /* length of string section */ > __u32 inst_off; /* offset of instance section */ > __u32 inst_len; /* length of instance section */ > }; > > Then in the .BTF.ext section, we have both > > struct bpf_func_info { > __u32 prog_symbol; /* Index of symbol giving address of subprog */ > __u32 inst_id; /* Index into instance section */ > } > > struct bpf_map_info { > { > __u32 map_symbol; /* Index of symbol creating this map */ > __u32 inst_id; /* Index into instance section */ > } > > (either living in different subsections, or in a single table with > the addition of a kind field, or in a single table relying on the > ultimately referenced type to distinguish funcs from maps). > > Note that the name (in btf_instance) of a map or function need not > match the name of the corresponding symbol; we use the .BTF.ext > section to tie together btf_instance IDs and symbol IDs. Then in > the case of functions (subprogs), the prog_symbol can be looked > up in the ELF symbol table to find the address (== insn_offset) > of the subprog, as well as the section containing it (since that > might not be .text). Similarly in the case of maps the BTF info > about the map is connected with the info in the maps section. > > Now when the loader has munged this, what it passes to the kernel > might not have map_symbol, but instead map_fd. Instead of > prog_symbol it will have whatever identifies the subprog in the > blob of stuff it feeds to the kernel (so probably insn_offset). > > All this would of course require a bit more compiler support than > the current BPF_ANNOTATE_KV_PAIR, since that just causes the > existing BTF machinery to declare a specially constructed struct > type. At the C level you could still have BPF_ANNOTATE_KV_PAIR > and the '____bpf_map_foo' name, but then the compiler would > recognise that and convert it into an instance record by looking > up the name 'foo' in its "maps" section. That way the special > ____bpf_map_* handling (which ties map names to symbol names, Compiler in general does not do transformation based on variable or struct type names by default, so this probably should stay in the loader. > also) would be entirely compiler-internal and not 'leak out' into > the definition of the format. Frontends for other languages > which do possess a native map type (e.g. Python dict) might have > other ways of indicating the key/value type of a map at source > level (e.g. PEP 484) and could directly generate the appropriate > BTF_KIND_MAP and bpf_map_info records rather than (as they would > with the current design) having to encode the information as a > struct ____bpf_map_foo type-definition. You mean a python application can generate bpf byte codes and BTFs (include map types)? That will be different from the C/LLVM use case. The python app. probably will be the loader as well. One option is to pass BPF specific flag like "-map-type-prefix="___bpf_map_" and LLVM will generate BTF_KIND_MAP type for any structure with name "___bpf_map_<...>". But if this is the case, user can just search the type table for struct name "___bpf_map_<...>" and llvm does not need to do anything. Note that once user passes "-map-type-prefix="___bpf_map_" to llvm, the definition of the format is already leaked. So I feel that this probably belongs to the loader. > > > While I realise the desire to concentrate on one topic at once, I > think this question of maps should be discussed in tomorrow's > call, since it is when we start having other kinds of instances > besides functions that the advantages of my design become > apparent, unifying the process of 'declaration' of functions, > maps, and (eventually) variables while separating them all from > the process of 'definition' of the types of all three. > > Thank you for your continued patience with me. > -Ed > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-08 22:56 ` Edward Cree 2018-11-09 1:26 ` Yonghong Song @ 2018-11-09 4:35 ` Alexei Starovoitov 2018-11-09 20:00 ` Edward Cree 1 sibling, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-09 4:35 UTC (permalink / raw) To: Edward Cree Cc: Martin KaFai Lau, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Network Development, Kernel Team On Thu, Nov 08, 2018 at 10:56:55PM +0000, Edward Cree wrote: > On 08/11/18 19:42, Alexei Starovoitov wrote: > > same link let's continue at 1pm PST. > So, one thing we didn't really get onto was maps, and you mentioned that it > wasn't really clear what I was proposing there. let's discuss ground rules first. 1. the patches should be upstreamable in both kernel _and_ llvm For example you propose 'prog_symbol' down below. I'm sure you can do it in your assembler. We can also do it in pahole (because it operates on dwarf inside elf), but there is no way for us to do it in llvm. Because symbol table gen comes last. dwarf, btf sections have been completed before that. llvm 'fixup' logic also done before symbol table. So to add something like 'prog_symbol' into .BTF.ext would mean that would need an extra pass after the last one that messes with the stuff already finalized. That will get nacked. Another example: in our first llvm->btf patchset we were using llvm's dwarf to generate btf. That got nacked and we had to do 2k+ lines complete rewrite to generate btf from llvm ir. That forced us to generate btf slightly differently than from dwarf. The difference is not fundamental (like bitfields). But it drives the point that elf format is _secondary_ to #1 rule above. Another example: you're proposing to teach bpf backend to recognize ____bpf_map* name. That is not something we can upstream in llvm either. Similarly on the kernel api side we decided to craft an api in a way that what is passed to the kernel is returned back in the same way. Sort of like if kernel understands 'struct bpf_func_info' from patch 5 it should speak back in the same language. I think it's important to agree on that principle to continue discussion. Another key point is obvious, but worth repeating. kernel abi is cast in stone. elf format is not. which means that what was defined in include/uapi/linux/btf.h is fixed. That is the format of .BTF section that llvm/pahole emits. Whereas .BTF.ext section is _not_ defined in kernel uapi. It defines the elf format and we can change it the future. The format of .BTF.ext is the protocol between libbpf and llvm. We define .BTF.ext in libbpf with libbpf license and coding style and independently define it in llvm with its license and its coding style. We need to discuss .BTF.ext and make sure it's extensible, so we can upgrade libbpf and llvm independently of each other, but it doesn't have the same requirements as kernel abi. Like in the kernel abi the extensibility of any structure means that uknown fields should be zero. Ex: sys_bpf() accepts 'bpf_attr' and size. If user's size is larger than kernel size. All uknown fields must be zero. In case of libbpf <-> llvm that is not the case. libbpf _has_ to deal with non-zero unknown fields in some way. Like it can print warning, but it has to accept the extended format. Otherwise upgrading llvm will not be possible without upgrading libbpf. Similarly when both llvm is new and libbpf is new, but kernel is old, libbpf has to pass to the kernel only the fields it knows about. (though it may understand all of them). So libbpf has to deal will all combinations: old/new kernel, old/new llvm (and corresponding .BTF and .BTF.ext sections). Now my understanding that you're mainly concerned with elf file format, correct? I'm making this guess, because you're arguing strongly about KIND_MAP and blasting our __btf_map* hack. I'm all for improving this part, but it cannot go into .BTF, because we already have an api to pass btf_key_type_id, btf_value_type_id in MAP_CREATE syscall. Adding new BTF_KIND_MAP into .BTF would be pointless, since we cannot change MAP_CREATE. As discussed on the call, currently we're designing KIND_VARIABLE that will describe global/static variables in .BTF and corresponding prog_load api changes for the kernel. We'll be talking about it at bpf uconf. Imo current 'struct bpf_map_def map_foo;' hack that libbpf/iproute2 and other loaders are dealing with will be cleaned up by this KIND_VARIABLE BTF support. Because of rule #1 we cannot pattern match 'bpf_map_def' name in llvm. All global variables have to be dealt in the common way by the llvm. We can add new __builtin_ specifically for map creation and ask folks to start using new interface in .c programs for map creation. All that is exciting, but I'd like to table that discussion and focus on this patch set which is about adding bpf_func_infos, BTF vs BTF.ext split, instances vs types. > What I have in mind comes in two parts: > 1) map type. A new BTF_KIND_MAP with metadata 'key_type', 'value_type' > (both are type_ids referencing other BTF type records), describing the > type "map from key_type to value_type". > 2) record in the 'instances' table. This would have a name_off (the > name of the map), a type_id (pointing at a BTF_KIND_MAP in the 'types' > table), and potentially also some indication of what symbol (from > section 'maps') refers to this map. This is pretty much the exact > same metadata that a function in the 'instances' table has, the only > differences being > (a) function's type_id points at a BTF_KIND_FUNC record > (b) function's symbol indication refers from .text section > (c) in future functions may be nested inside other functions, whereas > AIUI a map can't live inside a function. (But a variable, which is > the other thing that would want to go in an 'instances' table, can.) > So the 'instances' table record structure looks like > > struct btf_instance { > __u32 type_id; /* Type of object declared. An index into type section */ > __u32 name_off; /* Name of object. An offset into string section */ > __u32 parent; /* Containing object if any (else 0). An index into instance section */ > }; I have two issues with above structure: 1. it's not naturally extensible. Meaning it's possible to define new struct by bumping version in btf header, but it's not clean. 2. parent field has to be checked for loops Both of these issues are resolved by existing BTF and kernel code. BTF layout is extensible. BTF kernel verifier already checks for loops. Hence I propose to use existing 'struct btf_type' for these new instances section instead of inventing new format. > and we extend the BTF header: > > struct btf_header { > __u16 magic; > __u8 version; > __u8 flags; > __u32 hdr_len; > > /* All offsets are in bytes relative to the end of this header */ > __u32 type_off; /* offset of type section */ > __u32 type_len; /* length of type section */ > __u32 str_off; /* offset of string section */ > __u32 str_len; /* length of string section */ > __u32 inst_off; /* offset of instance section */ > __u32 inst_len; /* length of instance section */ The addition of above two fields is certainly doable and fits design of BTF well. Kernel side already has code to deal with multiple BTF sections. We don't even need to bump version for that. That's great, but the key point here is that btf.h is both kernel abi and file format. If llvm emits this instance section into elf file libbpf loader should better pass it to the kernel as well. Otherwise having populated instance section in elf and empty instances for the kernel will make kernel patch to uapi/btf.h non-upstreamble. Why ? Because we do not add fields to uapi structs that kernel will not be using. I suspect you're actually proposing to pass populated instance section to the kernel as part of BTF_LOAD sys_bpf command. So all good. Just making sure we're on the same page. Correct? So the tweak to your 'struct btf_instance' proposal is to use 'struct btf_type' of variable length instead in this instances section. And define new BTF_KIND_FUNC that can only be used in the instances. In the future we will add BTF_KIND_VARIABLE in there too. As far as type section there we will use BTF_KIND_FUNC_PROTO only. It will always have empty name and can have full names for function arguments. BTF_KIND_FUNC in instances section must have non-zero name_off (which will describe function name) and type_id pointing to BTF_KIND_FUNC_PROTO in type section. Multiple KIND_FUNC potentially can point to the same KIND_FUNC_PROTO if all argument names are the same or all of them are empty. (and types match, of course). I'm not counting on overall BTF size compression because of that, but it's kinda nice to have this option. > Then in the .BTF.ext section, we have both > > struct bpf_func_info { > __u32 prog_symbol; /* Index of symbol giving address of subprog */ > __u32 inst_id; /* Index into instance section */ > } Right, for .BPF.ext section we can keep existing 'struct bpf_func_info' with the minor tweak: struct bpf_func_info { __u32 insn_offset; __u32 inst_id; // this is now index in the instance section }; Since instance section will have KIND_FUNC and KIND_VARIABLE in the future the kernel has to check that 'bpf_func_info->inst_id' in .BTF.ext points to KIND_FUNC in .BTF. How does this sound? If we agree to that it will unblock us for this patch set and for follow on patch set that adds 'struct bpf_line_info' based on the same principles. > > struct bpf_map_info { > { > __u32 map_symbol; /* Index of symbol creating this map */ > __u32 inst_id; /* Index into instance section */ > } The map discussion I'd like to table for now due to reasons outlined above. > think this question of maps should be discussed in tomorrow's > call, since it is when we start having other kinds of instances turned out most of us have a conflict, so the earliest is 1:30pm on Friday. still works for you? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-09 4:35 ` Alexei Starovoitov @ 2018-11-09 20:00 ` Edward Cree 2018-11-09 21:14 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-11-09 20:00 UTC (permalink / raw) To: Alexei Starovoitov Cc: Martin KaFai Lau, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Network Development, Kernel Team On 09/11/18 04:35, Alexei Starovoitov wrote: > On Thu, Nov 08, 2018 at 10:56:55PM +0000, Edward Cree wrote: >> think this question of maps should be discussed in tomorrow's >> call, since it is when we start having other kinds of instances > turned out most of us have a conflict, so the earliest is 1:30pm on Friday. > still works for you? Yep (that's 9.30pm GMT right?) I'm assuming same bluejeans link again. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-09 20:00 ` Edward Cree @ 2018-11-09 21:14 ` Alexei Starovoitov 2018-11-09 21:28 ` Edward Cree 0 siblings, 1 reply; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-09 21:14 UTC (permalink / raw) To: Edward Cree, Alexei Starovoitov Cc: Martin Lau, Yonghong Song, Daniel Borkmann, Network Development, Kernel Team On 11/9/18 12:00 PM, Edward Cree wrote: > On 09/11/18 04:35, Alexei Starovoitov wrote: >> On Thu, Nov 08, 2018 at 10:56:55PM +0000, Edward Cree wrote: >>> think this question of maps should be discussed in tomorrow's >>> call, since it is when we start having other kinds of instances >> turned out most of us have a conflict, so the earliest is 1:30pm on Friday. >> still works for you? > > Yep (that's 9.30pm GMT right?) > > I'm assuming same bluejeans link again. same link, but i cannot make it right now. have to extinguish few fires. may be at 2pm (unlikely) or 3pm (more likely) PST? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-09 21:14 ` Alexei Starovoitov @ 2018-11-09 21:28 ` Edward Cree 2018-11-09 21:48 ` Alexei Starovoitov 0 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-11-09 21:28 UTC (permalink / raw) To: Alexei Starovoitov, Alexei Starovoitov Cc: Martin Lau, Yonghong Song, Daniel Borkmann, Network Development, Kernel Team On 09/11/18 21:14, Alexei Starovoitov wrote: > same link, but i cannot make it right now. > have to extinguish few fires. > may be at 2pm (unlikely) or 3pm (more likely) PST? Yep I can do either of those, just let me know which when you can. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO 2018-11-09 21:28 ` Edward Cree @ 2018-11-09 21:48 ` Alexei Starovoitov 0 siblings, 0 replies; 37+ messages in thread From: Alexei Starovoitov @ 2018-11-09 21:48 UTC (permalink / raw) To: Edward Cree, Alexei Starovoitov Cc: Martin Lau, Yonghong Song, Daniel Borkmann, Network Development, Kernel Team On 11/9/18 1:28 PM, Edward Cree wrote: > On 09/11/18 21:14, Alexei Starovoitov wrote: >> same link, but i cannot make it right now. >> have to extinguish few fires. >> may be at 2pm (unlikely) or 3pm (more likely) PST? > > Yep I can do either of those, just let me know which when you can. still swamped. but see the light. let's do 3pm ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH bpf-next v2 03/13] tools/bpf: sync kernel btf.h header 2018-10-17 7:23 [PATCH bpf-next v2 00/13] bpf: add btf func info support Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 01/13] bpf: btf: Break up btf_type_is_void() Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO Yonghong Song @ 2018-10-17 7:23 ` Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 04/13] tools/bpf: add btf func/func_proto unit tests in selftest test_btf Yonghong Song 2018-10-17 11:02 ` [PATCH bpf-next v2 00/13] bpf: add btf func info support Edward Cree 4 siblings, 0 replies; 37+ messages in thread From: Yonghong Song @ 2018-10-17 7:23 UTC (permalink / raw) To: ast, kafai, daniel, netdev; +Cc: kernel-team The kernel uapi btf.h is synced to the tools directory. Signed-off-by: Martin KaFai Lau <kafai@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/include/uapi/linux/btf.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h index 972265f32871..63f8500e6f34 100644 --- a/tools/include/uapi/linux/btf.h +++ b/tools/include/uapi/linux/btf.h @@ -40,7 +40,8 @@ struct btf_type { /* "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 and RESTRICT. + * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT, + * FUNC and FUNC_PROTO. * "type" is a type_id referring to another type. */ union { @@ -64,8 +65,10 @@ struct btf_type { #define BTF_KIND_VOLATILE 9 /* Volatile */ #define BTF_KIND_CONST 10 /* Const */ #define BTF_KIND_RESTRICT 11 /* Restrict */ -#define BTF_KIND_MAX 11 -#define NR_BTF_KINDS 12 +#define BTF_KIND_FUNC 12 /* Function */ +#define BTF_KIND_FUNC_PROTO 13 /* Function Prototype */ +#define BTF_KIND_MAX 13 +#define NR_BTF_KINDS 14 /* For some specific BTF_KIND, "struct btf_type" is immediately * followed by extra data. -- 2.17.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH bpf-next v2 04/13] tools/bpf: add btf func/func_proto unit tests in selftest test_btf 2018-10-17 7:23 [PATCH bpf-next v2 00/13] bpf: add btf func info support Yonghong Song ` (2 preceding siblings ...) 2018-10-17 7:23 ` [PATCH bpf-next v2 03/13] tools/bpf: sync kernel btf.h header Yonghong Song @ 2018-10-17 7:23 ` Yonghong Song 2018-10-17 11:02 ` [PATCH bpf-next v2 00/13] bpf: add btf func info support Edward Cree 4 siblings, 0 replies; 37+ messages in thread From: Yonghong Song @ 2018-10-17 7:23 UTC (permalink / raw) To: ast, kafai, daniel, netdev; +Cc: kernel-team Add several BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO unit tests in bpf selftest test_btf. Signed-off-by: Martin KaFai Lau <kafai@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/lib/bpf/btf.c | 4 + tools/testing/selftests/bpf/test_btf.c | 216 +++++++++++++++++++++++++ 2 files changed, 220 insertions(+) diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 449591aa9900..33095fc1860b 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -165,6 +165,10 @@ static int btf_parse_type_sec(struct btf *btf, btf_print_fn_t err_log) case BTF_KIND_ENUM: next_type += vlen * sizeof(struct btf_enum); break; + case BTF_KIND_FUNC: + case BTF_KIND_FUNC_PROTO: + next_type += vlen * sizeof(int); + break; case BTF_KIND_TYPEDEF: case BTF_KIND_PTR: case BTF_KIND_FWD: diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c index f42b3396d622..b6461c3c5e11 100644 --- a/tools/testing/selftests/bpf/test_btf.c +++ b/tools/testing/selftests/bpf/test_btf.c @@ -1374,6 +1374,222 @@ static struct btf_raw_test raw_tests[] = { .map_create_err = true, }, +{ + .descr = "func pointer #1", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + /* int (*func)(int, unsigned int) */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 2), 1), /* [3] */ + 1, 2, + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3), /* [4] */ + BTF_END_RAW, + }, + .str_sec = "", + .str_sec_size = sizeof(""), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, +}, + +{ + .descr = "func pointer #2", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + /* void (*func)(int, unsigned int, ....) */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 3), 0), /* [3] */ + 1, 2, 0, + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3), /* [4] */ + BTF_END_RAW, + }, + .str_sec = "", + .str_sec_size = sizeof(""), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, +}, + +{ + .descr = "func pointer #3", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + /* void (*func)(void, int, unsigned int) */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 3), 0), /* [3] */ + 1, 0, 2, + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3), /* [4] */ + BTF_END_RAW, + }, + .str_sec = "", + .str_sec_size = sizeof(""), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, + .btf_load_err = true, + .err_str = "Invalid arg#2", +}, + +{ + .descr = "func pointer #4", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + /* + * Testing: + * BTF_KIND_CONST => BTF_KIND_TYPEDEF => BTF_KIND_PTR => + * BTF_KIND_FUNC_PROTO + */ + /* typedef void (*func_ptr)(int, unsigned int) */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_TYPEDEF, 0, 0), 5),/* [3] */ + /* const func_ptr */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_CONST, 0, 0), 3), /* [4] */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 6), /* [5] */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 2), 0), /* [6] */ + 1, 2, + BTF_END_RAW, + }, + .str_sec = "", + .str_sec_size = sizeof(""), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, +}, + +{ + .descr = "func pointer #5", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + /* + * Skipped the BTF_KIND_PTR. + * BTF_KIND_CONST => BTF_KIND_TYPEDEF => BTF_KIND_FUNC_PROTO + */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_CONST, 0, 0), 4), /* [3] */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_TYPEDEF, 0, 0), 5),/* [4] */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 2), 0), /* [5] */ + 1, 2, + BTF_END_RAW, + }, + .str_sec = "", + .str_sec_size = sizeof(""), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, + .btf_load_err = true, + .err_str = "Invalid type_id", +}, + +{ + /* Test btf_resolve() in btf_check_func() */ + .descr = "func pointer #6", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + /* void (*func)(const void *) */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 1), 0), /* [2] */ + 4, + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 2), /* [3] */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_CONST, 0, 0), 5), /* [4] */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 0), /* [5] */ + BTF_END_RAW, + }, + .str_sec = "", + .str_sec_size = sizeof(""), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, +}, + +{ + .descr = "func #1", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [3] */ + 1, 2, + BTF_END_RAW, + }, + .str_sec = "\0A\0", + .str_sec_size = sizeof("\0A\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, +}, + +{ + .descr = "func #2", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1), /* [3] */ + 1, 2, + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3), /* [4] */ + BTF_END_RAW, + }, + .str_sec = "\0A\0", + .str_sec_size = sizeof("\0A\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, + .btf_load_err = true, + .err_str = "Invalid type_id", +}, + +{ + .descr = "func #3", + .raw_types = { + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + BTF_TYPE_INT_ENC(0, 0, 0, 32, 4), /* [2] */ + BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_TYPEDEF, 0, 0), 4),/* [3] */ + BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 0), /* [4] */ + 1, 2, + BTF_END_RAW, + }, + .str_sec = "\0A\0", + .str_sec_size = sizeof("\0A\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .map_name = "func_type_check_btf", + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 4, + .btf_load_err = true, + .err_str = "Invalid type_id", +}, + }; /* struct btf_raw_test raw_tests[] */ static const char *get_next_str(const char *start, const char *end) -- 2.17.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 00/13] bpf: add btf func info support 2018-10-17 7:23 [PATCH bpf-next v2 00/13] bpf: add btf func info support Yonghong Song ` (3 preceding siblings ...) 2018-10-17 7:23 ` [PATCH bpf-next v2 04/13] tools/bpf: add btf func/func_proto unit tests in selftest test_btf Yonghong Song @ 2018-10-17 11:02 ` Edward Cree 2018-10-17 16:13 ` Yonghong Song 4 siblings, 1 reply; 37+ messages in thread From: Edward Cree @ 2018-10-17 11:02 UTC (permalink / raw) To: Yonghong Song, ast, kafai, daniel, netdev; +Cc: kernel-team I think the BTF work needs to be better documented; at the moment the only way to determine how BTF sections are structured is to read through the headers, and cross-reference with the DWARF spec to guess at the semantics of various fields. I've been working on adding BTF support to ebpf_asm, and finding very frustrating the amount of guesswork required. Therefore please make sure that each patch extending the BTF format includes documentation patches describing both the layout and the semantics of the new extensions. For example in patch #9 there is no explanation of btf_ext_header.line_info_off and btf_ext_header.line_info_len (they're not even used by the code, so one cannot reverse-engineer it); while it's fairly clear that they indicate the bounds of the line_info subsection, there is no specification of what this subsection contains. -Ed ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH bpf-next v2 00/13] bpf: add btf func info support 2018-10-17 11:02 ` [PATCH bpf-next v2 00/13] bpf: add btf func info support Edward Cree @ 2018-10-17 16:13 ` Yonghong Song 0 siblings, 0 replies; 37+ messages in thread From: Yonghong Song @ 2018-10-17 16:13 UTC (permalink / raw) To: Edward Cree, Alexei Starovoitov, Martin Lau, daniel, netdev; +Cc: Kernel Team On 10/17/18 4:02 AM, Edward Cree wrote: > I think the BTF work needs to be better documented; at the moment the only way > to determine how BTF sections are structured is to read through the headers, > and cross-reference with the DWARF spec to guess at the semantics of various > fields. I've been working on adding BTF support to ebpf_asm, and finding > very frustrating the amount of guesswork required. > Therefore please make sure that each patch extending the BTF format includes > documentation patches describing both the layout and the semantics of the new Make sense. I will add some comments to describe the layout in patch #9. > extensions. For example in patch #9 there is no explanation of > btf_ext_header.line_info_off and btf_ext_header.line_info_len (they're not > even used by the code, so one cannot reverse-engineer it); while it's fairly > clear that they indicate the bounds of the line_info subsection, there is no The line_info field is added because it is implemented in llvm. I imported it to kernel tools directory to be compatible with what llvm generates although we did not process it yet. I will add a comment on this. In the long term, I guess we should add description of format etc. in Documentation/bpf directory like BTF.rst. > specification of what this subsection contains. > > -Ed > ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2018-11-10 7:32 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-17 7:23 [PATCH bpf-next v2 00/13] bpf: add btf func info support Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 01/13] bpf: btf: Break up btf_type_is_void() Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO Yonghong Song 2018-10-17 16:13 ` Edward Cree 2018-10-17 17:25 ` Yonghong Song 2018-10-17 17:50 ` Martin Lau 2018-10-18 16:47 ` Edward Cree 2018-10-18 18:12 ` Martin Lau 2018-10-18 19:41 ` Edward Cree 2018-10-18 21:19 ` Martin Lau 2018-10-19 17:04 ` Edward Cree 2018-10-19 19:36 ` Martin Lau 2018-10-19 20:35 ` Martin Lau 2018-10-19 21:26 ` Edward Cree 2018-10-19 23:27 ` Martin Lau 2018-11-01 21:08 ` Edward Cree 2018-11-06 6:29 ` Alexei Starovoitov 2018-11-06 18:52 ` Edward Cree 2018-11-06 21:56 ` Alexei Starovoitov 2018-11-06 22:58 ` Edward Cree 2018-11-07 0:59 ` Alexei Starovoitov 2018-11-07 19:29 ` Edward Cree 2018-11-07 21:49 ` Alexei Starovoitov 2018-11-08 17:58 ` Edward Cree 2018-11-08 18:21 ` Alexei Starovoitov 2018-11-08 19:42 ` Alexei Starovoitov 2018-11-08 22:56 ` Edward Cree 2018-11-09 1:26 ` Yonghong Song 2018-11-09 4:35 ` Alexei Starovoitov 2018-11-09 20:00 ` Edward Cree 2018-11-09 21:14 ` Alexei Starovoitov 2018-11-09 21:28 ` Edward Cree 2018-11-09 21:48 ` Alexei Starovoitov 2018-10-17 7:23 ` [PATCH bpf-next v2 03/13] tools/bpf: sync kernel btf.h header Yonghong Song 2018-10-17 7:23 ` [PATCH bpf-next v2 04/13] tools/bpf: add btf func/func_proto unit tests in selftest test_btf Yonghong Song 2018-10-17 11:02 ` [PATCH bpf-next v2 00/13] bpf: add btf func info support Edward Cree 2018-10-17 16:13 ` Yonghong Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.