netdev - Re: [PATCH bpf-next v2 4/5] bpf: selftests: add MPTCP test base

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]

Message-ID: <CAPhsuW5Gbx2pWgM1XcSYqVsN6L=q+0u3QFNxG7A+Qez=Tziu2A@mail.gmail.com>
Date:   Mon, 14 Sep 2020 11:07:30 -0700
From:   Song Liu <song@...nel.org>
To:     Nicolas Rybowski <nicolas.rybowski@...sares.net>
Cc:     Shuah Khan <shuah@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Matthieu Baerts <matthieu.baerts@...sares.net>,
        open list <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 v2 4/5] bpf: selftests: add MPTCP test base

On Fri, Sep 11, 2020 at 8:02 AM Nicolas Rybowski
<nicolas.rybowski@...sares.net> wrote:
>
> This patch adds a base for MPTCP specific tests.
>
> It is currently limited to the is_mptcp field in case of plain TCP
> connection because for the moment there is no easy way to get the subflow
> sk from a msk in userspace. This implies that we cannot lookup the
> sk_storage attached to the subflow sk in the sockops program.
>
> Acked-by: Matthieu Baerts <matthieu.baerts@...sares.net>
> Signed-off-by: Nicolas Rybowski <nicolas.rybowski@...sares.net>

Acked-by: Song Liu <songliubraving@...com>

With some nitpicks below.

> ---
>
> Notes:
>     v1 -> v2:
>     - new patch: mandatory selftests (Alexei)
>
[...]
>                      int timeout_ms);
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> new file mode 100644
> index 000000000000..0e65d64868e9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include "cgroup_helpers.h"
> +#include "network_helpers.h"
> +
> +struct mptcp_storage {
> +       __u32 invoked;
> +       __u32 is_mptcp;
> +};
> +
> +static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
> +{
> +       int err = 0, cfd = client_fd;
> +       struct mptcp_storage val;
> +
> +       /* Currently there is no easy way to get back the subflow sk from the MPTCP
> +        * sk, thus we cannot access here the sk_storage associated to the subflow
> +        * sk. Also, there is no sk_storage associated with the MPTCP sk since it
> +        * does not trigger sockops events.
> +        * We silently pass this situation at the moment.
> +        */
> +       if (is_mptcp == 1)
> +               return 0;
> +
> +       if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
> +               perror("Failed to read socket storage");

Maybe simplify this with CHECK(), which contains a customized error message?
Same for some other calls.

> +               return -1;
> +       }
> +
> +       if (val.invoked != 1) {
> +               log_err("%s: unexpected invoked count %d != %d",
> +                       msg, val.invoked, 1);
> +               err++;
> +       }
> +
> +       if (val.is_mptcp != is_mptcp) {
> +               log_err("%s: unexpected bpf_tcp_sock.is_mptcp %d != %d",
> +                       msg, val.is_mptcp, is_mptcp);
> +               err++;
> +       }
> +
> +       return err;
> +}
> +
> +static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
[...]

> +
> +       client_fd = is_mptcp ? connect_to_mptcp_fd(server_fd, 0) :
> +                              connect_to_fd(server_fd, 0);
> +       if (client_fd < 0) {
> +               err = -1;
> +               goto close_client_fd;

This should be "goto close_bpf_object;", and we don't really need the label
close_client_fd.

> +       }
> +
> +       err += is_mptcp ? verify_sk(map_fd, client_fd, "MPTCP subflow socket", 1) :

It doesn't really change the logic, but I guess we only need "err = xxx"?

> +                         verify_sk(map_fd, client_fd, "plain TCP socket", 0);
> +
> +close_client_fd:
> +       close(client_fd);
> +
> +close_bpf_object:
> +       bpf_object__close(obj);
> +       return err;
> +}
> +