From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stanislav Fomichev Subject: Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types Date: Thu, 20 Dec 2018 14:51:46 -0800 Message-ID: <20181220225146.GA2959@mini-arch.hsd1.ca.comcast.net> References: <20181217182554.52170-1-sdf@google.com> <20181217182554.52170-5-sdf@google.com> <20181219000240.GF20955@mini-arch.hsd1.ca.comcast.net> <5f82a125-03ee-dddb-4a29-a29f08dcc394@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Stanislav Fomichev , netdev@vger.kernel.org, ast@kernel.org, davem@davemloft.net, ecree@solarflare.com, quentin.monnet@netronome.com To: Daniel Borkmann Return-path: Received: from mail-pl1-f193.google.com ([209.85.214.193]:42397 "EHLO mail-pl1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730086AbeLTWvt (ORCPT ); Thu, 20 Dec 2018 17:51:49 -0500 Received: by mail-pl1-f193.google.com with SMTP id y1so1529244plp.9 for ; Thu, 20 Dec 2018 14:51:49 -0800 (PST) Content-Disposition: inline In-Reply-To: <5f82a125-03ee-dddb-4a29-a29f08dcc394@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: 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 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 > >>>> --- > >>>> 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