mptcp: Fix conflicts between MPTCP and sockmap

The sockmap feature allows bpf syscall from userspace using , or based

on bpf sockops, replacing the sk_prot of sockets during protocol stack processing with sockmap's custom read/write interfaces.
'''
tcp_rcv_state_process()
subflow_syn_recv_sock()

    tcp_init_transfer(BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB)
      bpf_skops_established       <== sockops
        bpf_sock_map_update(sk)   <== call bpf helper
          tcp_bpf_update_proto()  <== update sk_prot

'''
Consider two scenarios:

1. When the server has MPTCP enabled and the client also requests MPTCP, the sk passed to the BPF program is a subflow sk. Since subflows only handle partial data, replacing their sk_prot is meaningless and will cause traffic disruption.

2. When the server has MPTCP enabled but the client sends a TCP SYN without MPTCP, subflow_syn_recv_sock() performs a fallback on the subflow, replacing the subflow sk's sk_prot with the native sk_prot. '''
subflow_ulp_fallback()
subflow_drop_ctx()
mptcp_subflow_ops_undo_override()
'''
Subsequently, accept::mptcp_stream_accept::mptcp_fallback_tcp_ops() converts the subflow to plain TCP.

For the first case, we should prevent it from being combined with sockmap by setting sk_prot->psock_update_sk_prot to NULL, which will be blocked by sockmap's own flow.

For the second case, since subflow_syn_recv_sock() has already restored sk_prot to native tcp_prot/tcpv6_prot, no further action is needed.

Fixes 0b4f33def7bb ("mptcp: fix tcp fallback crash")

The sockmap feature allows bpf syscall from userspace, or based on bpf sockops, replacing the sk_prot of sockets during protocol stack processing with sockmap's custom read/write interfaces.
'''
tcp_rcv_state_process()
syn_recv_sock()/subflow_syn_recv_sock()

    tcp_init_transfer(BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB)
      bpf_skops_established       <== sockops
        bpf_sock_map_update(sk)   <== call bpf helper
          tcp_bpf_update_proto()  <== update sk_prot

