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

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.

}