linux-kernel - Re: [PATCH bpf-next 6/6] selftests: bpf: test sockmap update from BPF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Aug 2020 07:49:14 -0700
From:   Yonghong Song <yhs@...com>
To:     Lorenz Bauer <lmb@...udflare.com>
CC:     Jakub Sitnicki <jakub@...udflare.com>,
        John Fastabend <john.fastabend@...il.com>,
        Shuah Khan <shuah@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        kernel-team <kernel-team@...udflare.com>,
        LKML <linux-kernel@...r.kernel.org>,
        <linux-kselftest@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 6/6] selftests: bpf: test sockmap update from BPF



On 8/20/20 4:58 AM, Lorenz Bauer wrote:
> On Wed, 19 Aug 2020 at 21:46, Yonghong Song <yhs@...com> wrote:
>>
>>
>>
>> On 8/19/20 2:24 AM, Lorenz Bauer wrote:
>>> Add a test which copies a socket from a sockmap into another sockmap
>>> or sockhash. This excercises bpf_map_update_elem support from BPF
>>> context. Compare the socket cookies from source and destination to
>>> ensure that the copy succeeded.
>>>
>>> Signed-off-by: Lorenz Bauer <lmb@...udflare.com>
>>> ---
>>>    .../selftests/bpf/prog_tests/sockmap_basic.c  | 76 +++++++++++++++++++
>>>    .../selftests/bpf/progs/test_sockmap_copy.c   | 48 ++++++++++++
>>>    2 files changed, 124 insertions(+)
>>>    create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_copy.c
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>>> index 96e7b7f84c65..d30cabc00e9e 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>>> @@ -4,6 +4,7 @@
>>>
>>>    #include "test_progs.h"
>>>    #include "test_skmsg_load_helpers.skel.h"
>>> +#include "test_sockmap_copy.skel.h"
>>>
>>>    #define TCP_REPAIR          19      /* TCP sock is under repair right now */
>>>
>>> @@ -101,6 +102,77 @@ static void test_skmsg_helpers(enum bpf_map_type map_type)
>>>        test_skmsg_load_helpers__destroy(skel);
>>>    }
>>>
>>> +static void test_sockmap_copy(enum bpf_map_type map_type)
>>> +{
>>> +     struct bpf_prog_test_run_attr attr;
>>> +     struct test_sockmap_copy *skel;
>>> +     __u64 src_cookie, dst_cookie;
>>> +     int err, prog, s, src, dst;
>>> +     const __u32 zero = 0;
>>> +     char dummy[14] = {0};
>>> +
>>> +     s = connected_socket_v4();
>>
>> Maybe change variable name to "sk" for better clarity?
> 
> Yup!
> 
>>
>>> +     if (CHECK_FAIL(s == -1))
>>> +             return;
>>> +
>>> +     skel = test_sockmap_copy__open_and_load();
>>> +     if (CHECK_FAIL(!skel)) {
>>> +             close(s);
>>> +             perror("test_sockmap_copy__open_and_load");
>>> +             return;
>>> +     }
>>
>> Could you use CHECK instead of CHECK_FAIL?
>> With CHECK, you can print additional information without perror.
> 
> I avoid CHECK because it requires `duration`, which doesn't make sense
> for most things that I call CHECK_FAIL on here. So either it outputs 0
> nsec (which is bogus) or it outputs the value from the last
> bpf_prog_test_run call (which is also bogus). How do other tests
> handle this? Just ignore it?

Just ignore it. You can define a static variable duration in the 
beginning of file and then use CHECK in the rest of file.

> 
>>
>>
>>> +
>>> +     prog = bpf_program__fd(skel->progs.copy_sock_map);
>>> +     src = bpf_map__fd(skel->maps.src);
>>> +     if (map_type == BPF_MAP_TYPE_SOCKMAP)
>>> +             dst = bpf_map__fd(skel->maps.dst_sock_map);
>>> +     else
>>> +             dst = bpf_map__fd(skel->maps.dst_sock_hash);
>>> +
[...]