Hi Jiayuan, Thank you for this new test!
I'm not very familiar with the BPF selftests: it would be nice if someone else can have a quick look. On 05/11/2025 12:36, Jiayuan Chen wrote: > Add test cases to verify that when MPTCP falls back to plain TCP sockets, > they can properly work with sockmap. > > Additionally, add test cases to ensure that sockmap correctly rejects > MPTCP sockets as expected. > > Signed-off-by: Jiayuan Chen <[email protected]> > --- > .../testing/selftests/bpf/prog_tests/mptcp.c | 150 ++++++++++++++++++ > .../selftests/bpf/progs/mptcp_sockmap.c | 43 +++++ > 2 files changed, 193 insertions(+) > create mode 100644 tools/testing/selftests/bpf/progs/mptcp_sockmap.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c > b/tools/testing/selftests/bpf/prog_tests/mptcp.c > index f8eb7f9d4fd2..56c556f603cc 100644 > --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c > +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c > @@ -6,11 +6,14 @@ > #include <netinet/in.h> > #include <test_progs.h> > #include <unistd.h> > +#include <error.h> Do you use this new include? > #include "cgroup_helpers.h" > #include "network_helpers.h" > +#include "socket_helpers.h" > #include "mptcp_sock.skel.h" > #include "mptcpify.skel.h" > #include "mptcp_subflow.skel.h" > +#include "mptcp_sockmap.skel.h" > > #define NS_TEST "mptcp_ns" > #define ADDR_1 "10.0.1.1" > @@ -436,6 +439,151 @@ static void test_subflow(void) > close(cgroup_fd); > } > > +/* Test sockmap on MPTCP server handling non-mp-capable clients. */ > +static void test_sockmap_with_mptcp_fallback(struct mptcp_sockmap *skel) > +{ > + int listen_fd = -1, client_fd1 = -1, client_fd2 = -1; > + int server_fd1 = -1, server_fd2 = -1, sent, recvd; > + char snd[9] = "123456789"; > + char rcv[10]; > + > + /* start server with MPTCP enabled */ > + listen_fd = start_mptcp_server(AF_INET, NULL, 0, 0); > + if (!ASSERT_OK_FD(listen_fd, "redirect:start_mptcp_server")) > + return; > + > + skel->bss->trace_port = ntohs(get_socket_local_port(listen_fd)); > + skel->bss->sk_index = 0; > + /* create client without MPTCP enabled */ > + client_fd1 = connect_to_fd_opts(listen_fd, NULL); > + if (!ASSERT_OK_FD(client_fd1, "redirect:connect_to_fd")) > + goto end; > + > + server_fd1 = xaccept_nonblock(listen_fd, NULL, NULL); > + skel->bss->sk_index = 1; > + client_fd2 = connect_to_fd_opts(listen_fd, NULL); > + if (!ASSERT_OK_FD(client_fd2, "redirect:connect_to_fd")) > + goto end; > + > + server_fd2 = xaccept_nonblock(listen_fd, NULL, NULL); > + /* test normal redirect behavior: data sent by client_fd1 can be > + * received by client_fd2 > + */ > + skel->bss->redirect_idx = 1; > + sent = xsend(client_fd1, snd, sizeof(snd), 0); > + if (!ASSERT_EQ(sent, sizeof(snd), "redirect:xsend(client_fd1)")) > + goto end; > + > + /* try to recv more bytes to avoid truncation check */ > + recvd = recv_timeout(client_fd2, rcv, sizeof(rcv), MSG_DONTWAIT, 2); > + if (!ASSERT_EQ(recvd, sizeof(snd), "redirect:recv(client_fd2)")) > + goto end; > + > +end: > + if (client_fd1 > 1) > + close(client_fd1); > + if (client_fd2 > 1) > + close(client_fd2); > + if (server_fd1 > 0) > + close(server_fd1); > + if (server_fd2 > 0) > + close(server_fd2); Why do you check if it is above 0 or 1? Should you not always check if it is >= 0 for each fd? > + close(listen_fd); > +} > + > +/* Test sockmap rejection of MPTCP sockets - both server and client sides. */ > +static void test_sockmap_reject_mptcp(struct mptcp_sockmap *skel) > +{ > + int client_fd1 = -1, client_fd2 = -1; > + int listen_fd = -1, server_fd = -1; > + int err, zero = 0; > + > + /* start server with MPTCP enabled */ > + listen_fd = start_mptcp_server(AF_INET, NULL, 0, 0); > + if (!ASSERT_OK_FD(listen_fd, "start_mptcp_server")) In test_sockmap_with_mptcp_fallback(), you prefixed each error with 'redirect:'. Should you also have a different prefix here? 'sockmap-fb:' vs 'sockmap-mptcp:' eventually? > + return; > + > + skel->bss->trace_port = ntohs(get_socket_local_port(listen_fd)); > + skel->bss->sk_index = 0; > + /* create client with MPTCP enabled */ > + client_fd1 = connect_to_fd(listen_fd, 0); > + if (!ASSERT_OK_FD(client_fd1, "connect_to_fd client_fd1")) > + goto end; > + > + /* bpf_sock_map_update() called from sockops should reject MPTCP sk */ > + if (!ASSERT_EQ(skel->bss->helper_ret, -EOPNOTSUPP, "should reject")) > + goto end; So here, the client is connected, but sockmap doesn't operate on it, right? So most likely, the connection is stalled until the userspace realises that and takes an action? > + /* set trace_port = -1 to stop sockops */ > + skel->bss->trace_port = -1; What do you want to demonstrate from here? That without the sockmap injection, there are no new entries added? Is it worth checking that here? > + client_fd2 = connect_to_fd(listen_fd, 0); > + if (!ASSERT_OK_FD(client_fd2, "connect_to_fd client_fd2")) > + goto end; > + > + server_fd = xaccept_nonblock(listen_fd, NULL, NULL); > + err = bpf_map_update_elem(bpf_map__fd(skel->maps.sock_map), > + &zero, &server_fd, BPF_NOEXIST); > + if (!ASSERT_EQ(err, -EOPNOTSUPP, "server should be disallowed")) > + goto end; > + > + /* MPTCP client should also be disallowed */ > + err = bpf_map_update_elem(bpf_map__fd(skel->maps.sock_map), > + &zero, &client_fd1, BPF_NOEXIST); > + if (!ASSERT_EQ(err, -EOPNOTSUPP, "client should be disallowed")) > + goto end; > +end: > + if (client_fd1 > 0) > + close(client_fd1); > + if (client_fd2 > 0) > + close(client_fd2); > + if (server_fd > 0) > + close(server_fd); Same here: should it not be "*fd >= 0"? > + close(listen_fd); > +} (...) Cheers, Matt -- Sponsored by the NGI0 Core fund.
