Re: [PATCH bpf-next v2 4/6] selftests/bpf: skip verifier tests for unsupported map types

[Daniel Borkmann <daniel@iogearbox.net>]

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