[PATCH mptcp-next v4 0/3] add distinct lockdep class for MPTCP sockets

[PATCH mptcp-next v4 0/3] add distinct lockdep class for MPTCP sockets

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The mptcp_nvme.sh selftest reports lockdep "possible recursive locking"
warnings when running NVMe over MPTCP. The issue occurs because MPTCP
sockets and their subflows share the same k-sk_lock-AF_INET lockdep class,
causing lockdep to flag false warnings in paths that hold the MPTCP socket
lock and then lock a subflow (setsockopt, connect, bind, worker send-ack).

This series fixes the issue by giving MPTCP sockets their own distinct
lockdep class (sk_lock-AF_INET-MPTCP for IPv4, sk_lock-AF_INET6-MPTCP for
IPv6), similar to how nvme-tcp handles it. This approach is more efficient
than reclassifying each subflow individually, properly supports IPv6, and
follows existing kernel patterns.

A KUnit test is included to verify the fix works correctly.

v4:
 - Include existing patch to propagate sk_reuse and sk_reuseport settings
   from MPTCP socket to subflows, and fix unconditional sk_bound_dev_if
   override in __mptcp_subflow_connect()
 - Enhanced KUnit test to cover SO_BINDTODEVICE and SO_BINDTOIFINDEX
   socket options in addition to SO_REUSEADDR
 - Refactored sockopt test into helper functions (mptcp_test_reuseaddr,
   mptcp_test_bindtodevice, mptcp_test_bindtoifindex) for better
   maintainability
 - Added proper socket cleanup using kunit_add_action_or_reset() to
   prevent resource leaks on test failures
 - Added READ_ONCE annotation for lockless access to msk->first
 - Added period to Kconfig help text

v3:
 - Assign separate lock classes for kernel and user MPTCP sockets to
   prevent lockdep from conflating their usage patterns (kernel sockets
   may nest with fs/storage locks, user sockets record mmap_lock
   dependencies through page faults)
 - Call mptcp_reclassify_socket() from mptcp_init_sock() for active sockets
   and from mptcp_sk_clone_init() before bh_lock_sock() for passive sockets
 - Remove duplicate lock reclassification from pm_kernel.c (PM listener
   sockets are now reclassified through mptcp_init_sock() path)
 - Enhanced KUnit test to cover both IPv4/IPv6 and active/passive sockets,
   with direct lockdep key comparison when CONFIG_PROVE_LOCKING is enabled
 - Update Kconfig help text to mention sockopt tests
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1792378794.git.tanggeliang@kylinos.cn/

v2:
 - Move reclassification to __mptcp_init_sock() to cover cloned sockets,
   as Matt suggested
 - Extract mptcp_reclassify_socket() helper for reuse in PM listener socket
   creation
 - Remove #ifdef CONFIG_DEBUG_LOCK_ALLOC wrapper as suggested by Matt
 - Update lock class names to msk_lock-AF_INET/msk_lock-AF_INET6
 - Note: checkpatch.pl warns about line length > 80 columns in pm_kernel.c,
   but kept for consistency with existing code style
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1792114102.git.tanggeliang@kylinos.cn/

v1:
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1792060837.git.tanggeliang@kylinos.cn/

Geliang Tang (3):
  mptcp: give MPTCP sockets a distinct lockdep class
  mptcp: inherit sk_reuse and fix dev bind override
  mptcp: kunit: add sockopt test

 net/mptcp/Kconfig        |   2 +-
 net/mptcp/Makefile       |   4 +-
 net/mptcp/pm_kernel.c    |  16 ----
 net/mptcp/protocol.c     |  37 +++++++-
 net/mptcp/sockopt.c      |   5 ++
 net/mptcp/sockopt_test.c | 184 +++++++++++++++++++++++++++++++++++++++
 net/mptcp/subflow.c      |   4 +-
 7 files changed, 232 insertions(+), 20 deletions(-)
 create mode 100644 net/mptcp/sockopt_test.c

-- 
2.55.0

[PATCH mptcp-next v4 1/3] mptcp: give MPTCP sockets a distinct lockdep class

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

MPTCP sockets and their subflows share the same k-sk_lock-AF_INET
lockdep class. When both are kernel sockets, lockdep flags every path
that holds the MPTCP socket lock and then locks a subflow (setsockopt,
connect, bind, worker send-ack) as "possible recursive locking".

