netdev - Re: [PATCH bpf-next v9 5/5] bpf: Allow the new syncookie helpers to work with SKBs

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: <CAEf4BzZRHsW=e40+ZD7GAnUr+03GroouxpF82zO7GoBjrGBB7A@mail.gmail.com>
Date:   Fri, 6 May 2022 14:39:06 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Maxim Mikityanskiy <maximmi@...dia.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Joe Stringer <joe@...ium.io>,
        Florent Revest <revest@...omium.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Florian Westphal <fw@...len.de>, pabeni@...hat.com
Subject: Re: [PATCH bpf-next v9 5/5] bpf: Allow the new syncookie helpers to
 work with SKBs

On Tue, May 3, 2022 at 10:15 AM Maxim Mikityanskiy <maximmi@...dia.com> wrote:
>
> This commits allows the new BPF helpers to work in SKB context (in TC
> BPF programs): bpf_tcp_raw_{gen,check}_syncookie_ipv{4,6}.
>
> The sample application and selftest are updated to support the TC mode.
> It's not the recommended mode of operation, because the SKB is already
> created at this point, and it's unlikely that the BPF program will
> provide any substantional speedup compared to regular SYN cookies or
> synproxy.
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@...dia.com>
> Reviewed-by: Tariq Toukan <tariqt@...dia.com>
> ---
>  net/core/filter.c                             |  10 ++
>  .../selftests/bpf/prog_tests/xdp_synproxy.c   |  53 +++++--
>  .../selftests/bpf/progs/xdp_synproxy_kern.c   | 141 +++++++++++++-----
>  tools/testing/selftests/bpf/xdp_synproxy.c    |  94 +++++++++---

please split selftests and kernel code into separate patches (and use
selftests/bpf: prefix for selftests)

>  4 files changed, 230 insertions(+), 68 deletions(-)
>

[...]

> @@ -87,7 +112,11 @@ void test_xdp_synproxy(void)
>         if (!ASSERT_OK_PTR(ns, "setns"))
>                 goto out;
>
> -       ctrl_file = SYS_OUT("./xdp_synproxy --iface tmp1 --single");
> +       if (xdp)
> +               ctrl_file = SYS_OUT("./xdp_synproxy --iface tmp1 --single");
> +       else
> +               ctrl_file = SYS_OUT("./xdp_synproxy --prog %s --single",
> +                                   prog_id);
>         size = fread(buf, 1, sizeof(buf), ctrl_file);
>         pclose(ctrl_file);
>         if (!ASSERT_TRUE(expect_str(buf, size, "Total SYNACKs generated: 1\n"),
> @@ -107,3 +136,9 @@ void test_xdp_synproxy(void)
>         system("ip link del tmp0");
>         system("ip netns del synproxy");
>  }
> +
> +void test_xdp_synproxy(void)
> +{
> +       test_synproxy(true);
> +       test_synproxy(false);

let's model this as subtests instead? See test__start_subtest() use in
other selftests

> +}
> diff --git a/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c b/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
> index 9ae85b189072..f70b5f776dcf 100644
> --- a/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
> +++ b/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
> @@ -7,6 +7,9 @@
>  #include <bpf/bpf_endian.h>
>  #include <asm/errno.h>
>

[...]

> @@ -201,21 +220,50 @@ static int syncookie_attach(const char *argv0, unsigned int ifindex)
>                 fprintf(stderr, "Error: bpf_obj_get_info_by_fd: %s\n", strerror(-err));
>                 goto out;
>         }
> +       attached_tc = tc;
>         attached_prog_id = info.id;
>         signal(SIGINT, cleanup);
>         signal(SIGTERM, cleanup);
> -       err = bpf_xdp_attach(ifindex, prog_fd, XDP_FLAGS_UPDATE_IF_NOEXIST, NULL);
> -       if (err < 0) {
> -               fprintf(stderr, "Error: bpf_set_link_xdp_fd: %s\n", strerror(-err));
> -               signal(SIGINT, SIG_DFL);
> -               signal(SIGTERM, SIG_DFL);
> -               attached_prog_id = 0;
> -               goto out;
> +       if (tc) {
> +               DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook,

nit: LIBBPF_OPTS is shorter, DECLARE_LIBBPF_OPTS is discouraged

> +                                   .ifindex = ifindex,
> +                                   .attach_point = BPF_TC_INGRESS);
> +               DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts,
> +                                   .handle = 1,
> +                                   .priority = 1,
> +                                   .prog_fd = prog_fd);
> +

[...]