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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, May 3, 2022 at 10:15 AM Maxim Mikityanskiy <maximmi@nvidia.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@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.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);
> +

[...]