Extract the lock reclassification logic from PM listener socket creation
into mptcp_reclassify_socket() helper. Assign distinct lock classes
based on both socket type (kernel or user) and address family (IPv4 or
IPv6) to prevent lockdep from conflating kernel MPTCP socket usage
(which may nest with fs/storage locks) with user-space socket API usage
(which records mmap_lock dependencies through page faults).

Call mptcp_reclassify_socket() from mptcp_init_sock() for actively
created sockets, and from mptcp_sk_clone_init() before bh_lock_sock()
for passively cloned sockets. The reclassification must happen before
the lock is held to avoid lockdep warnings. PM listener sockets are
automatically reclassified through the mptcp_init_sock() path when
created via sock_create_kern().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_kernel.c | 16 ----------------
 net/mptcp/protocol.c  | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..03603defdf6c 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -846,13 +846,9 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 	return ret;
 }
 
-static struct lock_class_key mptcp_slock_keys[2];
-static struct lock_class_key mptcp_keys[2];
-
 static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 					    struct mptcp_pm_addr_entry *entry)
 {
-	bool is_ipv6 = entry->addr.family == AF_INET6;
 	int addrlen = sizeof(struct sockaddr_in);
 	struct sockaddr_storage addr;
 	struct sock *newsk, *ssk;
@@ -868,18 +864,6 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	if (!newsk)
 		return -EINVAL;
 
-	/* The subflow socket lock is acquired in a nested to the msk one
-	 * in several places, even by the TCP stack, and this msk is a kernel
-	 * socket: lockdep complains. Instead of propagating the _nested
-	 * modifiers in several places, re-init the lock class for the msk
-	 * socket to an mptcp specific one.
-	 */
-	sock_lock_init_class_and_name(newsk,
-				      is_ipv6 ? "mlock-AF_INET6" : "mlock-AF_INET",
-				      &mptcp_slock_keys[is_ipv6],
-				      is_ipv6 ? "msk_lock-AF_INET6" : "msk_lock-AF_INET",
-				      &mptcp_keys[is_ipv6]);
-
 	lock_sock(newsk);
 	ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
 	release_sock(newsk);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 60cbd91be632..eae3f25dd3d4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3260,11 +3260,43 @@ static void mptcp_ca_reset(struct sock *sk)
 	icsk->icsk_ca_ops = NULL;
 }
 
+/* The subflow socket lock is acquired in a nested to the msk one
+ * in several places, even by the TCP stack: lockdep complains when
+ * both are kernel sockets. Instead of propagating the _nested modifiers
+ * in several places, re-init the lock class for the msk socket to an
+ * mptcp specific one. Separate lock classes are used for kernel and
+ * user sockets to prevent lockdep from conflating their usage patterns.
+ */
+static struct lock_class_key mptcp_slock_keys[2][2];
+static struct lock_class_key mptcp_keys[2][2];
+
+static void mptcp_reclassify_socket(struct sock *sk)
+{
+	bool is_ipv6 = sk->sk_family == AF_INET6;
+	const char *slock_name, *lock_name;
+	bool is_kern = !!sk->sk_kern_sock;
+
+	if (is_kern) {
+		slock_name = is_ipv6 ? "kmlock-AF_INET6" : "kmlock-AF_INET";
+		lock_name = is_ipv6 ? "kmsk_lock-AF_INET6" :
+				      "kmsk_lock-AF_INET";
+	} else {
+		slock_name = is_ipv6 ? "mlock-AF_INET6" : "mlock-AF_INET";
+		lock_name = is_ipv6 ? "msk_lock-AF_INET6" : "msk_lock-AF_INET";
+	}
+
+	sock_lock_init_class_and_name(sk, slock_name,
+				      &mptcp_slock_keys[is_kern][is_ipv6],
+				      lock_name, &mptcp_keys[is_kern][is_ipv6]);
+}
+
 static int mptcp_init_sock(struct sock *sk)
 {
 	struct net *net = sock_net(sk);
 	int ret;
 
+	mptcp_reclassify_socket(sk);
+
 	__mptcp_init_sock(sk);
 
 	if (!mptcp_is_enabled(net))
@@ -3765,13 +3797,16 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 				 struct request_sock *req)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
-	struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
+	struct sock *nsk = sk_clone(sk, GFP_ATOMIC, false);
 	struct mptcp_subflow_context *subflow;
 	struct mptcp_sock *msk;
 
 	if (!nsk)
 		return NULL;
 
