| Message ID | 20181217182554.52170-5-sdf@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | skip verifier/map tests if kernel support is missing | expand |
On 12/17/2018 07:25 PM, Stanislav Fomichev wrote: > Use recently introduced bpf_map_type_supported() to skip tests in the > test_verifier if map creation (create_map) fails. It's handled > explicitly for each fixup, i.e. if bpf_create_map returns negative fd, > we probe the kernel for the appropriate map support and skip the > test is map type is not supported. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++-- > 1 file changed, 31 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > index 124d21306c27..d267f5248b5d 100644 > --- a/tools/testing/selftests/bpf/test_verifier.c > +++ b/tools/testing/selftests/bpf/test_verifier.c > @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu) > return fd; > } > > +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type) > +{ > + if (ret < 0 && !bpf_map_type_supported(map_type)) { > + printf("SKIP (unsupported map type %d)\n", map_type); > + skips++; > + return true; > + } > + return false; > +} > + > static char bpf_vlog[UINT_MAX >> 8]; > > -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > - struct bpf_insn *prog, int *map_fds) > +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > + struct bpf_insn *prog, int *map_fds) > { > int *fixup_map_hash_8b = test->fixup_map_hash_8b; > int *fixup_map_hash_48b = test->fixup_map_hash_48b; > @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > if (*fixup_cgroup_storage) { > map_fds[7] = create_cgroup_storage(false); > + if (skip_unsupported_map(map_fds[7], > + BPF_MAP_TYPE_CGROUP_STORAGE)) > + return -1; > do { > prog[*fixup_cgroup_storage].imm = map_fds[7]; > fixup_cgroup_storage++; > @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > if (*fixup_percpu_cgroup_storage) { > map_fds[8] = create_cgroup_storage(true); > + if (skip_unsupported_map(map_fds[8], > + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)) > + return -1; > do { > prog[*fixup_percpu_cgroup_storage].imm = map_fds[8]; > fixup_percpu_cgroup_storage++; > @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > if (*fixup_map_sockmap) { > map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int), > sizeof(int), 1); > + if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP)) > + return -1; > do { > prog[*fixup_map_sockmap].imm = map_fds[9]; > fixup_map_sockmap++; > @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > if (*fixup_map_sockhash) { > map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int), > sizeof(int), 1); > + if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH)) > + return -1; > do { > prog[*fixup_map_sockhash].imm = map_fds[10]; > fixup_map_sockhash++; > @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > if (*fixup_map_xskmap) { > map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int), > sizeof(int), 1); > + if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP)) > + return -1; > do { > prog[*fixup_map_xskmap].imm = map_fds[11]; > fixup_map_xskmap++; > @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > if (*fixup_map_stacktrace) { > map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32), > sizeof(u64), 1); > + if (skip_unsupported_map(map_fds[12], > + BPF_MAP_TYPE_STACK_TRACE)) > + return -1; Nit: Could probably be slightly simplified by moving this into and by reworking create_{map,cgroup_storage}() a bit. > do { > prog[*fixup_map_stacktrace].imm = map_fds[12]; > fixup_map_stacktrace++; > } while (fixup_map_stacktrace); > } > + > + return 0; > } > > static int set_admin(bool admin) > @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > > if (!prog_type) > prog_type = BPF_PROG_TYPE_SOCKET_FILTER; > - do_test_fixup(test, prog_type, prog, map_fds); > + if (do_test_fixup(test, prog_type, prog, map_fds) < 0) > + return; > prog_len = probe_filter_length(prog); > > pflags = 0; >
On 12/19, Daniel Borkmann wrote: > On 12/17/2018 07:25 PM, Stanislav Fomichev wrote: > > Use recently introduced bpf_map_type_supported() to skip tests in the > > test_verifier if map creation (create_map) fails. It's handled > > explicitly for each fixup, i.e. if bpf_create_map returns negative fd, > > we probe the kernel for the appropriate map support and skip the > > test is map type is not supported. > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > > index 124d21306c27..d267f5248b5d 100644 > > --- a/tools/testing/selftests/bpf/test_verifier.c > > +++ b/tools/testing/selftests/bpf/test_verifier.c > > @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu) > > return fd; > > } > > > > +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type) > > +{ > > + if (ret < 0 && !bpf_map_type_supported(map_type)) { > > + printf("SKIP (unsupported map type %d)\n", map_type); > > + skips++; > > + return true; > > + } > > + return false; > > +} > > + > > static char bpf_vlog[UINT_MAX >> 8]; > > > > -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > - struct bpf_insn *prog, int *map_fds) > > +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > + struct bpf_insn *prog, int *map_fds) > > { > > int *fixup_map_hash_8b = test->fixup_map_hash_8b; > > int *fixup_map_hash_48b = test->fixup_map_hash_48b; > > @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > > if (*fixup_cgroup_storage) { > > map_fds[7] = create_cgroup_storage(false); > > + if (skip_unsupported_map(map_fds[7], > > + BPF_MAP_TYPE_CGROUP_STORAGE)) > > + return -1; > > do { > > prog[*fixup_cgroup_storage].imm = map_fds[7]; > > fixup_cgroup_storage++; > > @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > > if (*fixup_percpu_cgroup_storage) { > > map_fds[8] = create_cgroup_storage(true); > > + if (skip_unsupported_map(map_fds[8], > > + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)) > > + return -1; > > do { > > prog[*fixup_percpu_cgroup_storage].imm = map_fds[8]; > > fixup_percpu_cgroup_storage++; > > @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > if (*fixup_map_sockmap) { > > map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int), > > sizeof(int), 1); > > + if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP)) > > + return -1; > > do { > > prog[*fixup_map_sockmap].imm = map_fds[9]; > > fixup_map_sockmap++; > > @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > if (*fixup_map_sockhash) { > > map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int), > > sizeof(int), 1); > > + if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH)) > > + return -1; > > do { > > prog[*fixup_map_sockhash].imm = map_fds[10]; > > fixup_map_sockhash++; > > @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > if (*fixup_map_xskmap) { > > map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int), > > sizeof(int), 1); > > + if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP)) > > + return -1; > > do { > > prog[*fixup_map_xskmap].imm = map_fds[11]; > > fixup_map_xskmap++; > > @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > if (*fixup_map_stacktrace) { > > map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32), > > sizeof(u64), 1); > > + if (skip_unsupported_map(map_fds[12], > > + BPF_MAP_TYPE_STACK_TRACE)) > > + return -1; > > Nit: Could probably be slightly simplified by moving this into and by reworking > create_{map,cgroup_storage}() a bit. Yeah, I stated that option in the cover letter. I did that initially, but it required some additional argument (skip/supported) to the create_{map,cgroup_storage} and I scrapped this approach due to too much plumbing. But I think since we are not doing any parallel tests in the verifier, we can do something like the following patch below. WDYT? --- diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 124d21306c27..43f71783f7b5 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -14113,6 +14113,16 @@ static int probe_filter_length(const struct bpf_insn *fp) return len + 1; } +static bool skip_unsupported_map(enum bpf_map_type map_type) +{ + if (!bpf_map_type_supported(map_type)) { + printf("SKIP (unsupported map type %d)\n", map_type); + skips++; + return true; + } + return false; +} + static int create_map(uint32_t type, uint32_t size_key, uint32_t size_value, uint32_t max_elem) { @@ -14120,8 +14130,11 @@ static int create_map(uint32_t type, uint32_t size_key, fd = bpf_create_map(type, size_key, size_value, max_elem, type == BPF_MAP_TYPE_HASH ? BPF_F_NO_PREALLOC : 0); - if (fd < 0) + if (fd < 0) { + if (skip_unsupported_map(type)) + return -1; printf("Failed to create hash map '%s'!\n", strerror(errno)); + } return fd; } @@ -14161,6 +14174,8 @@ static int create_prog_array(enum bpf_map_type prog_type, uint32_t max_elem, mfd = bpf_create_map(BPF_MAP_TYPE_PROG_ARRAY, sizeof(int), sizeof(int), max_elem, 0); if (mfd < 0) { + if (skip_unsupported_map(BPF_MAP_TYPE_PROG_ARRAY)) + return -1; printf("Failed to create prog array '%s'!\n", strerror(errno)); return -1; } @@ -14191,15 +14206,20 @@ static int create_map_in_map(void) inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(int), sizeof(int), 1, 0); if (inner_map_fd < 0) { + if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY)) + return -1; printf("Failed to create array '%s'!\n", strerror(errno)); return inner_map_fd; } outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL, sizeof(int), inner_map_fd, 1, 0); - if (outer_map_fd < 0) + if (outer_map_fd < 0) { + if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY_OF_MAPS)) + return -1; printf("Failed to create array of maps '%s'!\n", strerror(errno)); + } close(inner_map_fd); @@ -14214,9 +14234,12 @@ static int create_cgroup_storage(bool percpu) fd = bpf_create_map(type, sizeof(struct bpf_cgroup_storage_key), TEST_DATA_LEN, 0, 0); - if (fd < 0) + if (fd < 0) { + if (skip_unsupported_map(type)) + return -1; printf("Failed to create cgroup storage '%s'!\n", strerror(errno)); + } return fd; } @@ -14392,6 +14415,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv, int map_fds[MAX_NR_MAPS]; const char *expected_err; uint32_t expected_val; + int fixup_skips; uint32_t retval; __u32 pflags; int i, err; @@ -14401,7 +14425,13 @@ static void do_test_single(struct bpf_test *test, bool unpriv, if (!prog_type) prog_type = BPF_PROG_TYPE_SOCKET_FILTER; + fixup_skips = skips; do_test_fixup(test, prog_type, prog, map_fds); + /* If there were some map skips during fixup due to missing bpf + * features, skip this test. + */ + if (fixup_skips != skips) + return; prog_len = probe_filter_length(prog); pflags = 0; > > > do { > > prog[*fixup_map_stacktrace].imm = map_fds[12]; > > fixup_map_stacktrace++; > > } while (fixup_map_stacktrace); > > } > > + > > + return 0; > > } > > > > static int set_admin(bool admin) > > @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > > > > if (!prog_type) > > prog_type = BPF_PROG_TYPE_SOCKET_FILTER; > > - do_test_fixup(test, prog_type, prog, map_fds); > > + if (do_test_fixup(test, prog_type, prog, map_fds) < 0) > > + return; > > prog_len = probe_filter_length(prog); > > > > pflags = 0; > > >
On Tue, Dec 18, 2018 at 4:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 12/19, Daniel Borkmann wrote: > > On 12/17/2018 07:25 PM, Stanislav Fomichev wrote: > > > Use recently introduced bpf_map_type_supported() to skip tests in the > > > test_verifier if map creation (create_map) fails. It's handled > > > explicitly for each fixup, i.e. if bpf_create_map returns negative fd, > > > we probe the kernel for the appropriate map support and skip the > > > test is map type is not supported. > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > --- > > > tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++-- > > > 1 file changed, 31 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > > > index 124d21306c27..d267f5248b5d 100644 > > > --- a/tools/testing/selftests/bpf/test_verifier.c > > > +++ b/tools/testing/selftests/bpf/test_verifier.c > > > @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu) > > > return fd; > > > } > > > > > > +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type) > > > +{ > > > + if (ret < 0 && !bpf_map_type_supported(map_type)) { > > > + printf("SKIP (unsupported map type %d)\n", map_type); > > > + skips++; > > > + return true; > > > + } > > > + return false; > > > +} > > > + > > > static char bpf_vlog[UINT_MAX >> 8]; > > > > > > -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > - struct bpf_insn *prog, int *map_fds) > > > +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > + struct bpf_insn *prog, int *map_fds) > > > { > > > int *fixup_map_hash_8b = test->fixup_map_hash_8b; > > > int *fixup_map_hash_48b = test->fixup_map_hash_48b; > > > @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > > > > if (*fixup_cgroup_storage) { > > > map_fds[7] = create_cgroup_storage(false); > > > + if (skip_unsupported_map(map_fds[7], > > > + BPF_MAP_TYPE_CGROUP_STORAGE)) > > > + return -1; > > > do { > > > prog[*fixup_cgroup_storage].imm = map_fds[7]; > > > fixup_cgroup_storage++; > > > @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > > > > if (*fixup_percpu_cgroup_storage) { > > > map_fds[8] = create_cgroup_storage(true); > > > + if (skip_unsupported_map(map_fds[8], > > > + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)) > > > + return -1; > > > do { > > > prog[*fixup_percpu_cgroup_storage].imm = map_fds[8]; > > > fixup_percpu_cgroup_storage++; > > > @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > if (*fixup_map_sockmap) { > > > map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int), > > > sizeof(int), 1); > > > + if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP)) > > > + return -1; > > > do { > > > prog[*fixup_map_sockmap].imm = map_fds[9]; > > > fixup_map_sockmap++; > > > @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > if (*fixup_map_sockhash) { > > > map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int), > > > sizeof(int), 1); > > > + if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH)) > > > + return -1; > > > do { > > > prog[*fixup_map_sockhash].imm = map_fds[10]; > > > fixup_map_sockhash++; > > > @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > if (*fixup_map_xskmap) { > > > map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int), > > > sizeof(int), 1); > > > + if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP)) > > > + return -1; > > > do { > > > prog[*fixup_map_xskmap].imm = map_fds[11]; > > > fixup_map_xskmap++; > > > @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > > > if (*fixup_map_stacktrace) { > > > map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32), > > > sizeof(u64), 1); > > > + if (skip_unsupported_map(map_fds[12], > > > + BPF_MAP_TYPE_STACK_TRACE)) > > > + return -1; > > > > Nit: Could probably be slightly simplified by moving this into and by reworking > > create_{map,cgroup_storage}() a bit. > Yeah, I stated that option in the cover letter. I did that initially, > but it required some additional argument (skip/supported) to the > create_{map,cgroup_storage} and I scrapped this approach due to too > much plumbing. > > But I think since we are not doing any parallel tests in the verifier, > we can do something like the following patch below. WDYT? Daniel, should this go as is or you'd like me to respin to the version from my last reply (or something similar)? > > --- > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > index 124d21306c27..43f71783f7b5 100644 > --- a/tools/testing/selftests/bpf/test_verifier.c > +++ b/tools/testing/selftests/bpf/test_verifier.c > @@ -14113,6 +14113,16 @@ static int probe_filter_length(const struct bpf_insn *fp) > return len + 1; > } > > +static bool skip_unsupported_map(enum bpf_map_type map_type) > +{ > + if (!bpf_map_type_supported(map_type)) { > + printf("SKIP (unsupported map type %d)\n", map_type); > + skips++; > + return true; > + } > + return false; > +} > + > static int create_map(uint32_t type, uint32_t size_key, > uint32_t size_value, uint32_t max_elem) > { > @@ -14120,8 +14130,11 @@ static int create_map(uint32_t type, uint32_t size_key, > > fd = bpf_create_map(type, size_key, size_value, max_elem, > type == BPF_MAP_TYPE_HASH ? BPF_F_NO_PREALLOC : 0); > - if (fd < 0) > + if (fd < 0) { > + if (skip_unsupported_map(type)) > + return -1; > printf("Failed to create hash map '%s'!\n", strerror(errno)); > + } > > return fd; > } > @@ -14161,6 +14174,8 @@ static int create_prog_array(enum bpf_map_type prog_type, uint32_t max_elem, > mfd = bpf_create_map(BPF_MAP_TYPE_PROG_ARRAY, sizeof(int), > sizeof(int), max_elem, 0); > if (mfd < 0) { > + if (skip_unsupported_map(BPF_MAP_TYPE_PROG_ARRAY)) > + return -1; > printf("Failed to create prog array '%s'!\n", strerror(errno)); > return -1; > } > @@ -14191,15 +14206,20 @@ static int create_map_in_map(void) > inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(int), > sizeof(int), 1, 0); > if (inner_map_fd < 0) { > + if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY)) > + return -1; > printf("Failed to create array '%s'!\n", strerror(errno)); > return inner_map_fd; > } > > outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL, > sizeof(int), inner_map_fd, 1, 0); > - if (outer_map_fd < 0) > + if (outer_map_fd < 0) { > + if (skip_unsupported_map(BPF_MAP_TYPE_ARRAY_OF_MAPS)) > + return -1; > printf("Failed to create array of maps '%s'!\n", > strerror(errno)); > + } > > close(inner_map_fd); > > @@ -14214,9 +14234,12 @@ static int create_cgroup_storage(bool percpu) > > fd = bpf_create_map(type, sizeof(struct bpf_cgroup_storage_key), > TEST_DATA_LEN, 0, 0); > - if (fd < 0) > + if (fd < 0) { > + if (skip_unsupported_map(type)) > + return -1; > printf("Failed to create cgroup storage '%s'!\n", > strerror(errno)); > + } > > return fd; > } > @@ -14392,6 +14415,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > int map_fds[MAX_NR_MAPS]; > const char *expected_err; > uint32_t expected_val; > + int fixup_skips; > uint32_t retval; > __u32 pflags; > int i, err; > @@ -14401,7 +14425,13 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > > if (!prog_type) > prog_type = BPF_PROG_TYPE_SOCKET_FILTER; > + fixup_skips = skips; > do_test_fixup(test, prog_type, prog, map_fds); > + /* If there were some map skips during fixup due to missing bpf > + * features, skip this test. > + */ > + if (fixup_skips != skips) > + return; > prog_len = probe_filter_length(prog); > > pflags = 0; > > > > > > do { > > > prog[*fixup_map_stacktrace].imm = map_fds[12]; > > > fixup_map_stacktrace++; > > > } while (fixup_map_stacktrace); > > > } > > > + > > > + return 0; > > > } > > > > > > static int set_admin(bool admin) > > > @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > > > > > > if (!prog_type) > > > prog_type = BPF_PROG_TYPE_SOCKET_FILTER; > > > - do_test_fixup(test, prog_type, prog, map_fds); > > > + if (do_test_fixup(test, prog_type, prog, map_fds) < 0) > > > + return; > > > prog_len = probe_filter_length(prog); > > > > > > pflags = 0; > > > > >
On 12/20/2018 09:51 PM, Stanislav Fomichev wrote: > On Tue, Dec 18, 2018 at 4:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote: >> On 12/19, Daniel Borkmann wrote: >>> On 12/17/2018 07:25 PM, Stanislav Fomichev wrote: >>>> Use recently introduced bpf_map_type_supported() to skip tests in the >>>> test_verifier if map creation (create_map) fails. It's handled >>>> explicitly for each fixup, i.e. if bpf_create_map returns negative fd, >>>> we probe the kernel for the appropriate map support and skip the >>>> test is map type is not supported. >>>> >>>> Signed-off-by: Stanislav Fomichev <sdf@google.com> >>>> --- >>>> tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++-- >>>> 1 file changed, 31 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c >>>> index 124d21306c27..d267f5248b5d 100644 >>>> --- a/tools/testing/selftests/bpf/test_verifier.c >>>> +++ b/tools/testing/selftests/bpf/test_verifier.c >>>> @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu) >>>> return fd; >>>> } >>>> >>>> +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type) >>>> +{ >>>> + if (ret < 0 && !bpf_map_type_supported(map_type)) { >>>> + printf("SKIP (unsupported map type %d)\n", map_type); >>>> + skips++; >>>> + return true; >>>> + } >>>> + return false; >>>> +} >>>> + >>>> static char bpf_vlog[UINT_MAX >> 8]; >>>> >>>> -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> - struct bpf_insn *prog, int *map_fds) >>>> +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> + struct bpf_insn *prog, int *map_fds) >>>> { >>>> int *fixup_map_hash_8b = test->fixup_map_hash_8b; >>>> int *fixup_map_hash_48b = test->fixup_map_hash_48b; >>>> @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> >>>> if (*fixup_cgroup_storage) { >>>> map_fds[7] = create_cgroup_storage(false); >>>> + if (skip_unsupported_map(map_fds[7], >>>> + BPF_MAP_TYPE_CGROUP_STORAGE)) >>>> + return -1; >>>> do { >>>> prog[*fixup_cgroup_storage].imm = map_fds[7]; >>>> fixup_cgroup_storage++; >>>> @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> >>>> if (*fixup_percpu_cgroup_storage) { >>>> map_fds[8] = create_cgroup_storage(true); >>>> + if (skip_unsupported_map(map_fds[8], >>>> + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)) >>>> + return -1; >>>> do { >>>> prog[*fixup_percpu_cgroup_storage].imm = map_fds[8]; >>>> fixup_percpu_cgroup_storage++; >>>> @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> if (*fixup_map_sockmap) { >>>> map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int), >>>> sizeof(int), 1); >>>> + if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP)) >>>> + return -1; >>>> do { >>>> prog[*fixup_map_sockmap].imm = map_fds[9]; >>>> fixup_map_sockmap++; >>>> @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> if (*fixup_map_sockhash) { >>>> map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int), >>>> sizeof(int), 1); >>>> + if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH)) >>>> + return -1; >>>> do { >>>> prog[*fixup_map_sockhash].imm = map_fds[10]; >>>> fixup_map_sockhash++; >>>> @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> if (*fixup_map_xskmap) { >>>> map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int), >>>> sizeof(int), 1); >>>> + if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP)) >>>> + return -1; >>>> do { >>>> prog[*fixup_map_xskmap].imm = map_fds[11]; >>>> fixup_map_xskmap++; >>>> @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, >>>> if (*fixup_map_stacktrace) { >>>> map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32), >>>> sizeof(u64), 1); >>>> + if (skip_unsupported_map(map_fds[12], >>>> + BPF_MAP_TYPE_STACK_TRACE)) >>>> + return -1; >>> >>> Nit: Could probably be slightly simplified by moving this into and by reworking >>> create_{map,cgroup_storage}() a bit. >> Yeah, I stated that option in the cover letter. I did that initially, >> but it required some additional argument (skip/supported) to the >> create_{map,cgroup_storage} and I scrapped this approach due to too >> much plumbing. >> >> But I think since we are not doing any parallel tests in the verifier, >> we can do something like the following patch below. WDYT? > Daniel, should this go as is or you'd like me to respin to the version > from my last reply (or something similar)? Your diff on top of the set looks good to me. My preference though is that we get both your work and Quentin's merged in the next cycle (given we're about to close bpf-next) and have plenty of time to consolidate the two and get it into good shape in order to then move the logic into libbpf as next step such that bpftool /and/ kselftests can make use of it. Thanks a lot, Daniel
On 12/20, Daniel Borkmann wrote: > On 12/20/2018 09:51 PM, Stanislav Fomichev wrote: > > On Tue, Dec 18, 2018 at 4:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > >> On 12/19, Daniel Borkmann wrote: > >>> On 12/17/2018 07:25 PM, Stanislav Fomichev wrote: > >>>> Use recently introduced bpf_map_type_supported() to skip tests in the > >>>> test_verifier if map creation (create_map) fails. It's handled > >>>> explicitly for each fixup, i.e. if bpf_create_map returns negative fd, > >>>> we probe the kernel for the appropriate map support and skip the > >>>> test is map type is not supported. > >>>> > >>>> Signed-off-by: Stanislav Fomichev <sdf@google.com> > >>>> --- > >>>> tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++-- > >>>> 1 file changed, 31 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > >>>> index 124d21306c27..d267f5248b5d 100644 > >>>> --- a/tools/testing/selftests/bpf/test_verifier.c > >>>> +++ b/tools/testing/selftests/bpf/test_verifier.c > >>>> @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu) > >>>> return fd; > >>>> } > >>>> > >>>> +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type) > >>>> +{ > >>>> + if (ret < 0 && !bpf_map_type_supported(map_type)) { > >>>> + printf("SKIP (unsupported map type %d)\n", map_type); > >>>> + skips++; > >>>> + return true; > >>>> + } > >>>> + return false; > >>>> +} > >>>> + > >>>> static char bpf_vlog[UINT_MAX >> 8]; > >>>> > >>>> -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> - struct bpf_insn *prog, int *map_fds) > >>>> +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> + struct bpf_insn *prog, int *map_fds) > >>>> { > >>>> int *fixup_map_hash_8b = test->fixup_map_hash_8b; > >>>> int *fixup_map_hash_48b = test->fixup_map_hash_48b; > >>>> @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> > >>>> if (*fixup_cgroup_storage) { > >>>> map_fds[7] = create_cgroup_storage(false); > >>>> + if (skip_unsupported_map(map_fds[7], > >>>> + BPF_MAP_TYPE_CGROUP_STORAGE)) > >>>> + return -1; > >>>> do { > >>>> prog[*fixup_cgroup_storage].imm = map_fds[7]; > >>>> fixup_cgroup_storage++; > >>>> @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> > >>>> if (*fixup_percpu_cgroup_storage) { > >>>> map_fds[8] = create_cgroup_storage(true); > >>>> + if (skip_unsupported_map(map_fds[8], > >>>> + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)) > >>>> + return -1; > >>>> do { > >>>> prog[*fixup_percpu_cgroup_storage].imm = map_fds[8]; > >>>> fixup_percpu_cgroup_storage++; > >>>> @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> if (*fixup_map_sockmap) { > >>>> map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int), > >>>> sizeof(int), 1); > >>>> + if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP)) > >>>> + return -1; > >>>> do { > >>>> prog[*fixup_map_sockmap].imm = map_fds[9]; > >>>> fixup_map_sockmap++; > >>>> @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> if (*fixup_map_sockhash) { > >>>> map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int), > >>>> sizeof(int), 1); > >>>> + if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH)) > >>>> + return -1; > >>>> do { > >>>> prog[*fixup_map_sockhash].imm = map_fds[10]; > >>>> fixup_map_sockhash++; > >>>> @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> if (*fixup_map_xskmap) { > >>>> map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int), > >>>> sizeof(int), 1); > >>>> + if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP)) > >>>> + return -1; > >>>> do { > >>>> prog[*fixup_map_xskmap].imm = map_fds[11]; > >>>> fixup_map_xskmap++; > >>>> @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, > >>>> if (*fixup_map_stacktrace) { > >>>> map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32), > >>>> sizeof(u64), 1); > >>>> + if (skip_unsupported_map(map_fds[12], > >>>> + BPF_MAP_TYPE_STACK_TRACE)) > >>>> + return -1; > >>> > >>> Nit: Could probably be slightly simplified by moving this into and by reworking > >>> create_{map,cgroup_storage}() a bit. > >> Yeah, I stated that option in the cover letter. I did that initially, > >> but it required some additional argument (skip/supported) to the > >> create_{map,cgroup_storage} and I scrapped this approach due to too > >> much plumbing. > >> > >> But I think since we are not doing any parallel tests in the verifier, > >> we can do something like the following patch below. WDYT? > > Daniel, should this go as is or you'd like me to respin to the version > > from my last reply (or something similar)? > > Your diff on top of the set looks good to me. My preference though is that > we get both your work and Quentin's merged in the next cycle (given we're > about to close bpf-next) and have plenty of time to consolidate the two and > get it into good shape in order to then move the logic into libbpf as next > step such that bpftool /and/ kselftests can make use of it. Ack, I'll use that for a v3 submission when bpf-next opens again. Maybe I can even drop my custom probes if Quentin's patch set is ready by that point. > > Thanks a lot, > Daniel
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 124d21306c27..d267f5248b5d 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -14221,10 +14221,20 @@ static int create_cgroup_storage(bool percpu) return fd; } +static bool skip_unsupported_map(int ret, enum bpf_map_type map_type) +{ + if (ret < 0 && !bpf_map_type_supported(map_type)) { + printf("SKIP (unsupported map type %d)\n", map_type); + skips++; + return true; + } + return false; +} + static char bpf_vlog[UINT_MAX >> 8]; -static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, - struct bpf_insn *prog, int *map_fds) +static int do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, + struct bpf_insn *prog, int *map_fds) { int *fixup_map_hash_8b = test->fixup_map_hash_8b; int *fixup_map_hash_48b = test->fixup_map_hash_48b; @@ -14309,6 +14319,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, if (*fixup_cgroup_storage) { map_fds[7] = create_cgroup_storage(false); + if (skip_unsupported_map(map_fds[7], + BPF_MAP_TYPE_CGROUP_STORAGE)) + return -1; do { prog[*fixup_cgroup_storage].imm = map_fds[7]; fixup_cgroup_storage++; @@ -14317,6 +14330,9 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, if (*fixup_percpu_cgroup_storage) { map_fds[8] = create_cgroup_storage(true); + if (skip_unsupported_map(map_fds[8], + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)) + return -1; do { prog[*fixup_percpu_cgroup_storage].imm = map_fds[8]; fixup_percpu_cgroup_storage++; @@ -14325,6 +14341,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, if (*fixup_map_sockmap) { map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int), sizeof(int), 1); + if (skip_unsupported_map(map_fds[9], BPF_MAP_TYPE_SOCKMAP)) + return -1; do { prog[*fixup_map_sockmap].imm = map_fds[9]; fixup_map_sockmap++; @@ -14333,6 +14351,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, if (*fixup_map_sockhash) { map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int), sizeof(int), 1); + if (skip_unsupported_map(map_fds[10], BPF_MAP_TYPE_SOCKHASH)) + return -1; do { prog[*fixup_map_sockhash].imm = map_fds[10]; fixup_map_sockhash++; @@ -14341,6 +14361,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, if (*fixup_map_xskmap) { map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int), sizeof(int), 1); + if (skip_unsupported_map(map_fds[11], BPF_MAP_TYPE_XSKMAP)) + return -1; do { prog[*fixup_map_xskmap].imm = map_fds[11]; fixup_map_xskmap++; @@ -14349,11 +14371,16 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, if (*fixup_map_stacktrace) { map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32), sizeof(u64), 1); + if (skip_unsupported_map(map_fds[12], + BPF_MAP_TYPE_STACK_TRACE)) + return -1; do { prog[*fixup_map_stacktrace].imm = map_fds[12]; fixup_map_stacktrace++; } while (fixup_map_stacktrace); } + + return 0; } static int set_admin(bool admin) @@ -14401,7 +14428,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, if (!prog_type) prog_type = BPF_PROG_TYPE_SOCKET_FILTER; - do_test_fixup(test, prog_type, prog, map_fds); + if (do_test_fixup(test, prog_type, prog, map_fds) < 0) + return; prog_len = probe_filter_length(prog); pflags = 0;
Use recently introduced bpf_map_type_supported() to skip tests in the test_verifier if map creation (create_map) fails. It's handled explicitly for each fixup, i.e. if bpf_create_map returns negative fd, we probe the kernel for the appropriate map support and skip the test is map type is not supported. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/testing/selftests/bpf/test_verifier.c | 34 +++++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-)