* [PATCH bpf-next 1/6] selftests/bpf: factorize conn and syncookies tests in a single runner
2024-10-16 18:35 [PATCH bpf-next 0/6] selftests/bpf: integrate test_tcp_check_syncookie.sh into test_progs Alexis Lothoré (eBPF Foundation)
@ 2024-10-16 18:35 ` Alexis Lothoré (eBPF Foundation)
2024-10-16 18:35 ` [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in btf_skc_cls_ingress Alexis Lothoré (eBPF Foundation)
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Alexis Lothoré (eBPF Foundation) @ 2024-10-16 18:35 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer
Cc: ebpf, Thomas Petazzoni, Lorenz Bauer, bpf, linux-kselftest,
linux-kernel, netdev, Alexis Lothoré (eBPF Foundation)
btf_skc_cls_ingress currently describe two tests, both running a simple
tcp server and then initializing a connection to it. The sole difference
between the tests is about the tcp_syncookie configuration, and some
checks around this feature being enabled/disabled.
Share the common code between those two tests by moving the code into a
single runner, parameterized by a "gen_cookies" argument. Split the
performed checks accordingly.
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
.../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 106 +++++++--------------
1 file changed, 37 insertions(+), 69 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
index ef4d6a3ae4231b250f43f8f93eeca6499abeaadc..5d8d7736edc095b647ca3fbc12cac0440b60140e 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
@@ -71,13 +71,14 @@ static void print_err_line(void)
printf("bpf prog error at line %u\n", skel->bss->linum);
}
-static void test_conn(void)
+static void run_test(bool gen_cookies)
{
+ const char *tcp_syncookies = gen_cookies ? "2" : "1";
int listen_fd = -1, cli_fd = -1, srv_fd = -1, err;
socklen_t addrlen = sizeof(srv_sa6);
int srv_port;
- if (write_sysctl("/proc/sys/net/ipv4/tcp_syncookies", "1"))
+ if (write_sysctl("/proc/sys/net/ipv4/tcp_syncookies", tcp_syncookies))
return;
listen_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
@@ -99,19 +100,36 @@ static void test_conn(void)
if (CHECK_FAIL(srv_fd == -1))
goto done;
- if (CHECK(skel->bss->listen_tp_sport != srv_port ||
- skel->bss->req_sk_sport != srv_port,
- "Unexpected sk src port",
- "listen_tp_sport:%u req_sk_sport:%u expected:%u\n",
- skel->bss->listen_tp_sport, skel->bss->req_sk_sport,
- srv_port))
+ if (CHECK(skel->bss->listen_tp_sport != srv_port,
+ "Unexpected listen tp src port",
+ "listen_tp_sport:%u expected:%u\n",
+ skel->bss->listen_tp_sport, srv_port))
goto done;
- if (CHECK(skel->bss->gen_cookie || skel->bss->recv_cookie,
- "Unexpected syncookie states",
- "gen_cookie:%u recv_cookie:%u\n",
- skel->bss->gen_cookie, skel->bss->recv_cookie))
- goto done;
+ if (!gen_cookies) {
+ if (CHECK(skel->bss->req_sk_sport != srv_port,
+ "Unexpected req_sk src port",
+ "req_sk_sport:%u expected:%u\n",
+ skel->bss->req_sk_sport, srv_port))
+ goto done;
+ if (CHECK(skel->bss->gen_cookie || skel->bss->recv_cookie,
+ "Unexpected syncookie states",
+ "gen_cookie:%u recv_cookie:%u\n",
+ skel->bss->gen_cookie, skel->bss->recv_cookie))
+ goto done;
+ } else {
+ if (CHECK(skel->bss->req_sk_sport,
+ "Unexpected req_sk src port",
+ "req_sk_sport:%u expected:0\n",
+ skel->bss->req_sk_sport))
+ goto done;
+ if (CHECK(!skel->bss->gen_cookie ||
+ skel->bss->gen_cookie != skel->bss->recv_cookie,
+ "Unexpected syncookie states",
+ "gen_cookie:%u recv_cookie:%u\n",
+ skel->bss->gen_cookie, skel->bss->recv_cookie))
+ goto done;
+ }
CHECK(skel->bss->linum, "bpf prog detected error", "at line %u\n",
skel->bss->linum);
@@ -125,64 +143,14 @@ static void test_conn(void)
close(srv_fd);
}
-static void test_syncookie(void)
+static void test_conn(void)
{
- int listen_fd = -1, cli_fd = -1, srv_fd = -1, err;
- socklen_t addrlen = sizeof(srv_sa6);
- int srv_port;
-
- /* Enforce syncookie mode */
- if (write_sysctl("/proc/sys/net/ipv4/tcp_syncookies", "2"))
- return;
-
- listen_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
- if (CHECK_FAIL(listen_fd == -1))
- return;
-
- err = getsockname(listen_fd, (struct sockaddr *)&srv_sa6, &addrlen);
- if (CHECK(err, "getsockname(listen_fd)", "err:%d errno:%d\n", err,
- errno))
- goto done;
- memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
- srv_port = ntohs(srv_sa6.sin6_port);
-
- cli_fd = connect_to_fd(listen_fd, 0);
- if (CHECK_FAIL(cli_fd == -1))
- goto done;
-
- srv_fd = accept(listen_fd, NULL, NULL);
- if (CHECK_FAIL(srv_fd == -1))
- goto done;
-
- if (CHECK(skel->bss->listen_tp_sport != srv_port,
- "Unexpected tp src port",
- "listen_tp_sport:%u expected:%u\n",
- skel->bss->listen_tp_sport, srv_port))
- goto done;
-
- if (CHECK(skel->bss->req_sk_sport,
- "Unexpected req_sk src port",
- "req_sk_sport:%u expected:0\n",
- skel->bss->req_sk_sport))
- goto done;
-
- if (CHECK(!skel->bss->gen_cookie ||
- skel->bss->gen_cookie != skel->bss->recv_cookie,
- "Unexpected syncookie states",
- "gen_cookie:%u recv_cookie:%u\n",
- skel->bss->gen_cookie, skel->bss->recv_cookie))
- goto done;
-
- CHECK(skel->bss->linum, "bpf prog detected error", "at line %u\n",
- skel->bss->linum);
+ run_test(false);
+}
-done:
- if (listen_fd != -1)
- close(listen_fd);
- if (cli_fd != -1)
- close(cli_fd);
- if (srv_fd != -1)
- close(srv_fd);
+static void test_syncookie(void)
+{
+ run_test(true);
}
struct test {
--
2.46.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in btf_skc_cls_ingress
2024-10-16 18:35 [PATCH bpf-next 0/6] selftests/bpf: integrate test_tcp_check_syncookie.sh into test_progs Alexis Lothoré (eBPF Foundation)
2024-10-16 18:35 ` [PATCH bpf-next 1/6] selftests/bpf: factorize conn and syncookies tests in a single runner Alexis Lothoré (eBPF Foundation)
@ 2024-10-16 18:35 ` Alexis Lothoré (eBPF Foundation)
2024-10-18 23:57 ` Martin KaFai Lau
2024-10-16 18:35 ` [PATCH bpf-next 3/6] selftests/bpf: get rid of global vars " Alexis Lothoré (eBPF Foundation)
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Alexis Lothoré (eBPF Foundation) @ 2024-10-16 18:35 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer
Cc: ebpf, Thomas Petazzoni, Lorenz Bauer, bpf, linux-kselftest,
linux-kernel, netdev, Alexis Lothoré (eBPF Foundation)
btf_skc_cls_ingress.c currently runs two subtests, and create a
dedicated network namespace for each, but never cleans up the created
namespace once the test has ended.
Add missing namespace cleanup after each namespace to avoid accumulating
namespaces for each new subtest. While at it, switch namespace
management to netns_{new,free}
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
.../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 31 ++++++++++++++--------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
index 5d8d7736edc095b647ca3fbc12cac0440b60140e..8d1fa8806cdda088d264b44104f7c80726b025e2 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
@@ -17,32 +17,34 @@
#include "test_progs.h"
#include "test_btf_skc_cls_ingress.skel.h"
+#define TEST_NS "skc_cls_ingress"
+
static struct test_btf_skc_cls_ingress *skel;
static struct sockaddr_in6 srv_sa6;
static __u32 duration;
-static int prepare_netns(void)
+static struct netns_obj *prepare_netns(void)
{
LIBBPF_OPTS(bpf_tc_hook, qdisc_lo, .attach_point = BPF_TC_INGRESS);
LIBBPF_OPTS(bpf_tc_opts, tc_attach,
.prog_fd = bpf_program__fd(skel->progs.cls_ingress));
+ struct netns_obj *ns = NULL;
- if (CHECK(unshare(CLONE_NEWNET), "create netns",
- "unshare(CLONE_NEWNET): %s (%d)",
- strerror(errno), errno))
- return -1;
+ ns = netns_new(TEST_NS, true);
+ if (!ASSERT_OK_PTR(ns, "create and join netns"))
+ return ns;
if (CHECK(system("ip link set dev lo up"),
"ip link set dev lo up", "failed\n"))
- return -1;
+ goto free_ns;
qdisc_lo.ifindex = if_nametoindex("lo");
if (!ASSERT_OK(bpf_tc_hook_create(&qdisc_lo), "qdisc add dev lo clsact"))
- return -1;
+ goto free_ns;
if (!ASSERT_OK(bpf_tc_attach(&qdisc_lo, &tc_attach),
"filter add dev lo ingress"))
- return -1;
+ goto free_ns;
/* Ensure 20 bytes options (i.e. in total 40 bytes tcp header) for the
* bpf_tcp_gen_syncookie() helper.
@@ -50,9 +52,13 @@ static int prepare_netns(void)
if (write_sysctl("/proc/sys/net/ipv4/tcp_window_scaling", "1") ||
write_sysctl("/proc/sys/net/ipv4/tcp_timestamps", "1") ||
write_sysctl("/proc/sys/net/ipv4/tcp_sack", "1"))
- return -1;
+ goto free_ns;
+
+ return ns;
- return 0;
+free_ns:
+ netns_free(ns);
+ return NULL;
}
static void reset_test(void)
@@ -169,6 +175,7 @@ void test_btf_skc_cls_ingress(void)
int i;
skel = test_btf_skc_cls_ingress__open_and_load();
+ struct netns_obj *ns;
if (CHECK(!skel, "test_btf_skc_cls_ingress__open_and_load", "failed\n"))
return;
@@ -176,13 +183,15 @@ void test_btf_skc_cls_ingress(void)
if (!test__start_subtest(tests[i].desc))
continue;
- if (prepare_netns())
+ ns = prepare_netns();
+ if (!ns)
break;
tests[i].run();
print_err_line();
reset_test();
+ netns_free(ns);
}
test_btf_skc_cls_ingress__destroy(skel);
--
2.46.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in btf_skc_cls_ingress
2024-10-16 18:35 ` [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in btf_skc_cls_ingress Alexis Lothoré (eBPF Foundation)
@ 2024-10-18 23:57 ` Martin KaFai Lau
2024-10-19 12:13 ` Alexis Lothoré
0 siblings, 1 reply; 12+ messages in thread
From: Martin KaFai Lau @ 2024-10-18 23:57 UTC (permalink / raw)
To: Alexis Lothoré (eBPF Foundation)
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer, ebpf, Thomas Petazzoni, Lorenz Bauer, bpf,
linux-kselftest, linux-kernel, netdev
On 10/16/24 11:35 AM, Alexis Lothoré (eBPF Foundation) wrote:
> btf_skc_cls_ingress.c currently runs two subtests, and create a
> dedicated network namespace for each, but never cleans up the created
> namespace once the test has ended.
>
> Add missing namespace cleanup after each namespace to avoid accumulating
> namespaces for each new subtest. While at it, switch namespace
> management to netns_{new,free}
>
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
> ---
> .../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 31 ++++++++++++++--------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> index 5d8d7736edc095b647ca3fbc12cac0440b60140e..8d1fa8806cdda088d264b44104f7c80726b025e2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> @@ -17,32 +17,34 @@
> #include "test_progs.h"
> #include "test_btf_skc_cls_ingress.skel.h"
>
> +#define TEST_NS "skc_cls_ingress"
> +
> static struct test_btf_skc_cls_ingress *skel;
> static struct sockaddr_in6 srv_sa6;
> static __u32 duration;
>
> -static int prepare_netns(void)
> +static struct netns_obj *prepare_netns(void)
> {
> LIBBPF_OPTS(bpf_tc_hook, qdisc_lo, .attach_point = BPF_TC_INGRESS);
> LIBBPF_OPTS(bpf_tc_opts, tc_attach,
> .prog_fd = bpf_program__fd(skel->progs.cls_ingress));
> + struct netns_obj *ns = NULL;
>
> - if (CHECK(unshare(CLONE_NEWNET), "create netns",
> - "unshare(CLONE_NEWNET): %s (%d)",
> - strerror(errno), errno))
> - return -1;
> + ns = netns_new(TEST_NS, true);
> + if (!ASSERT_OK_PTR(ns, "create and join netns"))
> + return ns;
>
> if (CHECK(system("ip link set dev lo up"),
> "ip link set dev lo up", "failed\n"))
nit. netns_new() takes care of "lo up" also, so the above can be removed.
test_progs.c has restore_netns() after each test, so the netns was not cleaned
up. The second unshare should have freed the earlier netns also.
Using netns_new() removed the boiler plate codes. It is nice to see this change
here regardless.
> - return -1;
> + goto free_ns;
>
> qdisc_lo.ifindex = if_nametoindex("lo");
> if (!ASSERT_OK(bpf_tc_hook_create(&qdisc_lo), "qdisc add dev lo clsact"))
> - return -1;
> + goto free_ns;
>
> if (!ASSERT_OK(bpf_tc_attach(&qdisc_lo, &tc_attach),
> "filter add dev lo ingress"))
> - return -1;
> + goto free_ns;
>
> /* Ensure 20 bytes options (i.e. in total 40 bytes tcp header) for the
> * bpf_tcp_gen_syncookie() helper.
> @@ -50,9 +52,13 @@ static int prepare_netns(void)
> if (write_sysctl("/proc/sys/net/ipv4/tcp_window_scaling", "1") ||
> write_sysctl("/proc/sys/net/ipv4/tcp_timestamps", "1") ||
> write_sysctl("/proc/sys/net/ipv4/tcp_sack", "1"))
> - return -1;
> + goto free_ns;
> +
> + return ns;
>
> - return 0;
> +free_ns:
> + netns_free(ns);
> + return NULL;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in btf_skc_cls_ingress
2024-10-18 23:57 ` Martin KaFai Lau
@ 2024-10-19 12:13 ` Alexis Lothoré
0 siblings, 0 replies; 12+ messages in thread
From: Alexis Lothoré @ 2024-10-19 12:13 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer, ebpf, Thomas Petazzoni, Lorenz Bauer, bpf,
linux-kselftest, linux-kernel, netdev
Hi Martin, thanks for the review !
On 10/19/24 01:57, Martin KaFai Lau wrote:
> On 10/16/24 11:35 AM, Alexis Lothoré (eBPF Foundation) wrote:
>> btf_skc_cls_ingress.c currently runs two subtests, and create a
>> dedicated network namespace for each, but never cleans up the created
>> namespace once the test has ended.
>>
>> Add missing namespace cleanup after each namespace to avoid accumulating
>> namespaces for each new subtest. While at it, switch namespace
>> management to netns_{new,free}
>>
>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
[...]
>> - if (CHECK(unshare(CLONE_NEWNET), "create netns",
>> - "unshare(CLONE_NEWNET): %s (%d)",
>> - strerror(errno), errno))
>> - return -1;
>> + ns = netns_new(TEST_NS, true);
>> + if (!ASSERT_OK_PTR(ns, "create and join netns"))
>> + return ns;
>> if (CHECK(system("ip link set dev lo up"),
>> "ip link set dev lo up", "failed\n"))
>
> nit. netns_new() takes care of "lo up" also, so the above can be removed.
Ah, indeed, I missed it in make_netns. Thanks, I'll remove this part from the
test then.
>
> test_progs.c has restore_netns() after each test, so the netns was not cleaned
> up. The second unshare should have freed the earlier netns also.
>
> Using netns_new() removed the boiler plate codes. It is nice to see this change
> here regardless.
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next 3/6] selftests/bpf: get rid of global vars in btf_skc_cls_ingress
2024-10-16 18:35 [PATCH bpf-next 0/6] selftests/bpf: integrate test_tcp_check_syncookie.sh into test_progs Alexis Lothoré (eBPF Foundation)
2024-10-16 18:35 ` [PATCH bpf-next 1/6] selftests/bpf: factorize conn and syncookies tests in a single runner Alexis Lothoré (eBPF Foundation)
2024-10-16 18:35 ` [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in btf_skc_cls_ingress Alexis Lothoré (eBPF Foundation)
@ 2024-10-16 18:35 ` Alexis Lothoré (eBPF Foundation)
2024-10-16 18:35 ` [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support " Alexis Lothoré (eBPF Foundation)
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Alexis Lothoré (eBPF Foundation) @ 2024-10-16 18:35 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer
Cc: ebpf, Thomas Petazzoni, Lorenz Bauer, bpf, linux-kselftest,
linux-kernel, netdev, Alexis Lothoré (eBPF Foundation)
There are a few global variables in btf_skc_cls_ingress.c, which are not
really used by different tests. Get rid of those global variables, by
performing the following updates:
- make srv_sa6 local to the main runner function
- make skel local to the main function, and propagate it through
function arguments
- get rid of duration by replacing CHECK macros with the ASSERT_XXX
macros. While updating those assert macros:
- do not return early on asserts performing some actual tests, let the
other tests run as well (keep the early return for parts handling
test setup)
- instead of converting the CHECK on skel->bss->linum, just remove it,
since there is already a call to print_err_line after the test to
print the failing line in the bpf program
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
.../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 91 +++++++++-------------
1 file changed, 35 insertions(+), 56 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
index 8d1fa8806cdda088d264b44104f7c80726b025e2..a20d104f9909e5ba20ddc4c107b910956f042fc1 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
@@ -19,11 +19,7 @@
#define TEST_NS "skc_cls_ingress"
-static struct test_btf_skc_cls_ingress *skel;
-static struct sockaddr_in6 srv_sa6;
-static __u32 duration;
-
-static struct netns_obj *prepare_netns(void)
+static struct netns_obj *prepare_netns(struct test_btf_skc_cls_ingress *skel)
{
LIBBPF_OPTS(bpf_tc_hook, qdisc_lo, .attach_point = BPF_TC_INGRESS);
LIBBPF_OPTS(bpf_tc_opts, tc_attach,
@@ -34,9 +30,7 @@ static struct netns_obj *prepare_netns(void)
if (!ASSERT_OK_PTR(ns, "create and join netns"))
return ns;
- if (CHECK(system("ip link set dev lo up"),
- "ip link set dev lo up", "failed\n"))
- goto free_ns;
+ SYS(free_ns, "ip link set dev lo up");
qdisc_lo.ifindex = if_nametoindex("lo");
if (!ASSERT_OK(bpf_tc_hook_create(&qdisc_lo), "qdisc add dev lo clsact"))
@@ -61,7 +55,7 @@ static struct netns_obj *prepare_netns(void)
return NULL;
}
-static void reset_test(void)
+static void reset_test(struct test_btf_skc_cls_ingress *skel)
{
memset(&skel->bss->srv_sa6, 0, sizeof(skel->bss->srv_sa6));
skel->bss->listen_tp_sport = 0;
@@ -71,16 +65,17 @@ static void reset_test(void)
skel->bss->linum = 0;
}
-static void print_err_line(void)
+static void print_err_line(struct test_btf_skc_cls_ingress *skel)
{
if (skel->bss->linum)
printf("bpf prog error at line %u\n", skel->bss->linum);
}
-static void run_test(bool gen_cookies)
+static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies)
{
const char *tcp_syncookies = gen_cookies ? "2" : "1";
int listen_fd = -1, cli_fd = -1, srv_fd = -1, err;
+ struct sockaddr_in6 srv_sa6;
socklen_t addrlen = sizeof(srv_sa6);
int srv_port;
@@ -88,58 +83,41 @@ static void run_test(bool gen_cookies)
return;
listen_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
- if (CHECK_FAIL(listen_fd == -1))
+ if (!ASSERT_OK_FD(listen_fd, "start server"))
return;
err = getsockname(listen_fd, (struct sockaddr *)&srv_sa6, &addrlen);
- if (CHECK(err, "getsockname(listen_fd)", "err:%d errno:%d\n", err,
- errno))
+ if (!ASSERT_OK(err, "getsockname(listen_fd)"))
goto done;
memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
srv_port = ntohs(srv_sa6.sin6_port);
cli_fd = connect_to_fd(listen_fd, 0);
- if (CHECK_FAIL(cli_fd == -1))
+ if (!ASSERT_OK_FD(cli_fd, "connect client"))
goto done;
srv_fd = accept(listen_fd, NULL, NULL);
- if (CHECK_FAIL(srv_fd == -1))
+ if (!ASSERT_OK_FD(srv_fd, "accept connection"))
goto done;
- if (CHECK(skel->bss->listen_tp_sport != srv_port,
- "Unexpected listen tp src port",
- "listen_tp_sport:%u expected:%u\n",
- skel->bss->listen_tp_sport, srv_port))
- goto done;
+ ASSERT_EQ(skel->bss->listen_tp_sport, srv_port, "listen tp src port");
if (!gen_cookies) {
- if (CHECK(skel->bss->req_sk_sport != srv_port,
- "Unexpected req_sk src port",
- "req_sk_sport:%u expected:%u\n",
- skel->bss->req_sk_sport, srv_port))
- goto done;
- if (CHECK(skel->bss->gen_cookie || skel->bss->recv_cookie,
- "Unexpected syncookie states",
- "gen_cookie:%u recv_cookie:%u\n",
- skel->bss->gen_cookie, skel->bss->recv_cookie))
- goto done;
+ ASSERT_EQ(skel->bss->req_sk_sport, srv_port,
+ "request socket source port with syncookies disabled");
+ ASSERT_EQ(skel->bss->gen_cookie, 0,
+ "generated syncookie with syncookies disabled");
+ ASSERT_EQ(skel->bss->recv_cookie, 0,
+ "received syncookie with syncookies disabled");
} else {
- if (CHECK(skel->bss->req_sk_sport,
- "Unexpected req_sk src port",
- "req_sk_sport:%u expected:0\n",
- skel->bss->req_sk_sport))
- goto done;
- if (CHECK(!skel->bss->gen_cookie ||
- skel->bss->gen_cookie != skel->bss->recv_cookie,
- "Unexpected syncookie states",
- "gen_cookie:%u recv_cookie:%u\n",
- skel->bss->gen_cookie, skel->bss->recv_cookie))
- goto done;
+ ASSERT_EQ(skel->bss->req_sk_sport, 0,
+ "request socket source port with syncookies enabled");
+ ASSERT_NEQ(skel->bss->gen_cookie, 0,
+ "syncookie properly generated");
+ ASSERT_EQ(skel->bss->gen_cookie, skel->bss->recv_cookie,
+ "matching syncookies on client and server");
}
- CHECK(skel->bss->linum, "bpf prog detected error", "at line %u\n",
- skel->bss->linum);
-
done:
if (listen_fd != -1)
close(listen_fd);
@@ -149,19 +127,19 @@ static void run_test(bool gen_cookies)
close(srv_fd);
}
-static void test_conn(void)
+static void test_conn(struct test_btf_skc_cls_ingress *skel)
{
- run_test(false);
+ run_test(skel, false);
}
-static void test_syncookie(void)
+static void test_syncookie(struct test_btf_skc_cls_ingress *skel)
{
- run_test(true);
+ run_test(skel, true);
}
struct test {
const char *desc;
- void (*run)(void);
+ void (*run)(struct test_btf_skc_cls_ingress *skel);
};
#define DEF_TEST(name) { #name, test_##name }
@@ -172,25 +150,26 @@ static struct test tests[] = {
void test_btf_skc_cls_ingress(void)
{
+ struct test_btf_skc_cls_ingress *skel;
+ struct netns_obj *ns;
int i;
skel = test_btf_skc_cls_ingress__open_and_load();
- struct netns_obj *ns;
- if (CHECK(!skel, "test_btf_skc_cls_ingress__open_and_load", "failed\n"))
+ if (!ASSERT_OK_PTR(skel, "test_btf_skc_cls_ingress__open_and_load"))
return;
for (i = 0; i < ARRAY_SIZE(tests); i++) {
if (!test__start_subtest(tests[i].desc))
continue;
- ns = prepare_netns();
+ ns = prepare_netns(skel);
if (!ns)
break;
- tests[i].run();
+ tests[i].run(skel);
- print_err_line();
- reset_test();
+ print_err_line(skel);
+ reset_test(skel);
netns_free(ns);
}
--
2.46.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support in btf_skc_cls_ingress
2024-10-16 18:35 [PATCH bpf-next 0/6] selftests/bpf: integrate test_tcp_check_syncookie.sh into test_progs Alexis Lothoré (eBPF Foundation)
` (2 preceding siblings ...)
2024-10-16 18:35 ` [PATCH bpf-next 3/6] selftests/bpf: get rid of global vars " Alexis Lothoré (eBPF Foundation)
@ 2024-10-16 18:35 ` Alexis Lothoré (eBPF Foundation)
2024-10-19 0:30 ` Martin KaFai Lau
2024-10-16 18:35 ` [PATCH bpf-next 5/6] selftests/bpf: test MSS value returned with bpf_tcp_gen_syncookie Alexis Lothoré (eBPF Foundation)
2024-10-16 18:35 ` [PATCH bpf-next 6/6] selftests/bpf: remove test_tcp_check_syncookie Alexis Lothoré (eBPF Foundation)
5 siblings, 1 reply; 12+ messages in thread
From: Alexis Lothoré (eBPF Foundation) @ 2024-10-16 18:35 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer
Cc: ebpf, Thomas Petazzoni, Lorenz Bauer, bpf, linux-kselftest,
linux-kernel, netdev, Alexis Lothoré (eBPF Foundation)
btf_skc_cls_ingress test currently checks that syncookie and
bpf_sk_assign/release helpers behave correctly in multiple scenarios,
but only with ipv4 socket.
Increase those helpers coverage by adding testing support for IPv6-only
sockets and IPv4/IPv6 sockets. The rework is mostly based on features
brought earlier in test_tcp_check_syncookie.sh to cover some fixes
performed on those helpers, but test_tcp_check_syncookie.sh is not
integrated in test_progs. The most notable changes linked to this are:
- some rework in the corresponding eBPF program to support both types of
traffic
- the switch from start_server to start_server_str to allow to check
some socket options
- the introduction of new subtests for ipv4 and ipv4/ipv6
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
The rework has been tested in a local Qemu environment and in CI:
# ./test_progs -a btf_skc_cls_ingress
#38/1 btf_skc_cls_ingress/conn_ipv4:OK
#38/2 btf_skc_cls_ingress/conn_ipv6:OK
#38/3 btf_skc_cls_ingress/conn_dual:OK
#38/4 btf_skc_cls_ingress/syncookie_ipv4:OK
#38/5 btf_skc_cls_ingress/syncookie_ipv6:OK
#38/6 btf_skc_cls_ingress/syncookie_dual:OK
#38 btf_skc_cls_ingress:OK
Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED
---
.../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 116 ++++++++++++++++++---
.../selftests/bpf/progs/test_btf_skc_cls_ingress.c | 81 +++++++++-----
2 files changed, 161 insertions(+), 36 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
index a20d104f9909e5ba20ddc4c107b910956f042fc1..e0f8fe818f4230a1d5bf0118133c5a9fb50345e1 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
@@ -19,6 +19,15 @@
#define TEST_NS "skc_cls_ingress"
+#define BIT(n) (1 << (n))
+#define TEST_MODE_IPV4 BIT(0)
+#define TEST_MODE_IPV6 BIT(1)
+#define TEST_MODE_DUAL (TEST_MODE_IPV4 | TEST_MODE_IPV6)
+
+#define SERVER_ADDR_IPV4 "127.0.0.1"
+#define SERVER_ADDR_IPV6 "::1"
+#define SERVER_ADDR_DUAL "::0"
+
static struct netns_obj *prepare_netns(struct test_btf_skc_cls_ingress *skel)
{
LIBBPF_OPTS(bpf_tc_hook, qdisc_lo, .attach_point = BPF_TC_INGRESS);
@@ -57,6 +66,7 @@ static struct netns_obj *prepare_netns(struct test_btf_skc_cls_ingress *skel)
static void reset_test(struct test_btf_skc_cls_ingress *skel)
{
+ memset(&skel->bss->srv_sa4, 0, sizeof(skel->bss->srv_sa4));
memset(&skel->bss->srv_sa6, 0, sizeof(skel->bss->srv_sa6));
skel->bss->listen_tp_sport = 0;
skel->bss->req_sk_sport = 0;
@@ -71,26 +81,84 @@ static void print_err_line(struct test_btf_skc_cls_ingress *skel)
printf("bpf prog error at line %u\n", skel->bss->linum);
}
-static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies)
+static int v6only_true(int fd, void *opts)
+{
+ int mode = true;
+
+ return setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &mode, sizeof(mode));
+}
+
+static int v6only_false(int fd, void *opts)
+{
+ int mode = false;
+
+ return setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &mode, sizeof(mode));
+}
+
+static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies,
+ int ip_mode)
{
const char *tcp_syncookies = gen_cookies ? "2" : "1";
int listen_fd = -1, cli_fd = -1, srv_fd = -1, err;
+ struct network_helper_opts opts = { 0 };
+ struct sockaddr_storage *addr;
struct sockaddr_in6 srv_sa6;
- socklen_t addrlen = sizeof(srv_sa6);
+ struct sockaddr_in srv_sa4;
+ socklen_t addr_len;
+ int sock_family;
+ char *srv_addr;
int srv_port;
+ switch (ip_mode) {
+ case TEST_MODE_IPV4:
+ sock_family = AF_INET;
+ srv_addr = SERVER_ADDR_IPV4;
+ addr = (struct sockaddr_storage *)&srv_sa4;
+ addr_len = sizeof(srv_sa4);
+ break;
+ case TEST_MODE_IPV6:
+ opts.post_socket_cb = v6only_true;
+ sock_family = AF_INET6;
+ srv_addr = SERVER_ADDR_IPV6;
+ addr = (struct sockaddr_storage *)&srv_sa6;
+ addr_len = sizeof(srv_sa6);
+ break;
+ case TEST_MODE_DUAL:
+ opts.post_socket_cb = v6only_false;
+ sock_family = AF_INET6;
+ srv_addr = SERVER_ADDR_DUAL;
+ addr = (struct sockaddr_storage *)&srv_sa6;
+ addr_len = sizeof(srv_sa6);
+ break;
+ default:
+ break;
+ }
+
if (write_sysctl("/proc/sys/net/ipv4/tcp_syncookies", tcp_syncookies))
return;
- listen_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
+ listen_fd = start_server_str(sock_family, SOCK_STREAM, srv_addr, 0,
+ &opts);
if (!ASSERT_OK_FD(listen_fd, "start server"))
return;
- err = getsockname(listen_fd, (struct sockaddr *)&srv_sa6, &addrlen);
+ err = getsockname(listen_fd, (struct sockaddr *)addr, &addr_len);
if (!ASSERT_OK(err, "getsockname(listen_fd)"))
goto done;
- memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
- srv_port = ntohs(srv_sa6.sin6_port);
+
+ switch (ip_mode) {
+ case TEST_MODE_IPV4:
+ memcpy(&skel->bss->srv_sa4, &srv_sa4, sizeof(srv_sa4));
+ srv_port = ntohs(srv_sa4.sin_port);
+ break;
+ case TEST_MODE_IPV6:
+ case TEST_MODE_DUAL:
+ memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
+ srv_port = ntohs(srv_sa6.sin6_port);
+ break;
+ default:
+ break;
+ }
cli_fd = connect_to_fd(listen_fd, 0);
if (!ASSERT_OK_FD(cli_fd, "connect client"))
@@ -127,14 +195,34 @@ static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies)
close(srv_fd);
}
-static void test_conn(struct test_btf_skc_cls_ingress *skel)
+static void test_conn_ipv4(struct test_btf_skc_cls_ingress *skel)
+{
+ run_test(skel, false, TEST_MODE_IPV4);
+}
+
+static void test_conn_ipv6(struct test_btf_skc_cls_ingress *skel)
+{
+ run_test(skel, false, TEST_MODE_IPV6);
+}
+
+static void test_conn_dual(struct test_btf_skc_cls_ingress *skel)
+{
+ run_test(skel, false, TEST_MODE_DUAL);
+}
+
+static void test_syncookie_ipv4(struct test_btf_skc_cls_ingress *skel)
+{
+ run_test(skel, true, TEST_MODE_IPV4);
+}
+
+static void test_syncookie_ipv6(struct test_btf_skc_cls_ingress *skel)
{
- run_test(skel, false);
+ run_test(skel, true, TEST_MODE_IPV6);
}
-static void test_syncookie(struct test_btf_skc_cls_ingress *skel)
+static void test_syncookie_dual(struct test_btf_skc_cls_ingress *skel)
{
- run_test(skel, true);
+ run_test(skel, true, TEST_MODE_DUAL);
}
struct test {
@@ -144,8 +232,12 @@ struct test {
#define DEF_TEST(name) { #name, test_##name }
static struct test tests[] = {
- DEF_TEST(conn),
- DEF_TEST(syncookie),
+ DEF_TEST(conn_ipv4),
+ DEF_TEST(conn_ipv6),
+ DEF_TEST(conn_dual),
+ DEF_TEST(syncookie_ipv4),
+ DEF_TEST(syncookie_ipv6),
+ DEF_TEST(syncookie_dual),
};
void test_btf_skc_cls_ingress(void)
diff --git a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
index f0759efff6ef15d2663927400dd064c53b020f78..cd528f8792ff2eb14683cbc13e8b0f3fd38329e9 100644
--- a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
@@ -10,6 +10,7 @@
#endif
struct sockaddr_in6 srv_sa6 = {};
+struct sockaddr_in srv_sa4 = {};
__u16 listen_tp_sport = 0;
__u16 req_sk_sport = 0;
__u32 recv_cookie = 0;
@@ -18,8 +19,8 @@ __u32 linum = 0;
#define LOG() ({ if (!linum) linum = __LINE__; })
-static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
- struct tcp_sock *tp,
+static void test_syncookie_helper(void *iphdr, int iphdr_size,
+ struct tcphdr *th, struct tcp_sock *tp,
struct __sk_buff *skb)
{
if (th->syn) {
@@ -38,7 +39,7 @@ static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
return;
}
- mss_cookie = bpf_tcp_gen_syncookie(tp, ip6h, sizeof(*ip6h),
+ mss_cookie = bpf_tcp_gen_syncookie(tp, iphdr, iphdr_size,
th, 40);
if (mss_cookie < 0) {
if (mss_cookie != -ENOENT)
@@ -48,7 +49,7 @@ static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
}
} else if (gen_cookie) {
/* It was in cookie mode */
- int ret = bpf_tcp_check_syncookie(tp, ip6h, sizeof(*ip6h),
+ int ret = bpf_tcp_check_syncookie(tp, iphdr, iphdr_size,
th, sizeof(*th));
if (ret < 0) {
@@ -60,26 +61,63 @@ static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
}
}
-static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
+static int handle_ip_tcp(struct ethhdr *eth, struct __sk_buff *skb)
{
- struct bpf_sock_tuple *tuple;
+ struct bpf_sock_tuple *tuple = NULL;
+ unsigned int tuple_len = 0;
struct bpf_sock *bpf_skc;
- unsigned int tuple_len;
+ struct ipv6hdr *ip6h;
+ void *iphdr = NULL;
+ int iphdr_size = 0;
+ struct iphdr *ip4h;
struct tcphdr *th;
void *data_end;
data_end = (void *)(long)(skb->data_end);
- th = (struct tcphdr *)(ip6h + 1);
- if (th + 1 > data_end)
- return TC_ACT_OK;
+ switch (eth->h_proto) {
+ case bpf_htons(ETH_P_IP):
+ ip4h = (struct iphdr *)(eth + 1);
+ if (ip4h + 1 > data_end)
+ return TC_ACT_OK;
+ if (ip4h->protocol != IPPROTO_TCP)
+ return TC_ACT_OK;
+ th = (struct tcphdr *)(ip4h + 1);
+ if (th + 1 > data_end)
+ return TC_ACT_OK;
+ /* Is it the testing traffic? */
+ if (th->dest != srv_sa4.sin_port)
+ return TC_ACT_OK;
+ tuple_len = sizeof(tuple->ipv4);
+ tuple = (struct bpf_sock_tuple *)&ip4h->saddr;
+ iphdr = ip4h;
+ iphdr_size = sizeof(*ip4h);
+ break;
+ case bpf_htons(ETH_P_IPV6):
+ ip6h = (struct ipv6hdr *)(eth + 1);
+ if (ip6h + 1 > data_end)
+ return TC_ACT_OK;
+ if (ip6h->nexthdr != IPPROTO_TCP)
+ return TC_ACT_OK;
+ th = (struct tcphdr *)(ip6h + 1);
+ if (th + 1 > data_end)
+ return TC_ACT_OK;
+ /* Is it the testing traffic? */
+ if (th->dest != srv_sa6.sin6_port)
+ return TC_ACT_OK;
+ tuple_len = sizeof(tuple->ipv6);
+ tuple = (struct bpf_sock_tuple *)&ip6h->saddr;
+ iphdr = ip6h;
+ iphdr_size = sizeof(*ip6h);
+ break;
+ default:
+ return TC_ACT_OK;
+ }
- /* Is it the testing traffic? */
- if (th->dest != srv_sa6.sin6_port)
+ if (!tuple) {
+ LOG();
return TC_ACT_OK;
-
- tuple_len = sizeof(tuple->ipv6);
- tuple = (struct bpf_sock_tuple *)&ip6h->saddr;
+ }
if ((void *)tuple + tuple_len > data_end) {
LOG();
return TC_ACT_OK;
@@ -126,7 +164,7 @@ static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
listen_tp_sport = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num;
- test_syncookie_helper(ip6h, th, tp, skb);
+ test_syncookie_helper(iphdr, iphdr_size, th, tp, skb);
bpf_sk_release(tp);
return TC_ACT_OK;
}
@@ -142,7 +180,6 @@ static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
SEC("tc")
int cls_ingress(struct __sk_buff *skb)
{
- struct ipv6hdr *ip6h;
struct ethhdr *eth;
void *data_end;
@@ -152,15 +189,11 @@ int cls_ingress(struct __sk_buff *skb)
if (eth + 1 > data_end)
return TC_ACT_OK;
- if (eth->h_proto != bpf_htons(ETH_P_IPV6))
- return TC_ACT_OK;
-
- ip6h = (struct ipv6hdr *)(eth + 1);
- if (ip6h + 1 > data_end)
+ if (eth->h_proto != bpf_htons(ETH_P_IP) &&
+ eth->h_proto != bpf_htons(ETH_P_IPV6))
return TC_ACT_OK;
- if (ip6h->nexthdr == IPPROTO_TCP)
- return handle_ip6_tcp(ip6h, skb);
+ return handle_ip_tcp(eth, skb);
return TC_ACT_OK;
}
--
2.46.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support in btf_skc_cls_ingress
2024-10-16 18:35 ` [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support " Alexis Lothoré (eBPF Foundation)
@ 2024-10-19 0:30 ` Martin KaFai Lau
2024-10-19 12:43 ` Alexis Lothoré
0 siblings, 1 reply; 12+ messages in thread
From: Martin KaFai Lau @ 2024-10-19 0:30 UTC (permalink / raw)
To: Alexis Lothoré (eBPF Foundation)
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer, ebpf, Thomas Petazzoni, Lorenz Bauer, bpf,
linux-kselftest, linux-kernel, netdev
On 10/16/24 11:35 AM, Alexis Lothoré (eBPF Foundation) wrote:
> btf_skc_cls_ingress test currently checks that syncookie and
> bpf_sk_assign/release helpers behave correctly in multiple scenarios,
> but only with ipv4 socket.
>
> Increase those helpers coverage by adding testing support for IPv6-only
> sockets and IPv4/IPv6 sockets. The rework is mostly based on features
> brought earlier in test_tcp_check_syncookie.sh to cover some fixes
> performed on those helpers, but test_tcp_check_syncookie.sh is not
> integrated in test_progs. The most notable changes linked to this are:
> - some rework in the corresponding eBPF program to support both types of
> traffic
> - the switch from start_server to start_server_str to allow to check
> some socket options
> - the introduction of new subtests for ipv4 and ipv4/ipv6
>
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
> ---
> The rework has been tested in a local Qemu environment and in CI:
> # ./test_progs -a btf_skc_cls_ingress
> #38/1 btf_skc_cls_ingress/conn_ipv4:OK
> #38/2 btf_skc_cls_ingress/conn_ipv6:OK
> #38/3 btf_skc_cls_ingress/conn_dual:OK
> #38/4 btf_skc_cls_ingress/syncookie_ipv4:OK
> #38/5 btf_skc_cls_ingress/syncookie_ipv6:OK
> #38/6 btf_skc_cls_ingress/syncookie_dual:OK
> #38 btf_skc_cls_ingress:OK
> Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED
> ---
> .../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 116 ++++++++++++++++++---
> .../selftests/bpf/progs/test_btf_skc_cls_ingress.c | 81 +++++++++-----
> 2 files changed, 161 insertions(+), 36 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> index a20d104f9909e5ba20ddc4c107b910956f042fc1..e0f8fe818f4230a1d5bf0118133c5a9fb50345e1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> @@ -19,6 +19,15 @@
>
> #define TEST_NS "skc_cls_ingress"
>
> +#define BIT(n) (1 << (n))
> +#define TEST_MODE_IPV4 BIT(0)
> +#define TEST_MODE_IPV6 BIT(1)
> +#define TEST_MODE_DUAL (TEST_MODE_IPV4 | TEST_MODE_IPV6)
> +
> +#define SERVER_ADDR_IPV4 "127.0.0.1"
> +#define SERVER_ADDR_IPV6 "::1"
> +#define SERVER_ADDR_DUAL "::0"
> +
> static struct netns_obj *prepare_netns(struct test_btf_skc_cls_ingress *skel)
> {
> LIBBPF_OPTS(bpf_tc_hook, qdisc_lo, .attach_point = BPF_TC_INGRESS);
> @@ -57,6 +66,7 @@ static struct netns_obj *prepare_netns(struct test_btf_skc_cls_ingress *skel)
>
> static void reset_test(struct test_btf_skc_cls_ingress *skel)
> {
> + memset(&skel->bss->srv_sa4, 0, sizeof(skel->bss->srv_sa4));
> memset(&skel->bss->srv_sa6, 0, sizeof(skel->bss->srv_sa6));
> skel->bss->listen_tp_sport = 0;
> skel->bss->req_sk_sport = 0;
> @@ -71,26 +81,84 @@ static void print_err_line(struct test_btf_skc_cls_ingress *skel)
> printf("bpf prog error at line %u\n", skel->bss->linum);
> }
>
> -static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies)
> +static int v6only_true(int fd, void *opts)
> +{
> + int mode = true;
> +
> + return setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &mode, sizeof(mode));
> +}
> +
> +static int v6only_false(int fd, void *opts)
> +{
> + int mode = false;
> +
> + return setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &mode, sizeof(mode));
> +}
> +
> +static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies,
> + int ip_mode)
> {
> const char *tcp_syncookies = gen_cookies ? "2" : "1";
> int listen_fd = -1, cli_fd = -1, srv_fd = -1, err;
> + struct network_helper_opts opts = { 0 };
> + struct sockaddr_storage *addr;
> struct sockaddr_in6 srv_sa6;
> - socklen_t addrlen = sizeof(srv_sa6);
> + struct sockaddr_in srv_sa4;
> + socklen_t addr_len;
> + int sock_family;
> + char *srv_addr;
> int srv_port;
>
> + switch (ip_mode) {
> + case TEST_MODE_IPV4:
> + sock_family = AF_INET;
> + srv_addr = SERVER_ADDR_IPV4;
> + addr = (struct sockaddr_storage *)&srv_sa4;
> + addr_len = sizeof(srv_sa4);
> + break;
> + case TEST_MODE_IPV6:
> + opts.post_socket_cb = v6only_true;
> + sock_family = AF_INET6;
> + srv_addr = SERVER_ADDR_IPV6;
> + addr = (struct sockaddr_storage *)&srv_sa6;
> + addr_len = sizeof(srv_sa6);
> + break;
> + case TEST_MODE_DUAL:
> + opts.post_socket_cb = v6only_false;
> + sock_family = AF_INET6;
> + srv_addr = SERVER_ADDR_DUAL;
> + addr = (struct sockaddr_storage *)&srv_sa6;
> + addr_len = sizeof(srv_sa6);
> + break;
> + default:
> + break;
nit. indentation is off.
better directly "return;", in case future something complains vars are not init.
> + }
> +
> if (write_sysctl("/proc/sys/net/ipv4/tcp_syncookies", tcp_syncookies))
> return;
>
> - listen_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> + listen_fd = start_server_str(sock_family, SOCK_STREAM, srv_addr, 0,
> + &opts);
> if (!ASSERT_OK_FD(listen_fd, "start server"))
> return;
>
> - err = getsockname(listen_fd, (struct sockaddr *)&srv_sa6, &addrlen);
> + err = getsockname(listen_fd, (struct sockaddr *)addr, &addr_len);
> if (!ASSERT_OK(err, "getsockname(listen_fd)"))
> goto done;
> - memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
> - srv_port = ntohs(srv_sa6.sin6_port);
> +
> + switch (ip_mode) {
> + case TEST_MODE_IPV4:
> + memcpy(&skel->bss->srv_sa4, &srv_sa4, sizeof(srv_sa4));
> + srv_port = ntohs(srv_sa4.sin_port);
> + break;
> + case TEST_MODE_IPV6:
> + case TEST_MODE_DUAL:
> + memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
> + srv_port = ntohs(srv_sa6.sin6_port);
> + break;
> + default:
> + break;
indentation off. also "goto done;"
> + }
>
> cli_fd = connect_to_fd(listen_fd, 0);
> if (!ASSERT_OK_FD(cli_fd, "connect client"))
> @@ -127,14 +195,34 @@ static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies)
> close(srv_fd);
> }
>
> -static void test_conn(struct test_btf_skc_cls_ingress *skel)
> +static void test_conn_ipv4(struct test_btf_skc_cls_ingress *skel)
> +{
> + run_test(skel, false, TEST_MODE_IPV4);
> +}
> +
> +static void test_conn_ipv6(struct test_btf_skc_cls_ingress *skel)
> +{
> + run_test(skel, false, TEST_MODE_IPV6);
> +}
> +
> +static void test_conn_dual(struct test_btf_skc_cls_ingress *skel)
> +{
> + run_test(skel, false, TEST_MODE_DUAL);
> +}
> +
> +static void test_syncookie_ipv4(struct test_btf_skc_cls_ingress *skel)
> +{
> + run_test(skel, true, TEST_MODE_IPV4);
> +}
> +
> +static void test_syncookie_ipv6(struct test_btf_skc_cls_ingress *skel)
> {
> - run_test(skel, false);
> + run_test(skel, true, TEST_MODE_IPV6);
> }
>
> -static void test_syncookie(struct test_btf_skc_cls_ingress *skel)
> +static void test_syncookie_dual(struct test_btf_skc_cls_ingress *skel)
> {
> - run_test(skel, true);
> + run_test(skel, true, TEST_MODE_DUAL);
> }
>
> struct test {
> @@ -144,8 +232,12 @@ struct test {
>
> #define DEF_TEST(name) { #name, test_##name }
> static struct test tests[] = {
> - DEF_TEST(conn),
> - DEF_TEST(syncookie),
> + DEF_TEST(conn_ipv4),
> + DEF_TEST(conn_ipv6),
> + DEF_TEST(conn_dual),
> + DEF_TEST(syncookie_ipv4),
> + DEF_TEST(syncookie_ipv6),
> + DEF_TEST(syncookie_dual),
> };
>
> void test_btf_skc_cls_ingress(void)
> diff --git a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
> index f0759efff6ef15d2663927400dd064c53b020f78..cd528f8792ff2eb14683cbc13e8b0f3fd38329e9 100644
> --- a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
> +++ b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
> @@ -10,6 +10,7 @@
> #endif
>
> struct sockaddr_in6 srv_sa6 = {};
> +struct sockaddr_in srv_sa4 = {};
> __u16 listen_tp_sport = 0;
> __u16 req_sk_sport = 0;
> __u32 recv_cookie = 0;
> @@ -18,8 +19,8 @@ __u32 linum = 0;
>
> #define LOG() ({ if (!linum) linum = __LINE__; })
>
> -static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
> - struct tcp_sock *tp,
> +static void test_syncookie_helper(void *iphdr, int iphdr_size,
> + struct tcphdr *th, struct tcp_sock *tp,
> struct __sk_buff *skb)
> {
> if (th->syn) {
> @@ -38,7 +39,7 @@ static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
> return;
> }
>
> - mss_cookie = bpf_tcp_gen_syncookie(tp, ip6h, sizeof(*ip6h),
> + mss_cookie = bpf_tcp_gen_syncookie(tp, iphdr, iphdr_size,
> th, 40);
> if (mss_cookie < 0) {
> if (mss_cookie != -ENOENT)
> @@ -48,7 +49,7 @@ static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
> }
> } else if (gen_cookie) {
> /* It was in cookie mode */
> - int ret = bpf_tcp_check_syncookie(tp, ip6h, sizeof(*ip6h),
> + int ret = bpf_tcp_check_syncookie(tp, iphdr, iphdr_size,
> th, sizeof(*th));
>
> if (ret < 0) {
> @@ -60,26 +61,63 @@ static void test_syncookie_helper(struct ipv6hdr *ip6h, struct tcphdr *th,
> }
> }
>
> -static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
> +static int handle_ip_tcp(struct ethhdr *eth, struct __sk_buff *skb)
> {
> - struct bpf_sock_tuple *tuple;
> + struct bpf_sock_tuple *tuple = NULL;
> + unsigned int tuple_len = 0;
> struct bpf_sock *bpf_skc;
> - unsigned int tuple_len;
> + struct ipv6hdr *ip6h;
> + void *iphdr = NULL;
> + int iphdr_size = 0;
> + struct iphdr *ip4h;
nit. All new "= 0;" and "= NULL;" init should not be needed.
> struct tcphdr *th;
> void *data_end;
>
> data_end = (void *)(long)(skb->data_end);
>
> - th = (struct tcphdr *)(ip6h + 1);
> - if (th + 1 > data_end)
> - return TC_ACT_OK;
> + switch (eth->h_proto) {
> + case bpf_htons(ETH_P_IP):
> + ip4h = (struct iphdr *)(eth + 1);
> + if (ip4h + 1 > data_end)
> + return TC_ACT_OK;
> + if (ip4h->protocol != IPPROTO_TCP)
> + return TC_ACT_OK;
> + th = (struct tcphdr *)(ip4h + 1);
> + if (th + 1 > data_end)
> + return TC_ACT_OK;
> + /* Is it the testing traffic? */
> + if (th->dest != srv_sa4.sin_port)
> + return TC_ACT_OK;
> + tuple_len = sizeof(tuple->ipv4);
> + tuple = (struct bpf_sock_tuple *)&ip4h->saddr;
> + iphdr = ip4h;
> + iphdr_size = sizeof(*ip4h);
> + break;
> + case bpf_htons(ETH_P_IPV6):
> + ip6h = (struct ipv6hdr *)(eth + 1);
> + if (ip6h + 1 > data_end)
> + return TC_ACT_OK;
> + if (ip6h->nexthdr != IPPROTO_TCP)
> + return TC_ACT_OK;
> + th = (struct tcphdr *)(ip6h + 1);
> + if (th + 1 > data_end)
> + return TC_ACT_OK;
> + /* Is it the testing traffic? */
> + if (th->dest != srv_sa6.sin6_port)
> + return TC_ACT_OK;
> + tuple_len = sizeof(tuple->ipv6);
> + tuple = (struct bpf_sock_tuple *)&ip6h->saddr;
> + iphdr = ip6h;
> + iphdr_size = sizeof(*ip6h);
> + break;
> + default:
> + return TC_ACT_OK;
indentation is off also.
> + }
>
> - /* Is it the testing traffic? */
> - if (th->dest != srv_sa6.sin6_port)
> + if (!tuple) {
!tuple should not be possible. can be removed.
> + LOG();
> return TC_ACT_OK;
> -
> - tuple_len = sizeof(tuple->ipv6);
> - tuple = (struct bpf_sock_tuple *)&ip6h->saddr;
> + }
> if ((void *)tuple + tuple_len > data_end) {
> LOG();
> return TC_ACT_OK;
> @@ -126,7 +164,7 @@ static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
>
> listen_tp_sport = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num;
>
> - test_syncookie_helper(ip6h, th, tp, skb);
> + test_syncookie_helper(iphdr, iphdr_size, th, tp, skb);
> bpf_sk_release(tp);
> return TC_ACT_OK;
> }
> @@ -142,7 +180,6 @@ static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
> SEC("tc")
> int cls_ingress(struct __sk_buff *skb)
> {
> - struct ipv6hdr *ip6h;
> struct ethhdr *eth;
> void *data_end;
>
> @@ -152,15 +189,11 @@ int cls_ingress(struct __sk_buff *skb)
> if (eth + 1 > data_end)
> return TC_ACT_OK;
>
> - if (eth->h_proto != bpf_htons(ETH_P_IPV6))
> - return TC_ACT_OK;
> -
> - ip6h = (struct ipv6hdr *)(eth + 1);
> - if (ip6h + 1 > data_end)
> + if (eth->h_proto != bpf_htons(ETH_P_IP) &&
> + eth->h_proto != bpf_htons(ETH_P_IPV6))
> return TC_ACT_OK;
>
> - if (ip6h->nexthdr == IPPROTO_TCP)
> - return handle_ip6_tcp(ip6h, skb);
> + return handle_ip_tcp(eth, skb);
>
> return TC_ACT_OK;
The last double return should have been removed also.
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support in btf_skc_cls_ingress
2024-10-19 0:30 ` Martin KaFai Lau
@ 2024-10-19 12:43 ` Alexis Lothoré
0 siblings, 0 replies; 12+ messages in thread
From: Alexis Lothoré @ 2024-10-19 12:43 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer, ebpf, Thomas Petazzoni, Lorenz Bauer, bpf,
linux-kselftest, linux-kernel, netdev
On 10/19/24 02:30, Martin KaFai Lau wrote:
> On 10/16/24 11:35 AM, Alexis Lothoré (eBPF Foundation) wrote:
[...]
>> + switch (ip_mode) {
>> + case TEST_MODE_IPV4:
>> + sock_family = AF_INET;
>> + srv_addr = SERVER_ADDR_IPV4;
>> + addr = (struct sockaddr_storage *)&srv_sa4;
>> + addr_len = sizeof(srv_sa4);
>> + break;
>> + case TEST_MODE_IPV6:
>> + opts.post_socket_cb = v6only_true;
>> + sock_family = AF_INET6;
>> + srv_addr = SERVER_ADDR_IPV6;
>> + addr = (struct sockaddr_storage *)&srv_sa6;
>> + addr_len = sizeof(srv_sa6);
>> + break;
>> + case TEST_MODE_DUAL:
>> + opts.post_socket_cb = v6only_false;
>> + sock_family = AF_INET6;
>> + srv_addr = SERVER_ADDR_DUAL;
>> + addr = (struct sockaddr_storage *)&srv_sa6;
>> + addr_len = sizeof(srv_sa6);
>> + break;
>> + default:
>> + break;
>
> nit. indentation is off.
True, and for some reason checkpatch does not raise any warning here. This will
be fixed in v2 for all switch cases
> better directly "return;", in case future something complains vars are not init.
ACK. I'll also add a PRINT_FAIL call in those default statements.
>
>> + }
>> +
>> if (write_sysctl("/proc/sys/net/ipv4/tcp_syncookies", tcp_syncookies))
>> return;
>> - listen_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
>> + listen_fd = start_server_str(sock_family, SOCK_STREAM, srv_addr, 0,
>> + &opts);
>> if (!ASSERT_OK_FD(listen_fd, "start server"))
>> return;
>> - err = getsockname(listen_fd, (struct sockaddr *)&srv_sa6, &addrlen);
>> + err = getsockname(listen_fd, (struct sockaddr *)addr, &addr_len);
>> if (!ASSERT_OK(err, "getsockname(listen_fd)"))
>> goto done;
>> - memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
>> - srv_port = ntohs(srv_sa6.sin6_port);
>> +
>> + switch (ip_mode) {
>> + case TEST_MODE_IPV4:
>> + memcpy(&skel->bss->srv_sa4, &srv_sa4, sizeof(srv_sa4));
>> + srv_port = ntohs(srv_sa4.sin_port);
>> + break;
>> + case TEST_MODE_IPV6:
>> + case TEST_MODE_DUAL:
>> + memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
>> + srv_port = ntohs(srv_sa6.sin6_port);
>> + break;
>> + default:
>> + break;
>
> indentation off. also "goto done;"
I can add the goto done as a safety net (eg in case someone modifies the test
later and miss some details), but with the return added on the switch above
(also done on ip_mode), the goto should never be executed.
[...]
>> -static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
>> +static int handle_ip_tcp(struct ethhdr *eth, struct __sk_buff *skb)
>> {
>> - struct bpf_sock_tuple *tuple;
>> + struct bpf_sock_tuple *tuple = NULL;
>> + unsigned int tuple_len = 0;
>> struct bpf_sock *bpf_skc;
>> - unsigned int tuple_len;
>> + struct ipv6hdr *ip6h;
>> + void *iphdr = NULL;
>> + int iphdr_size = 0;
>> + struct iphdr *ip4h;
>
> nit. All new "= 0;" and "= NULL;" init should not be needed.
I added those "by default" because my last series raised some build errors with
clang while building fine with gcc (but in the end, there was indeed code paths
possibly using those variables uninitialized). ACK, I'll remove those initializers.
[...]
>> + }
>> - /* Is it the testing traffic? */
>> - if (th->dest != srv_sa6.sin6_port)
>> + if (!tuple) {
>
> !tuple should not be possible. can be removed.
I initially added that check because of the verifier complained about tuple
being scalar. But I have checked and removed it again and it is now... accepted
? I guess I was still missing some other parts when I got this error, very
likely the `return TC_ACT_OK` in the default case. I'll then remove this check
in v2.
[...]
>> - if (ip6h->nexthdr == IPPROTO_TCP)
>> - return handle_ip6_tcp(ip6h, skb);
>> + return handle_ip_tcp(eth, skb);
>> return TC_ACT_OK;
>
> The last double return should have been removed also.
That's indeed a miss, I'll remove it.
Thanks,
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next 5/6] selftests/bpf: test MSS value returned with bpf_tcp_gen_syncookie
2024-10-16 18:35 [PATCH bpf-next 0/6] selftests/bpf: integrate test_tcp_check_syncookie.sh into test_progs Alexis Lothoré (eBPF Foundation)
` (3 preceding siblings ...)
2024-10-16 18:35 ` [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support " Alexis Lothoré (eBPF Foundation)
@ 2024-10-16 18:35 ` Alexis Lothoré (eBPF Foundation)
2024-10-16 18:35 ` [PATCH bpf-next 6/6] selftests/bpf: remove test_tcp_check_syncookie Alexis Lothoré (eBPF Foundation)
5 siblings, 0 replies; 12+ messages in thread
From: Alexis Lothoré (eBPF Foundation) @ 2024-10-16 18:35 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer
Cc: ebpf, Thomas Petazzoni, Lorenz Bauer, bpf, linux-kselftest,
linux-kernel, netdev, Alexis Lothoré (eBPF Foundation)
One remaining difference between test_tcp_check_syncookie.sh and
btf_skc_cls_ingress is a small test on the mss value embedded in the
cookie generated with the eBPF helper.
Bring the corresponding test in btf_skc_cls_ingress.
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 7 +++++++
tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c | 2 ++
2 files changed, 9 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
index e0f8fe818f4230a1d5bf0118133c5a9fb50345e1..64601bafdd23e7fb13ebd496ac914660685f1efc 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
@@ -27,6 +27,8 @@
#define SERVER_ADDR_IPV4 "127.0.0.1"
#define SERVER_ADDR_IPV6 "::1"
#define SERVER_ADDR_DUAL "::0"
+/* RFC791, 576 for minimal IPv4 datagram, minus 40 bytes of TCP header */
+#define MIN_IPV4_MSS 536
static struct netns_obj *prepare_netns(struct test_btf_skc_cls_ingress *skel)
{
@@ -73,6 +75,7 @@ static void reset_test(struct test_btf_skc_cls_ingress *skel)
skel->bss->recv_cookie = 0;
skel->bss->gen_cookie = 0;
skel->bss->linum = 0;
+ skel->bss->mss = 0;
}
static void print_err_line(struct test_btf_skc_cls_ingress *skel)
@@ -184,6 +187,10 @@ static void run_test(struct test_btf_skc_cls_ingress *skel, bool gen_cookies,
"syncookie properly generated");
ASSERT_EQ(skel->bss->gen_cookie, skel->bss->recv_cookie,
"matching syncookies on client and server");
+ ASSERT_GT(skel->bss->mss, MIN_IPV4_MSS,
+ "MSS in cookie min value");
+ ASSERT_LT(skel->bss->mss, USHRT_MAX,
+ "MSS in cookie max value");
}
done:
diff --git a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
index cd528f8792ff2eb14683cbc13e8b0f3fd38329e9..3810ce98d3d54276f9a0130a887f59c79a21288e 100644
--- a/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
+++ b/tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
@@ -15,6 +15,7 @@ __u16 listen_tp_sport = 0;
__u16 req_sk_sport = 0;
__u32 recv_cookie = 0;
__u32 gen_cookie = 0;
+__u32 mss = 0;
__u32 linum = 0;
#define LOG() ({ if (!linum) linum = __LINE__; })
@@ -46,6 +47,7 @@ static void test_syncookie_helper(void *iphdr, int iphdr_size,
LOG();
} else {
gen_cookie = (__u32)mss_cookie;
+ mss = mss_cookie >> 32;
}
} else if (gen_cookie) {
/* It was in cookie mode */
--
2.46.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH bpf-next 6/6] selftests/bpf: remove test_tcp_check_syncookie
2024-10-16 18:35 [PATCH bpf-next 0/6] selftests/bpf: integrate test_tcp_check_syncookie.sh into test_progs Alexis Lothoré (eBPF Foundation)
` (4 preceding siblings ...)
2024-10-16 18:35 ` [PATCH bpf-next 5/6] selftests/bpf: test MSS value returned with bpf_tcp_gen_syncookie Alexis Lothoré (eBPF Foundation)
@ 2024-10-16 18:35 ` Alexis Lothoré (eBPF Foundation)
2024-10-19 0:34 ` Martin KaFai Lau
5 siblings, 1 reply; 12+ messages in thread
From: Alexis Lothoré (eBPF Foundation) @ 2024-10-16 18:35 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer
Cc: ebpf, Thomas Petazzoni, Lorenz Bauer, bpf, linux-kselftest,
linux-kernel, netdev, Alexis Lothoré (eBPF Foundation)
Now that btf_skc_cls_ingress has the same coverage as
test_tcp_check_syncookie, remove the second one and keep the first one
as it is integrated in test_progs
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
tools/testing/selftests/bpf/.gitignore | 1 -
tools/testing/selftests/bpf/Makefile | 9 +-
.../bpf/progs/test_tcp_check_syncookie_kern.c | 167 ----------------
.../selftests/bpf/test_tcp_check_syncookie.sh | 85 --------
.../selftests/bpf/test_tcp_check_syncookie_user.c | 213 ---------------------
5 files changed, 3 insertions(+), 472 deletions(-)
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e6533b3400de5ef9b0c0e02aa0e1afedcab9b349..7e88551f2d38bdfa87fd56ac83874c88c4335927 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -24,7 +24,6 @@ test_flow_dissector
flow_dissector_load
test_tcpnotify_user
test_libbpf
-test_tcp_check_syncookie_user
test_sysctl
xdping
test_cpp
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 28a76baa854d3ef45bc6d511ad9188e737f0ebe8..7815b815fcef4a8e42f0211253c1e351a9af2abf 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -137,7 +137,6 @@ TEST_PROGS := test_kmod.sh \
test_xdp_vlan_mode_generic.sh \
test_xdp_vlan_mode_native.sh \
test_lwt_ip_encap.sh \
- test_tcp_check_syncookie.sh \
test_tc_tunnel.sh \
test_tc_edt.sh \
test_xdping.sh \
@@ -154,10 +153,9 @@ TEST_PROGS_EXTENDED := with_addr.sh \
# Compile but not part of 'make run_tests'
TEST_GEN_PROGS_EXTENDED = \
- flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
- test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
- xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
- xdp_features bpf_test_no_cfi.ko
+ flow_dissector_load test_flow_dissector test_lirc_mode2_user xdping \
+ test_cpp runqslower bench bpf_testmod.ko xskxceiver xdp_redirect_multi \
+ xdp_synproxy veristat xdp_hw_metadata xdp_features bpf_test_no_cfi.ko
TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
@@ -347,7 +345,6 @@ $(OUTPUT)/flow_dissector_load: $(TESTING_HELPERS)
$(OUTPUT)/test_maps: $(TESTING_HELPERS)
$(OUTPUT)/test_verifier: $(TESTING_HELPERS) $(CAP_HELPERS) $(UNPRIV_HELPERS)
$(OUTPUT)/xsk.o: $(BPFOBJ)
-$(OUTPUT)/test_tcp_check_syncookie_user: $(NETWORK_HELPERS)
BPFTOOL ?= $(DEFAULT_BPFTOOL)
$(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \
diff --git a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
deleted file mode 100644
index 6edebce563b57e96b04a20b6d14dbcdca62152d5..0000000000000000000000000000000000000000
--- a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
+++ /dev/null
@@ -1,167 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2018 Facebook
-// Copyright (c) 2019 Cloudflare
-
-#include <string.h>
-
-#include <linux/bpf.h>
-#include <linux/pkt_cls.h>
-#include <linux/if_ether.h>
-#include <linux/in.h>
-#include <linux/ip.h>
-#include <linux/ipv6.h>
-#include <sys/socket.h>
-#include <linux/tcp.h>
-
-#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_endian.h>
-
-struct {
- __uint(type, BPF_MAP_TYPE_ARRAY);
- __type(key, __u32);
- __type(value, __u32);
- __uint(max_entries, 3);
-} results SEC(".maps");
-
-static __always_inline __s64 gen_syncookie(void *data_end, struct bpf_sock *sk,
- void *iph, __u32 ip_size,
- struct tcphdr *tcph)
-{
- __u32 thlen = tcph->doff * 4;
-
- if (tcph->syn && !tcph->ack) {
- // packet should only have an MSS option
- if (thlen != 24)
- return 0;
-
- if ((void *)tcph + thlen > data_end)
- return 0;
-
- return bpf_tcp_gen_syncookie(sk, iph, ip_size, tcph, thlen);
- }
- return 0;
-}
-
-static __always_inline void check_syncookie(void *ctx, void *data,
- void *data_end)
-{
- struct bpf_sock_tuple tup;
- struct bpf_sock *sk;
- struct ethhdr *ethh;
- struct iphdr *ipv4h;
- struct ipv6hdr *ipv6h;
- struct tcphdr *tcph;
- int ret;
- __u32 key_mss = 2;
- __u32 key_gen = 1;
- __u32 key = 0;
- __s64 seq_mss;
-
- ethh = data;
- if (ethh + 1 > data_end)
- return;
-
- switch (bpf_ntohs(ethh->h_proto)) {
- case ETH_P_IP:
- ipv4h = data + sizeof(struct ethhdr);
- if (ipv4h + 1 > data_end)
- return;
-
- if (ipv4h->ihl != 5)
- return;
-
- tcph = data + sizeof(struct ethhdr) + sizeof(struct iphdr);
- if (tcph + 1 > data_end)
- return;
-
- tup.ipv4.saddr = ipv4h->saddr;
- tup.ipv4.daddr = ipv4h->daddr;
- tup.ipv4.sport = tcph->source;
- tup.ipv4.dport = tcph->dest;
-
- sk = bpf_skc_lookup_tcp(ctx, &tup, sizeof(tup.ipv4),
- BPF_F_CURRENT_NETNS, 0);
- if (!sk)
- return;
-
- if (sk->state != BPF_TCP_LISTEN)
- goto release;
-
- seq_mss = gen_syncookie(data_end, sk, ipv4h, sizeof(*ipv4h),
- tcph);
-
- ret = bpf_tcp_check_syncookie(sk, ipv4h, sizeof(*ipv4h),
- tcph, sizeof(*tcph));
- break;
-
- case ETH_P_IPV6:
- ipv6h = data + sizeof(struct ethhdr);
- if (ipv6h + 1 > data_end)
- return;
-
- if (ipv6h->nexthdr != IPPROTO_TCP)
- return;
-
- tcph = data + sizeof(struct ethhdr) + sizeof(struct ipv6hdr);
- if (tcph + 1 > data_end)
- return;
-
- memcpy(tup.ipv6.saddr, &ipv6h->saddr, sizeof(tup.ipv6.saddr));
- memcpy(tup.ipv6.daddr, &ipv6h->daddr, sizeof(tup.ipv6.daddr));
- tup.ipv6.sport = tcph->source;
- tup.ipv6.dport = tcph->dest;
-
- sk = bpf_skc_lookup_tcp(ctx, &tup, sizeof(tup.ipv6),
- BPF_F_CURRENT_NETNS, 0);
- if (!sk)
- return;
-
- if (sk->state != BPF_TCP_LISTEN)
- goto release;
-
- seq_mss = gen_syncookie(data_end, sk, ipv6h, sizeof(*ipv6h),
- tcph);
-
- ret = bpf_tcp_check_syncookie(sk, ipv6h, sizeof(*ipv6h),
- tcph, sizeof(*tcph));
- break;
-
- default:
- return;
- }
-
- if (seq_mss > 0) {
- __u32 cookie = (__u32)seq_mss;
- __u32 mss = seq_mss >> 32;
-
- bpf_map_update_elem(&results, &key_gen, &cookie, 0);
- bpf_map_update_elem(&results, &key_mss, &mss, 0);
- }
-
- if (ret == 0) {
- __u32 cookie = bpf_ntohl(tcph->ack_seq) - 1;
-
- bpf_map_update_elem(&results, &key, &cookie, 0);
- }
-
-release:
- bpf_sk_release(sk);
-}
-
-SEC("tc")
-int check_syncookie_clsact(struct __sk_buff *skb)
-{
- check_syncookie(skb, (void *)(long)skb->data,
- (void *)(long)skb->data_end);
- return TC_ACT_OK;
-}
-
-SEC("xdp")
-int check_syncookie_xdp(struct xdp_md *ctx)
-{
- check_syncookie(ctx, (void *)(long)ctx->data,
- (void *)(long)ctx->data_end);
- return XDP_PASS;
-}
-
-char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh b/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
deleted file mode 100755
index b42c24282c2543a61b559e70c4961011ddb8c463..0000000000000000000000000000000000000000
--- a/tools/testing/selftests/bpf/test_tcp_check_syncookie.sh
+++ /dev/null
@@ -1,85 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0
-# Copyright (c) 2018 Facebook
-# Copyright (c) 2019 Cloudflare
-
-set -eu
-readonly NS1="ns1-$(mktemp -u XXXXXX)"
-
-wait_for_ip()
-{
- local _i
- printf "Wait for IP %s to become available " "$1"
- for _i in $(seq ${MAX_PING_TRIES}); do
- printf "."
- if ns1_exec ping -c 1 -W 1 "$1" >/dev/null 2>&1; then
- echo " OK"
- return
- fi
- sleep 1
- done
- echo 1>&2 "ERROR: Timeout waiting for test IP to become available."
- exit 1
-}
-
-get_prog_id()
-{
- awk '/ id / {sub(/.* id /, "", $0); print($1)}'
-}
-
-ns1_exec()
-{
- ip netns exec ${NS1} "$@"
-}
-
-setup()
-{
- ip netns add ${NS1}
- ns1_exec ip link set lo up
-
- ns1_exec sysctl -w net.ipv4.tcp_syncookies=2
- ns1_exec sysctl -w net.ipv4.tcp_window_scaling=0
- ns1_exec sysctl -w net.ipv4.tcp_timestamps=0
- ns1_exec sysctl -w net.ipv4.tcp_sack=0
-
- wait_for_ip 127.0.0.1
- wait_for_ip ::1
-}
-
-cleanup()
-{
- ip netns del ns1 2>/dev/null || :
-}
-
-main()
-{
- trap cleanup EXIT 2 3 6 15
- setup
-
- printf "Testing clsact..."
- ns1_exec tc qdisc add dev "${TEST_IF}" clsact
- ns1_exec tc filter add dev "${TEST_IF}" ingress \
- bpf obj "${BPF_PROG_OBJ}" sec "${CLSACT_SECTION}" da
-
- BPF_PROG_ID=$(ns1_exec tc filter show dev "${TEST_IF}" ingress | \
- get_prog_id)
- ns1_exec "${PROG}" "${BPF_PROG_ID}"
- ns1_exec tc qdisc del dev "${TEST_IF}" clsact
-
- printf "Testing XDP..."
- ns1_exec ip link set "${TEST_IF}" xdp \
- object "${BPF_PROG_OBJ}" section "${XDP_SECTION}"
- BPF_PROG_ID=$(ns1_exec ip link show "${TEST_IF}" | get_prog_id)
- ns1_exec "${PROG}" "${BPF_PROG_ID}"
-}
-
-DIR=$(dirname $0)
-TEST_IF=lo
-MAX_PING_TRIES=5
-BPF_PROG_OBJ="${DIR}/test_tcp_check_syncookie_kern.bpf.o"
-CLSACT_SECTION="tc"
-XDP_SECTION="xdp"
-BPF_PROG_ID=0
-PROG="${DIR}/test_tcp_check_syncookie_user"
-
-main
diff --git a/tools/testing/selftests/bpf/test_tcp_check_syncookie_user.c b/tools/testing/selftests/bpf/test_tcp_check_syncookie_user.c
deleted file mode 100644
index 3844f9b8232a25278e8190bc9b878ed73060fd4d..0000000000000000000000000000000000000000
--- a/tools/testing/selftests/bpf/test_tcp_check_syncookie_user.c
+++ /dev/null
@@ -1,213 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2018 Facebook
-// Copyright (c) 2019 Cloudflare
-
-#include <limits.h>
-#include <string.h>
-#include <stdlib.h>
-#include <unistd.h>
-
-#include <arpa/inet.h>
-#include <netinet/in.h>
-#include <sys/types.h>
-#include <sys/socket.h>
-
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "cgroup_helpers.h"
-#include "network_helpers.h"
-
-static int get_map_fd_by_prog_id(int prog_id, bool *xdp)
-{
- struct bpf_prog_info info = {};
- __u32 info_len = sizeof(info);
- __u32 map_ids[1];
- int prog_fd = -1;
- int map_fd = -1;
-
- prog_fd = bpf_prog_get_fd_by_id(prog_id);
- if (prog_fd < 0) {
- log_err("Failed to get fd by prog id %d", prog_id);
- goto err;
- }
-
- info.nr_map_ids = 1;
- info.map_ids = (__u64)(unsigned long)map_ids;
-
- if (bpf_prog_get_info_by_fd(prog_fd, &info, &info_len)) {
- log_err("Failed to get info by prog fd %d", prog_fd);
- goto err;
- }
-
- if (!info.nr_map_ids) {
- log_err("No maps found for prog fd %d", prog_fd);
- goto err;
- }
-
- *xdp = info.type == BPF_PROG_TYPE_XDP;
-
- map_fd = bpf_map_get_fd_by_id(map_ids[0]);
- if (map_fd < 0)
- log_err("Failed to get fd by map id %d", map_ids[0]);
-err:
- if (prog_fd >= 0)
- close(prog_fd);
- return map_fd;
-}
-
-static int run_test(int server_fd, int results_fd, bool xdp)
-{
- int client = -1, srv_client = -1;
- int ret = 0;
- __u32 key = 0;
- __u32 key_gen = 1;
- __u32 key_mss = 2;
- __u32 value = 0;
- __u32 value_gen = 0;
- __u32 value_mss = 0;
-
- if (bpf_map_update_elem(results_fd, &key, &value, 0) < 0) {
- log_err("Can't clear results");
- goto err;
- }
-
- if (bpf_map_update_elem(results_fd, &key_gen, &value_gen, 0) < 0) {
- log_err("Can't clear results");
- goto err;
- }
-
- if (bpf_map_update_elem(results_fd, &key_mss, &value_mss, 0) < 0) {
- log_err("Can't clear results");
- goto err;
- }
-
- client = connect_to_fd(server_fd, 0);
- if (client == -1)
- goto err;
-
- srv_client = accept(server_fd, NULL, 0);
- if (srv_client == -1) {
- log_err("Can't accept connection");
- goto err;
- }
-
- if (bpf_map_lookup_elem(results_fd, &key, &value) < 0) {
- log_err("Can't lookup result");
- goto err;
- }
-
- if (value == 0) {
- log_err("Didn't match syncookie: %u", value);
- goto err;
- }
-
- if (bpf_map_lookup_elem(results_fd, &key_gen, &value_gen) < 0) {
- log_err("Can't lookup result");
- goto err;
- }
-
- if (xdp && value_gen == 0) {
- // SYN packets do not get passed through generic XDP, skip the
- // rest of the test.
- printf("Skipping XDP cookie check\n");
- goto out;
- }
-
- if (bpf_map_lookup_elem(results_fd, &key_mss, &value_mss) < 0) {
- log_err("Can't lookup result");
- goto err;
- }
-
- if (value != value_gen) {
- log_err("BPF generated cookie does not match kernel one");
- goto err;
- }
-
- if (value_mss < 536 || value_mss > USHRT_MAX) {
- log_err("Unexpected MSS retrieved");
- goto err;
- }
-
- goto out;
-
-err:
- ret = 1;
-out:
- close(client);
- close(srv_client);
- return ret;
-}
-
-static int v6only_true(int fd, void *opts)
-{
- int mode = true;
-
- return setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &mode, sizeof(mode));
-}
-
-static int v6only_false(int fd, void *opts)
-{
- int mode = false;
-
- return setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &mode, sizeof(mode));
-}
-
-int main(int argc, char **argv)
-{
- struct network_helper_opts opts = { 0 };
- int server = -1;
- int server_v6 = -1;
- int server_dual = -1;
- int results = -1;
- int err = 0;
- bool xdp;
-
- if (argc < 2) {
- fprintf(stderr, "Usage: %s prog_id\n", argv[0]);
- exit(1);
- }
-
- /* Use libbpf 1.0 API mode */
- libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
-
- results = get_map_fd_by_prog_id(atoi(argv[1]), &xdp);
- if (results < 0) {
- log_err("Can't get map");
- goto err;
- }
-
- server = start_server_str(AF_INET, SOCK_STREAM, "127.0.0.1", 0, NULL);
- if (server == -1)
- goto err;
-
- opts.post_socket_cb = v6only_true;
- server_v6 = start_server_str(AF_INET6, SOCK_STREAM, "::1", 0, &opts);
- if (server_v6 == -1)
- goto err;
-
- opts.post_socket_cb = v6only_false;
- server_dual = start_server_str(AF_INET6, SOCK_STREAM, "::0", 0, &opts);
- if (server_dual == -1)
- goto err;
-
- if (run_test(server, results, xdp))
- goto err;
-
- if (run_test(server_v6, results, xdp))
- goto err;
-
- if (run_test(server_dual, results, xdp))
- goto err;
-
- printf("ok\n");
- goto out;
-err:
- err = 1;
-out:
- close(server);
- close(server_v6);
- close(server_dual);
- close(results);
- return err;
-}
--
2.46.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next 6/6] selftests/bpf: remove test_tcp_check_syncookie
2024-10-16 18:35 ` [PATCH bpf-next 6/6] selftests/bpf: remove test_tcp_check_syncookie Alexis Lothoré (eBPF Foundation)
@ 2024-10-19 0:34 ` Martin KaFai Lau
0 siblings, 0 replies; 12+ messages in thread
From: Martin KaFai Lau @ 2024-10-19 0:34 UTC (permalink / raw)
To: Alexis Lothoré (eBPF Foundation)
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer, ebpf, Thomas Petazzoni, Lorenz Bauer, bpf,
linux-kselftest, linux-kernel, netdev
On 10/16/24 11:35 AM, Alexis Lothoré (eBPF Foundation) wrote:
> .../bpf/progs/test_tcp_check_syncookie_kern.c | 167 ----------------
> .../selftests/bpf/test_tcp_check_syncookie.sh | 85 --------
> .../selftests/bpf/test_tcp_check_syncookie_user.c | 213 ---------------------
Nice.
Left some comments in the earlier patches. Overall looks good. Thanks.
pw-bot: cr
^ permalink raw reply [flat|nested] 12+ messages in thread