+	mptcp_reclassify_socket(nsk);
+
+	bh_lock_sock(nsk);
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 	if (nsk->sk_family == AF_INET6)
 		inet_sk(nsk)->pinet6 = mptcp_inet6_sk(nsk);
-- 
2.55.0

[PATCH mptcp-next v4 2/3] mptcp: inherit sk_reuse and fix dev bind override

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Propagate sk_reuse and sk_reuseport from master socket to subflows, and
bump setsockopt_seq so later subflows pick up the changes.

This patch also fixes unconditional sk_bound_dev_if override in
__mptcp_subflow_connect(): only set it when local->ifindex is non-zero,
otherwise preserve the value inherited from the master socket via
sync_socket_options().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/sockopt.c | 5 +++++
 net/mptcp/subflow.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 922f6ae5c80c..952bc015eee8 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -335,6 +335,8 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname,
 				sk->sk_bound_dev_if = ssk->sk_bound_dev_if;
 			else if (optname == SO_BINDTOIFINDEX)
 				sk->sk_bound_dev_if = ssk->sk_bound_dev_if;
+
+			sockopt_seq_inc(msk);
 		}
 		release_sock(sk);
 		return ret;
@@ -1706,6 +1708,9 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
 		assign_bit(b, &inet_sk(ssk)->inet_flags, src & BIT(b));
 
 	WRITE_ONCE(inet_sk(ssk)->local_port_range, READ_ONCE(inet_sk(sk)->local_port_range));
+
+	ssk->sk_reuse = sk->sk_reuse;
+	ssk->sk_reuseport = sk->sk_reuseport;
 }
 
 void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..5bdfbe172665 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1713,7 +1713,9 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 	if (addr.ss_family == AF_INET6)
 		addrlen = sizeof(struct sockaddr_in6);
 #endif
-	ssk->sk_bound_dev_if = local->ifindex;
+	/* Only override the bound device if the path manager picked one. */
+	if (local->ifindex)
+		ssk->sk_bound_dev_if = local->ifindex;
 	err = kernel_bind(sf, (struct sockaddr_unsized *)&addr, addrlen);
 	if (err) {
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTXBINDERR);
-- 
2.55.0

[PATCH mptcp-next v4 3/3] mptcp: kunit: add sockopt test

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add a KUnit test to verify MPTCP socket options and lockdep class
separation. The test creates kernel MPTCP sockets, sets SO_REUSEADDR,
SO_BINDTODEVICE, and SO_BINDTOIFINDEX options via helper functions
(mptcp_test_reuseaddr, mptcp_test_bindtodevice, mptcp_test_bindtoifindex),
establishes a connection (bind, listen, connect, accept), transfers data,
and verifies that MPTCP sockets have distinct lockdep classes from their
subflows.

The test covers both IPv4 and IPv6 address families, and checks both
actively created sockets (client) and passively cloned sockets
(accepted) to ensure proper lockdep class assignment in all paths.

When CONFIG_PROVE_LOCKING is enabled, the test directly compares
lockdep keys between MPTCP sockets and their subflows to detect any
regression in lock class assignment.

The test uses INADDR_ANY instead of INADDR_LOOPBACK to avoid binding
failures when running during early boot before loopback device is up.
Socket cleanup is handled via kunit_add_action_or_reset() to prevent
resource leaks on test failures.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/Kconfig        |   2 +-
 net/mptcp/Makefile       |   4 +-
 net/mptcp/sockopt_test.c | 184 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+), 2 deletions(-)
 create mode 100644 net/mptcp/sockopt_test.c

diff --git a/net/mptcp/Kconfig b/net/mptcp/Kconfig
index a9f58edc354c..ee9e0a04d52a 100644
--- a/net/mptcp/Kconfig
+++ b/net/mptcp/Kconfig
@@ -28,7 +28,7 @@ config MPTCP_KUNIT_TEST
 	depends on KUNIT
 	default KUNIT_ALL_TESTS
 	help
-	  Currently covers the MPTCP crypto and token helpers.
+	  Currently covers the MPTCP crypto and token helpers, and sockopt tests.
 	  Only useful for kernel devs running KUnit test harness and are not
 	  for inclusion into a production build.
 
diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
index bd72b300a0f6..e9ae91cff940 100644
--- a/net/mptcp/Makefile
+++ b/net/mptcp/Makefile
@@ -10,7 +10,9 @@ obj-$(CONFIG_INET_MPTCP_DIAG) += mptcp_diag.o
 
 mptcp_crypto_test-objs := crypto_test.o
 mptcp_token_test-objs := token_test.o
-obj-$(CONFIG_MPTCP_KUNIT_TEST) += mptcp_crypto_test.o mptcp_token_test.o
+mptcp_sockopt_test-objs := sockopt_test.o
+obj-$(CONFIG_MPTCP_KUNIT_TEST) += mptcp_crypto_test.o mptcp_token_test.o \
+				  mptcp_sockopt_test.o
 
 obj-$(CONFIG_BPF_SYSCALL) += bpf.o
 
diff --git a/net/mptcp/sockopt_test.c b/net/mptcp/sockopt_test.c
new file mode 100644
index 000000000000..f8ce69911caf
--- /dev/null
+++ b/net/mptcp/sockopt_test.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <kunit/test.h>
+
+#include "protocol.h"
+
+#ifdef CONFIG_PROVE_LOCKING
+static void mptcp_check_lockdep_class(struct kunit *test, struct sock *sk)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct sock *ssk;
+
+	ssk = READ_ONCE(msk->first);
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ssk);
+	KUNIT_EXPECT_PTR_NE(test, sk->sk_lock.dep_map.key,
+			    ssk->sk_lock.dep_map.key);
+}
+#endif
+
+static void sock_release_wrapper(void *sock)
+{
+	sock_release(sock);
+}
+
+static void mptcp_test_reuseaddr(struct kunit *test, struct socket *sock)
+{
+	int reuse = 1;
+	int err;
+
+	err = sock_common_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
+				     KERNEL_SOCKPTR(&reuse), sizeof(reuse));
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, (int)sock->sk->sk_reuse, SK_CAN_REUSE);
+}
+
+static void mptcp_test_bindtodevice(struct kunit *test, struct socket *sock)
+{
+	struct net_device *lo = dev_get_by_name(&init_net, "lo");
+	char ifname[IFNAMSIZ];
+	int ifindex, err;
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, lo);
+	ifindex = lo->ifindex;
+	strscpy(ifname, lo->name, sizeof(ifname));
+	dev_put(lo);
+
+	err = sock_common_setsockopt(sock, SOL_SOCKET, SO_BINDTODEVICE,
+				     KERNEL_SOCKPTR(ifname),
+				     strlen(ifname) + 1);
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, sock->sk->sk_bound_dev_if, ifindex);
+}
+
+static void mptcp_test_bindtoifindex(struct kunit *test, struct socket *sock)
+{
+	struct net_device *lo = dev_get_by_name(&init_net, "lo");
+	int ifindex, err;
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, lo);
+	ifindex = lo->ifindex;
+	dev_put(lo);
+
+	err = sock_common_setsockopt(sock, SOL_SOCKET, SO_BINDTOIFINDEX,
+				     KERNEL_SOCKPTR(&ifindex), sizeof(ifindex));
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, sock->sk->sk_bound_dev_if, ifindex);
+}
+
+static void mptcp_sockopt_test(struct kunit *test, sa_family_t family)
+{
+	struct socket *server_sock, *client_sock, *accepted_sock;
+	struct msghdr send_msg = { 0 }, recv_msg = { 0 };
+	struct sockaddr_storage addr = { 0 };
+	struct kvec send_vec, recv_vec;
+	char send_buf[] = "MPTCP";
+	char recv_buf[32] = { 0 };
+	int err, addrlen;
+
+	if (family == AF_INET6) {
+		struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)&addr;
+
+		addr6->sin6_family = AF_INET6;
+		addr6->sin6_port = htons(0);
+		addr6->sin6_addr = in6addr_any;
+		addrlen = sizeof(struct sockaddr_in6);
+	} else {
+		struct sockaddr_in *addr4 = (struct sockaddr_in *)&addr;
+
+		addr4->sin_family = AF_INET;
+		addr4->sin_port = htons(0);
+		addr4->sin_addr.s_addr = htonl(INADDR_ANY);
+		addrlen = sizeof(struct sockaddr_in);
+	}
+
+	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
+			       &server_sock);
+	KUNIT_ASSERT_EQ(test, err, 0);
+	err = kunit_add_action_or_reset(test, sock_release_wrapper,
+					server_sock);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	mptcp_test_reuseaddr(test, server_sock);
+
+	mptcp_test_bindtodevice(test, server_sock);
+
+	err = kernel_bind(server_sock, (struct sockaddr_unsized *)&addr,
+			  addrlen);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	err = kernel_getsockname(server_sock, (struct sockaddr *)&addr);
+	KUNIT_ASSERT_EQ(test, err, addrlen);
+
+	err = kernel_listen(server_sock, 5);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
+			       &client_sock);
+	KUNIT_ASSERT_EQ(test, err, 0);
+	err = kunit_add_action_or_reset(test, sock_release_wrapper,
+					client_sock);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	mptcp_test_bindtoifindex(test, client_sock);
+
+	err = kernel_connect(client_sock,
+			     (struct sockaddr_unsized *)&addr,
+			     addrlen, 0);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	err = kernel_accept(server_sock, &accepted_sock, 0);
+	KUNIT_ASSERT_EQ(test, err, 0);
+	err = kunit_add_action_or_reset(test, sock_release_wrapper,
+					accepted_sock);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	send_vec.iov_base = send_buf;
+	send_vec.iov_len = strlen(send_buf);
+	err = kernel_sendmsg(client_sock, &send_msg, &send_vec, 1,
+			     strlen(send_buf));
+	KUNIT_ASSERT_EQ(test, err, (int)strlen(send_buf));
+
+	recv_vec.iov_base = recv_buf;
+	recv_vec.iov_len = sizeof(recv_buf);
+	err = kernel_recvmsg(accepted_sock, &recv_msg, &recv_vec, 1,
+			     sizeof(recv_buf), 0);
+	KUNIT_ASSERT_EQ(test, err, (int)strlen(send_buf));
+
+	KUNIT_EXPECT_STREQ(test, recv_buf, send_buf);
+
+#ifdef CONFIG_PROVE_LOCKING
+	mptcp_check_lockdep_class(test, client_sock->sk);
+	mptcp_check_lockdep_class(test, accepted_sock->sk);
+#endif
+}
+
+static void mptcp_sockopt_test_v4(struct kunit *test)
+{
+	mptcp_sockopt_test(test, AF_INET);
+}
+
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+static void mptcp_sockopt_test_v6(struct kunit *test)
+{
+	mptcp_sockopt_test(test, AF_INET6);
+}
+#endif
+
+static struct kunit_case mptcp_sockopt_test_cases[] = {
+	KUNIT_CASE(mptcp_sockopt_test_v4),
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	KUNIT_CASE(mptcp_sockopt_test_v6),
+#endif
+	{}
+};
+
+static struct kunit_suite mptcp_sockopt_suite = {
+	.name = "mptcp-sockopt",
+	.test_cases = mptcp_sockopt_test_cases,
+};
+
+kunit_test_suite(mptcp_sockopt_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("KUnit tests for MPTCP sockopt");
-- 
2.55.0

Re: [PATCH mptcp-next v4 2/3] mptcp: inherit sk_reuse and fix dev bind override

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: wrong sk_uid during subflow bind when syncing sk_reuseport
--

commit 1d41a74d8cd4f5351a1f4186bfb92fe0ffeada01
Author: Geliang Tang <geliang@kernel.org>

mptcp: inherit sk_reuse and fix dev bind override

This commit propagates sk_reuse and sk_reuseport from the master socket to
subflows, increments setsockopt_seq when SOL_SOCKET options are set, and
avoids overriding sk_bound_dev_if in __mptcp_subflow_connect() when
local->ifindex is zero.

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 922f6ae5c80cb..952bc015eee87 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c

[ ... ]

> @@ -1706,6 +1708,9 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
>  		assign_bit(b, &inet_sk(ssk)->inet_flags, src & BIT(b));
>  
>  	WRITE_ONCE(inet_sk(ssk)->local_port_range, READ_ONCE(inet_sk(sk)->local_port_range));
> +
> +	ssk->sk_reuse = sk->sk_reuse;
> +	ssk->sk_reuseport = sk->sk_reuseport;
>  }
>  
>  void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk)

