diff mbox series

[bpf-next,4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support in btf_skc_cls_ingress

Message ID 20241016-syncookie-v1-4-3b7a0de12153@bootlin.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series selftests/bpf: integrate test_tcp_check_syncookie.sh into test_progs | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success Errors and warnings before: 0 (+1) this patch: 0 (+1)
netdev/cc_maintainers success CCed 17 of 17 maintainers
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 304 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18

Commit Message

Alexis Lothoré (eBPF Foundation) Oct. 16, 2024, 6:35 p.m. UTC 
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(-)

Comments

Martin KaFai Lau Oct. 19, 2024, 12:30 a.m. UTC | #1 
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.

>   }
>
Alexis Lothoré (eBPF Foundation) Oct. 19, 2024, 12:43 p.m. UTC | #2 
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
diff mbox series

Patch

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;
 }