'''

When the server has MPTCP enabled but the client sends a TCP SYN without MPTCP, subflow_syn_recv_sock() performs a fallback on the subflow, replacing the subflow sk's sk_prot with the native sk_prot. '''
subflow_syn_recv_sock()
subflow_ulp_fallback()
subflow_drop_ctx()
mptcp_subflow_ops_undo_override()
'''

Then, this subflow can be normally used by sockmap, which replaces the native sk_prot with sockmap's custom sk_prot. The issue occurs when the user executes accept::mptcp_stream_accept::mptcp_fallback_tcp_ops(). Here, it uses sk->sk_prot to compare with the native sk_prot, but this is incorrect when sockmap is used, as we may incorrectly set sk->sk_socket->ops.

This fix uses the more generic sk_family for the comparison instead.

Additionally, this also prevents a WARNING from occurring:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 388 at net/mptcp/protocol.c:68 \
mptcp_stream_accept+0x34c/0x380
Modules linked in:
RIP: 0010:mptcp_stream_accept+0x34c/0x380
RSP: 0018:ffffc90000cf3cf8 EFLAGS: 00010202
PKRU: 55555554
Call Trace:
 <TASK>
 do_accept+0xeb/0x190
 ? __x64_sys_pselect6+0x61/0x80
 ? _raw_spin_unlock+0x12/0x30
 ? alloc_fd+0x11e/0x190
 __sys_accept4+0x8c/0x100
 __x64_sys_accept+0x1f/0x30
 x64_sys_call+0x202f/0x20f0
 do_syscall_64+0x72/0x9a0
 ? switch_fpu_return+0x60/0xf0
 ? irqentry_exit_to_user_mode+0xdb/0x1e0
 ? irqentry_exit+0x3f/0x50
 ? clear_bhb_loop+0x50/0xa0
 ? clear_bhb_loop+0x50/0xa0
 ? clear_bhb_loop+0x50/0xa0
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
 </TASK>
---[ end trace 0000000000000000 ]---

result from ./scripts/decode_stacktrace.sh:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 337 at net/mptcp/protocol.c:68 mptcp_stream_accept \
(net-next/net/mptcp/protocol.c:4005)
Modules linked in:

...

PKRU: 55555554

Call Trace:
<TASK>

do_accept (net-next/net/socket.c:1989)
__sys_accept4 (net-next/net/socket.c:2028 net-next/net/socket.c:2057) __x64_sys_accept (net-next/net/socket.c:2067)
x64_sys_call (net-next/arch/x86/entry/syscall_64.c:41) do_syscall_64 (net-next/arch/x86/entry/syscall_64.c:63 \
net-next/arch/x86/entry/syscall_64.c:94) entry_SYSCALL_64_after_hwframe (net-next/arch/x86/entry/entry_64.S:130)

RIP: 0033:0x7f87ac92b83d

---[ end trace 0000000000000000 ]---

Fixes 0b4f33def7bb ("mptcp: fix tcp fallback crash")

Cc <[email protected]>

Signed-off-by Jiayuan Chen <[email protected]>

Reviewed-by Jakub Sitnicki <[email protected]>

---
 net/mptcp/protocol.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

net/mptcp/protocol.c +0 -0

index 4cd5df01446e..b5e5e130b158 100644
@@ -61,11 +61,13 @@ static u64 mptcp_wnd_end(const struct mptcp_sock *msk)
 
 static const struct proto_ops *mptcp_fallback_tcp_ops(const struct sock *sk)

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

tools/testing/selftests/bpf/prog_tests/mptcp.c +0 -0

index f8eb7f9d4fd2..56c556f603cc 100644

tools/testing/selftests/bpf/prog_tests/mptcp.c +41 -0

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

tools/testing/selftests/bpf/prog_tests/mptcp.c +12 -0

+	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),

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.

Quoted 15 lines

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

Quoted 10 lines

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?

Quoted 12 lines

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

Quoted 15 lines

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

Quoted 14 lines

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

Quoted 4 lines

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

Quoted 7 lines

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

Quoted 1 line · Matthieu Baerts

Quoted 6 lines

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.

Quoted 26 lines · Jiayuan Chen

Quoted 25 lines

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>

Quoted 1 line

Do you use this new include?

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

Quoted 13 lines

Quoted 4 lines

+ return;
+
+ skel->bss->trace_port = ntohs(get_socket_local_port(listen_fd));
+ skel->bss->sk_index = 0;

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 8 lines · Jiayuan Chen

Quoted 3 lines · Matthieu Baerts

Hi Jiayuan,

Quoted 8 lines

Quoted 5 lines

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.

Thanks!

Quoted 31 lines

Quoted 4 lines · Jiayuan Chen

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>

Quoted 3 lines

Do you use this new include?

Quoted 2 lines

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

Ah OK. I usually only include 'error.h' to use 'error()'. Is it not 'errno.h' (or 'linux/errno.h') you want instead?

I'm just surprised it is not already included but another one above. But OK if it is not.

Quoted 7 lines

Quoted 6 lines

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?

Quoted 5 lines

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.

Good to know, thanks!

So MPTCP request might still be handled by the "native logic" if any?

Hi Jiayuan,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Script error! ❓
- KVM Validation: btf-debug (only bpftest_all): Script error! ❓

Overall, we encountered a warning [1] that can be triggered by running the selftest I provided.

sockmap works by replacing sk_data_ready, recvmsg, sendmsg operations and implementing fast socket-level forwarding logic:
1. Users can obtain file descriptors through userspace socket()/accept() interfaces, then call BPF syscall to perform these replacements.
2. Users can also use the bpf_sock_hash_update helper (in sockops programs) to replace handlers when TCP connections enter ESTABLISHED state (BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB/BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB)

However, when combined with MPTCP, an issue arises: MPTCP creates subflow sk's and performs TCP handshakes, so the BPF program obtains subflow sk's and may incorrectly replace their sk_prot. We need to reject such operations. In patch 1, we set psock_update_sk_prot to NULL in the subflow's custom sk_prot.

Additionally, if the server's listening socket has MPTCP enabled and the client's TCP also uses MPTCP, we should allow the combination of subflow and sockmap. This is because the latest Golang programs have enabled MPTCP for listening sockets by default [2]. For programs already using sockmap, upgrading Golang should not cause sockmap functionality to fail.