[Severity: High]
When syncing sk_reuseport to ssk here in sync_socket_options(), does ssk
have the right sk_uid before kernel_bind() runs in
__mptcp_subflow_connect()?

In mptcp_subflow_create_socket(), sock_create_kern() initializes
SOCK_INODE(sf)->i_uid to current_fsuid(), and sock_init_data() copies that
into sf->sk->sk_uid. Although mptcp_subflow_create_socket() then updates
SOCK_INODE(sf)->i_uid to the master socket's UID:

net/mptcp/subflow.c:mptcp_subflow_create_socket() {
	...
	SOCK_INODE(sf)->i_ino = SOCK_INODE(sk->sk_socket)->i_ino;
	SOCK_INODE(sf)->i_uid = SOCK_INODE(sk->sk_socket)->i_uid;
	SOCK_INODE(sf)->i_gid = SOCK_INODE(sk->sk_socket)->i_gid;
	...
}

ssk->sk_uid is not updated until mptcp_sock_graft() calls sk_set_socket()
at the end of __mptcp_subflow_connect(), after kernel_bind() and
kernel_connect():

net/mptcp/subflow.c:__mptcp_subflow_connect() {
	...
	err = mptcp_subflow_create_socket(sk, local->addr.family, &sf);
	...
	err = kernel_bind(sf, (struct sockaddr_unsized *)&addr, addrlen);
	...
	err = kernel_connect(sf, (struct sockaddr_unsized *)&addr, addrlen, O_NONBLOCK);
	...
	mptcp_sock_graft(ssk, sk->sk_socket);
}

