Re: [PATCH net v4 3/3] selftests/bpf: Add mptcp test with sockmap

["Jiayuan Chen" <jiayuan.chen@linux.dev>]

November 5, 2025 at 22:40, "Matthieu Baerts" <matttbe@kernel.org mailto:matttbe@kernel.org?to=%22Matthieu%20Baerts%22%20%3Cmatttbe%40kernel.org%3E > wrote:


> 
> 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.

Thanks for the review. I've seen the feedback on the other patches(1/3, 2/3) and will fix them up.


> 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 <jiayuan.chen@linux.dev>
> >  ---
> >  .../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?

"EOPNOTSUPP" I used was defined in error.h.

> > 
> >  +
> >  +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?

My bad, ">=0" is correct.

> > 
> > + 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?

I will do it.

> > 
> > + 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?
>

It depends. Sockmap usually runs as a bypass. The user app (like Nginx)
has its own native forwarding logic, and sockmap just kicks in to accelerate
it. So in known cases, turning off sockmap falls back to the native logic.
But if there's no native logic, the connection just stalls.


> > 
> > + /* 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?

That's redundant. I'll drop it.


[...]
> >  + 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"?

I will fix it.

Thanks.