| Message ID | 20220529223646.862464-3-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf_loop inlining | expand |
On Sun, May 29, 2022 at 3:37 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > The BTF and func_info specification for test_verifier tests follows > the same notation as in prog_tests/btf.c tests. E.g.: > > ... > .func_info = { { 0, 6 }, { 8, 7 } }, > .func_info_cnt = 2, > .btf_strings = "\0int\0", > .btf_types = { > BTF_TYPE_INT_ENC(1, BTF_INT_SIGNED, 0, 32, 4), > BTF_PTR_ENC(1), > }, > ... > > The BTF specification is loaded only when specified. > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> This can be very useful! Thanks! > --- > tools/testing/selftests/bpf/prog_tests/btf.c | 1 - > tools/testing/selftests/bpf/test_btf.h | 2 + > tools/testing/selftests/bpf/test_verifier.c | 110 ++++++++++++++++--- > 3 files changed, 95 insertions(+), 18 deletions(-) [...] > > -static int load_btf(void) > +static char bpf_vlog[UINT_MAX >> 8]; > + > +static int load_btf_spec(__u32 *types, int types_len, > + const char *strings, int strings_len) > { > struct btf_header hdr = { > .magic = BTF_MAGIC, > .version = BTF_VERSION, > .hdr_len = sizeof(struct btf_header), > - .type_len = sizeof(btf_raw_types), > - .str_off = sizeof(btf_raw_types), > - .str_len = sizeof(btf_str_sec), > + .type_len = types_len, > + .str_off = types_len, > + .str_len = strings_len, > }; > void *ptr, *raw_btf; > int btf_fd; > > - ptr = raw_btf = malloc(sizeof(hdr) + sizeof(btf_raw_types) + > - sizeof(btf_str_sec)); > + raw_btf = malloc(sizeof(hdr) + types_len + strings_len); > > + ptr = raw_btf; > memcpy(ptr, &hdr, sizeof(hdr)); > ptr += sizeof(hdr); > - memcpy(ptr, btf_raw_types, hdr.type_len); > + memcpy(ptr, types, hdr.type_len); > ptr += hdr.type_len; > - memcpy(ptr, btf_str_sec, hdr.str_len); > + memcpy(ptr, strings, hdr.str_len); > ptr += hdr.str_len; > > - btf_fd = bpf_btf_load(raw_btf, ptr - raw_btf, NULL); > - free(raw_btf); > + LIBBPF_OPTS(bpf_btf_load_opts, opts, > + .log_buf = bpf_vlog, > + .log_size = sizeof(bpf_vlog), > + .log_level = (verbose > + ? VERBOSE_LIBBPF_LOG_LEVEL > + : DEFAULT_LIBBPF_LOG_LEVEL), > + ); Please move the local variable definition to the beginning of the function. > + > + btf_fd = bpf_btf_load(raw_btf, ptr - raw_btf, &opts); > if (btf_fd < 0) > - return -1; > - return btf_fd; > + printf("Failed to load BTF spec: '%s'\n", strerror(errno)); > + > + free(raw_btf); > + > + return btf_fd < 0 ? -1 : btf_fd; > +} > + [...] > + > static void do_test_single(struct bpf_test *test, bool unpriv, > int *passes, int *errors) > { > - int fd_prog, expected_ret, alignment_prevented_execution; > + int fd_prog, btf_fd, expected_ret, alignment_prevented_execution; > int prog_len, prog_type = test->prog_type; > struct bpf_insn *prog = test->insns; > LIBBPF_OPTS(bpf_prog_load_opts, opts); > @@ -1366,8 +1424,10 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > __u32 pflags; > int i, err; > > + fd_prog = -1; This is not really necessary. > for (i = 0; i < MAX_NR_MAPS; i++) > map_fds[i] = -1; > + btf_fd = -1; > > if (!prog_type) > prog_type = BPF_PROG_TYPE_SOCKET_FILTER; [...] > @@ -1540,6 +1614,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > fail_log: > (*errors)++; > printf("%s", bpf_vlog); > + if (!string_ends_with_nl(bpf_vlog, sizeof(bpf_vlog))) > + printf("\n"); This seems like overkill. Can we just print "\n" in all cases? > goto close_fds; > } > > -- > 2.25.1 >
> On Tue, 2022-05-31 at 13:52 -0700, Song Liu wrote: Hi Song, Thanks a lot for the review, I'll apply the suggested changes and provide the v3 in one or two days. My only objection is below. > > { > > - int fd_prog, expected_ret, alignment_prevented_execution; > > + int fd_prog, btf_fd, expected_ret, alignment_prevented_execution; > > int prog_len, prog_type = test->prog_type; > > struct bpf_insn *prog = test->insns; > > LIBBPF_OPTS(bpf_prog_load_opts, opts); __u32 pflags; > > int i, err; > > > > + fd_prog = -1; > > This is not really necessary. Actually this one is necessary to avoid compiler warning, note the following fragment of the do_test_single function below: static void do_test_single(...) { ... if (...) { btf_fd = load_btf_for_test(...); if (btf_fd < 0) goto fail_log; opts.prog_btf_fd = btf_fd; } ... fd_prog = bpf_prog_load(..., &opts); ... close_fds: ... close(fd_prog); close(btf_fd); ... return; fail_log: ... goto close_fds; } If load_btf_for_test fails the goto fail_log would eventually jump to close_fds, where fd_prog would be in an uninitialised state. Best regards, Eduard
On Tue, May 31, 2022 at 4:20 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > On Tue, 2022-05-31 at 13:52 -0700, Song Liu wrote: > > Hi Song, > > Thanks a lot for the review, I'll apply the suggested changes and > provide the v3 in one or two days. My only objection is below. > > > > { > > > - int fd_prog, expected_ret, alignment_prevented_execution; > > > + int fd_prog, btf_fd, expected_ret, alignment_prevented_execution; > > > int prog_len, prog_type = test->prog_type; > > > struct bpf_insn *prog = test->insns; > > > LIBBPF_OPTS(bpf_prog_load_opts, opts); __u32 pflags; > > > int i, err; > > > > > > + fd_prog = -1; > > > > This is not really necessary. > > Actually this one is necessary to avoid compiler warning, note the > following fragment of the do_test_single function below: > > static void do_test_single(...) > { > ... > if (...) { > btf_fd = load_btf_for_test(...); > if (btf_fd < 0) > goto fail_log; > opts.prog_btf_fd = btf_fd; > } > ... > fd_prog = bpf_prog_load(..., &opts); > ... > close_fds: > ... > close(fd_prog); > close(btf_fd); > ... > return; > fail_log: > ... > goto close_fds; > } > > If load_btf_for_test fails the goto fail_log would eventually jump to > close_fds, where fd_prog would be in an uninitialised state. Got it. Thanks for the explanation! Song
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c index ba5bde53d418..bebc98bad615 100644 --- a/tools/testing/selftests/bpf/prog_tests/btf.c +++ b/tools/testing/selftests/bpf/prog_tests/btf.c @@ -34,7 +34,6 @@ static bool always_log; #undef CHECK #define CHECK(condition, format...) _CHECK(condition, "check", duration, format) -#define BTF_END_RAW 0xdeadbeef #define NAME_TBD 0xdeadb33f #define NAME_NTH(N) (0xfffe0000 | N) diff --git a/tools/testing/selftests/bpf/test_btf.h b/tools/testing/selftests/bpf/test_btf.h index 128989bed8b7..044663c45a57 100644 --- a/tools/testing/selftests/bpf/test_btf.h +++ b/tools/testing/selftests/bpf/test_btf.h @@ -4,6 +4,8 @@ #ifndef _TEST_BTF_H #define _TEST_BTF_H +#define BTF_END_RAW 0xdeadbeef + #define BTF_INFO_ENC(kind, kind_flag, vlen) \ ((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN)) diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 13468221d227..b6dfa633bffa 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -59,11 +59,17 @@ #define MAX_TEST_RUNS 8 #define POINTER_VALUE 0xcafe4all #define TEST_DATA_LEN 64 +#define MAX_FUNC_INFOS 8 +#define MAX_BTF_STRINGS 256 +#define MAX_BTF_TYPES 256 #define INSN_OFF_MASK ((s16)0xFFFF) #define INSN_IMM_MASK ((s32)0xFFFFFFFF) #define SKIP_INSNS() BPF_RAW_INSN(0xde, 0xa, 0xd, 0xbeef, 0xdeadbeef) +#define DEFAULT_LIBBPF_LOG_LEVEL 4 +#define VERBOSE_LIBBPF_LOG_LEVEL 1 + #define F_NEEDS_EFFICIENT_UNALIGNED_ACCESS (1 << 0) #define F_LOAD_WITH_STRICT_ALIGNMENT (1 << 1) @@ -154,6 +160,14 @@ struct bpf_test { }; enum bpf_attach_type expected_attach_type; const char *kfunc; + struct bpf_func_info func_info[MAX_FUNC_INFOS]; + int func_info_cnt; + char btf_strings[MAX_BTF_STRINGS]; + /* A set of BTF types to load when specified, + * use macro definitions from test_btf.h, + * must end with BTF_END_RAW + */ + __u32 btf_types[MAX_BTF_TYPES]; }; /* Note we want this to be 64 bit aligned so that the end of our array is @@ -683,34 +697,67 @@ static __u32 btf_raw_types[] = { BTF_MEMBER_ENC(71, 13, 128), /* struct prog_test_member __kptr_ref *ptr; */ }; -static int load_btf(void) +static char bpf_vlog[UINT_MAX >> 8]; + +static int load_btf_spec(__u32 *types, int types_len, + const char *strings, int strings_len) { struct btf_header hdr = { .magic = BTF_MAGIC, .version = BTF_VERSION, .hdr_len = sizeof(struct btf_header), - .type_len = sizeof(btf_raw_types), - .str_off = sizeof(btf_raw_types), - .str_len = sizeof(btf_str_sec), + .type_len = types_len, + .str_off = types_len, + .str_len = strings_len, }; void *ptr, *raw_btf; int btf_fd; - ptr = raw_btf = malloc(sizeof(hdr) + sizeof(btf_raw_types) + - sizeof(btf_str_sec)); + raw_btf = malloc(sizeof(hdr) + types_len + strings_len); + ptr = raw_btf; memcpy(ptr, &hdr, sizeof(hdr)); ptr += sizeof(hdr); - memcpy(ptr, btf_raw_types, hdr.type_len); + memcpy(ptr, types, hdr.type_len); ptr += hdr.type_len; - memcpy(ptr, btf_str_sec, hdr.str_len); + memcpy(ptr, strings, hdr.str_len); ptr += hdr.str_len; - btf_fd = bpf_btf_load(raw_btf, ptr - raw_btf, NULL); - free(raw_btf); + LIBBPF_OPTS(bpf_btf_load_opts, opts, + .log_buf = bpf_vlog, + .log_size = sizeof(bpf_vlog), + .log_level = (verbose + ? VERBOSE_LIBBPF_LOG_LEVEL + : DEFAULT_LIBBPF_LOG_LEVEL), + ); + + btf_fd = bpf_btf_load(raw_btf, ptr - raw_btf, &opts); if (btf_fd < 0) - return -1; - return btf_fd; + printf("Failed to load BTF spec: '%s'\n", strerror(errno)); + + free(raw_btf); + + return btf_fd < 0 ? -1 : btf_fd; +} + +static int load_btf(void) +{ + return load_btf_spec(btf_raw_types, sizeof(btf_raw_types), + btf_str_sec, sizeof(btf_str_sec)); +} + +static int load_btf_for_test(struct bpf_test *test) +{ + int types_num = 0; + + while (types_num < MAX_BTF_TYPES && + test->btf_types[types_num] != BTF_END_RAW) + ++types_num; + + int types_len = types_num * sizeof(test->btf_types[0]); + + return load_btf_spec(test->btf_types, types_len, + test->btf_strings, sizeof(test->btf_strings)); } static int create_map_spin_lock(void) @@ -789,8 +836,6 @@ static int create_map_kptr(void) return fd; } -static char bpf_vlog[UINT_MAX >> 8]; - static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type, struct bpf_insn *prog, int *map_fds) { @@ -1351,10 +1396,23 @@ static bool check_xlated_program(struct bpf_test *test, int fd_prog) return result; } +static bool string_ends_with_nl(char *str, int str_len) +{ + bool ends_with_nl = false; + + for (int i = 0; i < str_len; ++i) { + if (str[i] == 0) + break; + ends_with_nl = str[i] == '\n'; + } + + return ends_with_nl; +} + static void do_test_single(struct bpf_test *test, bool unpriv, int *passes, int *errors) { - int fd_prog, expected_ret, alignment_prevented_execution; + int fd_prog, btf_fd, expected_ret, alignment_prevented_execution; int prog_len, prog_type = test->prog_type; struct bpf_insn *prog = test->insns; LIBBPF_OPTS(bpf_prog_load_opts, opts); @@ -1366,8 +1424,10 @@ static void do_test_single(struct bpf_test *test, bool unpriv, __u32 pflags; int i, err; + fd_prog = -1; for (i = 0; i < MAX_NR_MAPS; i++) map_fds[i] = -1; + btf_fd = -1; if (!prog_type) prog_type = BPF_PROG_TYPE_SOCKET_FILTER; @@ -1400,11 +1460,11 @@ static void do_test_single(struct bpf_test *test, bool unpriv, opts.expected_attach_type = test->expected_attach_type; if (verbose) - opts.log_level = 1; + opts.log_level = VERBOSE_LIBBPF_LOG_LEVEL; else if (expected_ret == VERBOSE_ACCEPT) opts.log_level = 2; else - opts.log_level = 4; + opts.log_level = DEFAULT_LIBBPF_LOG_LEVEL; opts.prog_flags = pflags; if (prog_type == BPF_PROG_TYPE_TRACING && test->kfunc) { @@ -1422,6 +1482,19 @@ static void do_test_single(struct bpf_test *test, bool unpriv, opts.attach_btf_id = attach_btf_id; } + if (test->btf_types[0] != 0) { + btf_fd = load_btf_for_test(test); + if (btf_fd < 0) + goto fail_log; + opts.prog_btf_fd = btf_fd; + } + + if (test->func_info_cnt != 0) { + opts.func_info = test->func_info; + opts.func_info_cnt = test->func_info_cnt; + opts.func_info_rec_size = sizeof(test->func_info[0]); + } + opts.log_buf = bpf_vlog; opts.log_size = sizeof(bpf_vlog); fd_prog = bpf_prog_load(prog_type, NULL, "GPL", prog, prog_len, &opts); @@ -1533,6 +1606,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv, if (test->fill_insns) free(test->fill_insns); close(fd_prog); + close(btf_fd); for (i = 0; i < MAX_NR_MAPS; i++) close(map_fds[i]); sched_yield(); @@ -1540,6 +1614,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, fail_log: (*errors)++; printf("%s", bpf_vlog); + if (!string_ends_with_nl(bpf_vlog, sizeof(bpf_vlog))) + printf("\n"); goto close_fds; }
The BTF and func_info specification for test_verifier tests follows the same notation as in prog_tests/btf.c tests. E.g.: ... .func_info = { { 0, 6 }, { 8, 7 } }, .func_info_cnt = 2, .btf_strings = "\0int\0", .btf_types = { BTF_TYPE_INT_ENC(1, BTF_INT_SIGNED, 0, 32, 4), BTF_PTR_ENC(1), }, ... The BTF specification is loaded only when specified. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- tools/testing/selftests/bpf/prog_tests/btf.c | 1 - tools/testing/selftests/bpf/test_btf.h | 2 + tools/testing/selftests/bpf/test_verifier.c | 110 ++++++++++++++++--- 3 files changed, 95 insertions(+), 18 deletions(-)