When additional subflows are created asynchronously via mptcp_worker or a
privileged path manager daemon, current_fsuid() is 0 (root) rather than the
UID of the owning MPTCP socket.

During kernel_bind() -> inet_csk_get_port(), the SO_REUSEPORT checks in
sk_reuseport_match(), inet_csk_bind_conflict() -> inet_bind_conflict(), and
inet_csk_update_fastreuse() all read sk_uid(ssk):

net/ipv4/inet_connection_sock.c:inet_bind_conflict() {
	...
		} else if (!reuseport_ok || !sk->sk_reuseport ||
			   !sk2->sk_reuseport || !reuseport_cb_ok ||
			   (sk2->sk_state != TCP_TIME_WAIT &&
			    !uid_eq(uid, sk_uid(sk2)))) {
			return true;
		}
	...
}

Because sk_uid(ssk) is still 0 during kernel_bind(), won't an unprivileged
user's subflow fail with -EADDRINUSE when binding to the same port as the
initial subflow or another SO_REUSEPORT socket owned by that user, while
also matching against root-owned SO_REUSEPORT sockets and recording
tb->fastuid as 0 in inet_csk_update_fastreuse()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792470313.git.tanggeliang@kylinos.cn?part=2

Re: [PATCH mptcp-next v4 3/3] mptcp: kunit: add sockopt test

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: kunit: mptcp_sockopt_test() fails in kernel_connect() when lo is down
--

commit d196ed613cae4d93a15c8207b2a196e0356c9e7d
Author: Geliang Tang <geliang@kernel.org>

mptcp: kunit: add sockopt test

This commit adds a KUnit test for MPTCP socket options (SO_REUSEADDR,
SO_BINDTODEVICE, and SO_BINDTOIFINDEX) and lockdep class separation between
MPTCP sockets and their subflows across IPv4 and IPv6.

> diff --git a/net/mptcp/sockopt_test.c b/net/mptcp/sockopt_test.c
> new file mode 100644
> index 0000000000000..f8ce69911cafd
> --- /dev/null
> +++ b/net/mptcp/sockopt_test.c
> @@ -0,0 +1,184 @@

[ ... ]

> +static void mptcp_sockopt_test(struct kunit *test, sa_family_t family)
> +{
> +	struct socket *server_sock, *client_sock, *accepted_sock;
> +	struct msghdr send_msg = { 0 }, recv_msg = { 0 };
> +	struct sockaddr_storage addr = { 0 };
> +	struct kvec send_vec, recv_vec;
> +	char send_buf[] = "MPTCP";
> +	char recv_buf[32] = { 0 };
> +	int err, addrlen;
> +
> +	if (family == AF_INET6) {
> +		struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)&addr;
> +
> +		addr6->sin6_family = AF_INET6;
> +		addr6->sin6_port = htons(0);
> +		addr6->sin6_addr = in6addr_any;
> +		addrlen = sizeof(struct sockaddr_in6);
> +	} else {
> +		struct sockaddr_in *addr4 = (struct sockaddr_in *)&addr;
> +
> +		addr4->sin_family = AF_INET;
> +		addr4->sin_port = htons(0);
> +		addr4->sin_addr.s_addr = htonl(INADDR_ANY);
> +		addrlen = sizeof(struct sockaddr_in);
> +	}
> +
> +	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
> +			       &server_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +	err = kunit_add_action_or_reset(test, sock_release_wrapper,
> +					server_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	mptcp_test_reuseaddr(test, server_sock);
> +
> +	mptcp_test_bindtodevice(test, server_sock);
> +
> +	err = kernel_bind(server_sock, (struct sockaddr_unsized *)&addr,
> +			  addrlen);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	err = kernel_getsockname(server_sock, (struct sockaddr *)&addr);
> +	KUNIT_ASSERT_EQ(test, err, addrlen);
> +
> +	err = kernel_listen(server_sock, 5);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
> +			       &client_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +	err = kunit_add_action_or_reset(test, sock_release_wrapper,
> +					client_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	mptcp_test_bindtoifindex(test, client_sock);
> +
> +	err = kernel_connect(client_sock,
> +			     (struct sockaddr_unsized *)&addr,
> +			     addrlen, 0);
> +	KUNIT_ASSERT_EQ(test, err, 0);

[Severity: Medium]
The commit message notes that INADDR_ANY is used instead of INADDR_LOOPBACK
to avoid failures when running during early boot before the loopback device
is up. However, will kernel_connect() in mptcp_sockopt_test() still fail
when CONFIG_MPTCP_KUNIT_TEST=y runs built-in at early boot?

When kunit_run_all_tests() executes from kernel_init_freeable(), the "lo"
device in init_net has been registered by loopback_net_init() without
IFF_UP set, so the NETDEV_UP handlers in inetdev_event() and
addrconf_notify() -> init_loopback() have not yet configured 127.0.0.1/8 or
::1/128.

While binding server_sock to INADDR_ANY or in6addr_any allows kernel_bind()
and kernel_listen() to succeed, client_sock is bound to lo->ifindex via
mptcp_test_bindtoifindex() before calling kernel_connect():

For AF_INET, mptcp_connect() -> tcp_v4_connect() -> ip_route_connect() ->
ip_route_output_key_hash_rcu() checks fl4->flowi4_oif in net/ipv4/route.c:

	if (fl4->flowi4_oif) {
		dev_out = dev_get_by_index_rcu(net, fl4->flowi4_oif);
		rth = ERR_PTR(-ENODEV);
		if (!dev_out)
			goto out;

		/* RACE: Check return value of inet_select_addr instead. */
		if (!(dev_out->flags & IFF_UP) || !__in_dev_get_rcu(dev_out)) {
			rth = ERR_PTR(-ENETUNREACH);
			goto out;
		}

Because "lo" does not have IFF_UP set, route lookup fails with
-ENETUNREACH.

For AF_INET6, mptcp_connect() -> tcp_v6_connect() rewrites in6addr_any to
in6addr_loopback in net/ipv6/tcp_ipv6.c:

	if (ipv6_addr_any(&usin->sin6_addr)) {
		if (ipv6_addr_v4mapped(&sk->sk_v6_rcv_saddr))
			ipv6_addr_set_v4mapped(htonl(INADDR_LOOPBACK),
					       &usin->sin6_addr);
		else
			usin->sin6_addr = in6addr_loopback;
	}

Subsequent route and source-address lookup via ip6_dst_lookup_flow() ->
ip6_dst_lookup_tail() -> ip6_route_get_saddr() -> ipv6_dev_get_saddr() then
fails with -EADDRNOTAVAIL or -ENETUNREACH because "lo" has no IPv6 address
or route while down (and __dev_queue_xmit() also drops packets with
-ENETDOWN when !(dev->flags & IFF_UP)).

Should the test run in a dedicated network namespace with its loopback
device brought up, or ensure "lo" is up before connecting?

[ ... ]

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("KUnit tests for MPTCP sockopt");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792470313.git.tanggeliang@kylinos.cn?part=3