[PATCH mptcp-next v3 0/2] add distinct lockdep class for MPTCP sockets

[PATCH mptcp-next v3 0/2] 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.

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

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 (2):
  mptcp: give MPTCP sockets a distinct lockdep class
  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_test.c | 145 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 185 insertions(+), 19 deletions(-)
 create mode 100644 net/mptcp/sockopt_test.c

-- 
2.55.0

[PATCH mptcp-next v3 1/2] 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 v3 2/2] 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,
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.

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

diff --git a/net/mptcp/Kconfig b/net/mptcp/Kconfig
index a9f58edc354c..cdae7be802fb 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..b4b9c40be92f
--- /dev/null
+++ b/net/mptcp/sockopt_test.c
@@ -0,0 +1,145 @@
+// 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 = 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 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 server_addr;
+	struct kvec send_vec, recv_vec;
+	struct sockaddr_storage addr;
+	char send_buf[] = "MPTCP";
+	char recv_buf[32] = { 0 };
+	int err, reuse = 1;
+	int addrlen;
+
+	/* Initialize server address */
+	memset(&addr, 0, sizeof(addr));
+	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_loopback;
+		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_LOOPBACK);
+		addrlen = sizeof(struct sockaddr_in);
+	}
+
+	/* Create server socket and test SO_REUSEADDR */
+	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
+			       &server_sock);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	err = sock_common_setsockopt(server_sock, SOL_SOCKET, SO_REUSEADDR,
+				     KERNEL_SOCKPTR(&reuse), sizeof(reuse));
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, (int)server_sock->sk->sk_reuse, SK_CAN_REUSE);
+
+	err = kernel_bind(server_sock, (struct sockaddr_unsized *)&addr,
+			  addrlen);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	/* Get the assigned port */
+	err = kernel_getsockname(server_sock, (struct sockaddr *)&server_addr);
+	KUNIT_ASSERT_EQ(test, err, addrlen);
+
+	err = kernel_listen(server_sock, 5);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	/* Create client socket and test SO_REUSEADDR */
+	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
+			       &client_sock);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	err = sock_common_setsockopt(client_sock, SOL_SOCKET, SO_REUSEADDR,
+				     KERNEL_SOCKPTR(&reuse), sizeof(reuse));
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, (int)client_sock->sk->sk_reuse, SK_CAN_REUSE);
+
+	/* Connect to server */
+	err = kernel_connect(client_sock,
+			     (struct sockaddr_unsized *)&server_addr,
+			     addrlen, 0);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	/* Accept connection on server */
+	err = kernel_accept(server_sock, &accepted_sock, 0);
+	KUNIT_ASSERT_EQ(test, err, 0);
+
+	/* Send data from client */
+	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, strlen(send_buf));
+
+	/* Receive data on server */
+	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, strlen(send_buf));
+
+	/* Verify data */
+	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
+
+	sock_release(accepted_sock);
+	sock_release(client_sock);
+	sock_release(server_sock);
+}
+
+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 v3 2/2] mptcp: kunit: add sockopt test

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mptcp: kunit: kernel_bind() fails in mptcp_sockopt_test() when lo is down
- [Medium] mptcp: kunit: socket leak in mptcp_sockopt_test() on assertion failure
--

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

mptcp: kunit: add sockopt test

This commit adds a KUnit test suite for MPTCP socket options and lockdep
class separation across IPv4 and IPv6 sockets.

> diff --git a/net/mptcp/sockopt_test.c b/net/mptcp/sockopt_test.c
> new file mode 100644
> index 0000000000000..b4b9c40be92fe
> --- /dev/null
> +++ b/net/mptcp/sockopt_test.c
> @@ -0,0 +1,145 @@
> +// 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 = 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 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 server_addr;
> +	struct kvec send_vec, recv_vec;
> +	struct sockaddr_storage addr;
> +	char send_buf[] = "MPTCP";
> +	char recv_buf[32] = { 0 };
> +	int err, reuse = 1;
> +	int addrlen;
> +
> +	/* Initialize server address */
> +	memset(&addr, 0, sizeof(addr));
> +	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_loopback;
> +		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_LOOPBACK);
> +		addrlen = sizeof(struct sockaddr_in);
> +	}
> +
> +	/* Create server socket and test SO_REUSEADDR */
> +	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
> +			       &server_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	err = sock_common_setsockopt(server_sock, SOL_SOCKET, SO_REUSEADDR,
> +				     KERNEL_SOCKPTR(&reuse), sizeof(reuse));
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	KUNIT_EXPECT_EQ(test, (int)server_sock->sk->sk_reuse, SK_CAN_REUSE);
> +
> +	err = kernel_bind(server_sock, (struct sockaddr_unsized *)&addr,
> +			  addrlen);
> +	KUNIT_ASSERT_EQ(test, err, 0);

[Severity: Medium]
Will kernel_bind() fail with -EADDRNOTAVAIL when CONFIG_MPTCP_KUNIT_TEST=y
runs at boot time?

When built-in, kunit_run_all_tests() runs during kernel_init_freeable()
before userspace starts. During do_initcalls(), loopback_net_init()
registers init_net.loopback_dev via gen_lo_setup() with only IFF_LOOPBACK
set (not IFF_UP).

Because 127.0.0.1 (via inetdev_event()) and ::1 (via addrconf_notify() ->
addrconf_init_auto_addrs() -> init_loopback()) are only configured when the
loopback device transitions to NETDEV_UP, binding server_sock to
INADDR_LOOPBACK or in6addr_loopback in init_net will fail in __inet_bind()
or __inet6_bind() if init_net.loopback_dev is still down.

> +
> +	/* Get the assigned port */
> +	err = kernel_getsockname(server_sock, (struct sockaddr *)&server_addr);
> +	KUNIT_ASSERT_EQ(test, err, addrlen);
> +
> +	err = kernel_listen(server_sock, 5);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	/* Create client socket and test SO_REUSEADDR */
> +	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
> +			       &client_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	err = sock_common_setsockopt(client_sock, SOL_SOCKET, SO_REUSEADDR,
> +				     KERNEL_SOCKPTR(&reuse), sizeof(reuse));
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	KUNIT_EXPECT_EQ(test, (int)client_sock->sk->sk_reuse, SK_CAN_REUSE);
> +
> +	/* Connect to server */
> +	err = kernel_connect(client_sock,
> +			     (struct sockaddr_unsized *)&server_addr,
> +			     addrlen, 0);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	/* Accept connection on server */
> +	err = kernel_accept(server_sock, &accepted_sock, 0);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	/* Send data from client */
> +	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, strlen(send_buf));
> +
> +	/* Receive data on server */
> +	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, strlen(send_buf));
> +
> +	/* Verify data */
> +	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
> +
> +	sock_release(accepted_sock);
> +	sock_release(client_sock);
> +	sock_release(server_sock);
> +}

[Severity: Medium]
Can mptcp_sockopt_test() leak server_sock, client_sock, or accepted_sock if
any of the preceding KUNIT_ASSERT_EQ() checks in mptcp_sockopt_test() or
KUNIT_ASSERT_NOT_ERR_OR_NULL() in mptcp_check_lockdep_class() fail?

When a KUNIT_ASSERT_* macro fails, it invokes __kunit_abort() ->
kunit_try_catch_throw() and terminates the test thread immediately before
reaching the sock_release() calls at the end of mptcp_sockopt_test(),
leaving the allocated kernel sockets open in init_net. Should these sockets
be registered with kunit_add_action_or_reset() or cleaned up via
KUNIT_EXPECT_* error paths?

[ ... ]

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

Re: [PATCH mptcp-next v3 2/2] mptcp: kunit: add sockopt test

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mptcp_sockopt_test() in net/mptcp/sockopt_test.c binds to
  127.0.0.1 or ::1 in init_net (lines 31-61).
- [Medium] Under CONFIG_PROVE_LOCKING, mptcp_sockopt_test() passes
  accepted_sock->sk to mptcp_check_lockdep_class() (sockopt_test.c:109).
- [Low] The MPTCP_KUNIT_TEST help text in net/mptcp/Kconfig (line 31) was
  changed to "Currently covers the MPTCP crypto and token helpers, and…
- [Low] mptcp_sockopt_test() releases server_sock, client_sock and
  accepted_sock only at the end of the function (sockopt_test.c:112-114).

--

mptcp: kunit: add sockopt test

This adds a KUnit suite that creates kernel MPTCP sockets in init_net,
sets SO_REUSEADDR, connects over loopback and transfers data. With
CONFIG_PROVE_LOCKING it also checks that the client and accepted MPTCP
sockets use a different lockdep key than their first subflow. Both IPv4
and IPv6 are covered.

> diff --git a/net/mptcp/Kconfig b/net/mptcp/Kconfig
> index a9f58edc354c..cdae7be802fb 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.

[Severity: Low]
This isn't a bug, but the new help line has lost its trailing period, so
it runs into the "Only useful for kernel devs ..." sentence after it.

[ ... ]

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

[ ... ]

> +#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 = 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

[Severity: Medium]
What happens here if the accepted socket fell back to plain TCP?

mptcp_sockopt_test() passes accepted_sock->sk to this helper. The helper
casts it with mptcp_sk() and reads msk->first, and it never checks
sk_is_mptcp() or sk->sk_protocol first.

On passive fallback, mptcp_stream_accept() grafts the plain TCP subflow
onto the new socket:

net/mptcp/protocol.c:mptcp_stream_accept() {
    ...
tcpfallback:
		newsk->sk_kern_sock = arg->kern;
		lock_sock(newsk);
		__inet_accept(sock, newsock, newsk);
    ...
}

When that happens, accepted_sock->sk is a struct tcp_sock. msk->first then
reads unrelated tcp_sock bytes, and the resulting pointer is dereferenced
for ssk->sk_lock.dep_map.key. KUNIT_ASSERT_NOT_ERR_OR_NULL() only catches
NULL and ERR_PTR values.

Passive fallback does not need malformed input. Some examples:

- The client is in the active-disable window, so mptcp_connect() calls
  mptcp_early_fallback(..., MPTCP_MIB_MPCAPABLEACTIVEDISABLED) and sends
  a plain SYN.
- Token allocation fails in mptcp_connect().
- mptcp_sk_clone_init() fails in subflow_syn_recv_sock(), which then
  sets fallback = true.
- Another local process connects with plain TCP to the ephemeral
  listening port in init_net before the test client does.
  kernel_accept() takes whichever connection is queued first.

Send and recv still work over TCP in each of these cases, so the test
gets as far as this cast. The test also never checks that MPTCP was
actually negotiated.

Could the test confirm that accepted_sock->sk really is an MPTCP socket
before it dereferences msk->first?

[ ... ]

> +	err = kernel_bind(server_sock, (struct sockaddr_unsized *)&addr,
> +			  addrlen);
> +	KUNIT_ASSERT_EQ(test, err, 0);

[Severity: Medium]
Can this bind succeed when the suite is built in?

The test binds to 127.0.0.1 or ::1 in init_net but never brings lo up.
The kernel does not set IFF_UP on lo by itself. The only in-kernel path
that opens it is ic_open_devs(), and ip_auto_config() skips that unless
ip= is on the command line:

net/ipv4/ipconfig.c:ip_auto_config() {
    ...
	if (!ic_enable)
		return 0;
    ...
}

The loopback addresses are only installed after lo gets NETDEV_UP. Until
then, __inet_bind() fails here:

net/ipv4/af_inet.c:__inet_bind() {
    ...
	err = -EADDRNOTAVAIL;
	if (!inet_addr_valid_or_nonlocal(net, inet, addr->sin_addr.s_addr,
	                                 chk_addr_ret))
		goto out;
    ...
}

The IPv6 bind fails the same way.

Built-in suites run from kernel_init_freeable() through
kunit_run_all_tests(), right after do_basic_setup(). That is before
userspace has a chance to run "ip link set lo up".

MPTCP_KUNIT_TEST defaults to KUNIT_ALL_TESTS. Won't both the v4 and v6
cases fail in a normal kunit.py or KUNIT_ALL_TESTS run, before the
lockdep check is reached?

Until now MPTCP_KUNIT_TEST only covered pure unit tests (crypto and
token). Neither the commit message nor the Kconfig help says this test
has to run as a module after lo is up.

Other KUnit tests that need a live netdev open it themselves. For
example, net/mctp/test/utils.c calls dev_open(ndev, NULL).

[ ... ]

> +#ifdef CONFIG_PROVE_LOCKING
> +	mptcp_check_lockdep_class(test, client_sock->sk);
> +	mptcp_check_lockdep_class(test, accepted_sock->sk);
> +#endif
> +
> +	sock_release(accepted_sock);
> +	sock_release(client_sock);
> +	sock_release(server_sock);
> +}

[Severity: Low]
Does a failed assertion leak server_sock, client_sock and accepted_sock?

These sock_release() calls are the only cleanup in the test. Between
sock_create_kern() and here there are many KUNIT_ASSERT_*() checks,
including the one in mptcp_check_lockdep_class().

A failed assertion ends the test thread through kunit_try_catch_throw():

lib/kunit/try-catch.c:kunit_try_catch_throw() {
    ...
	try_catch->try_result = -EFAULT;
	kthread_exit(0);
}

That skips these releases. No cleanup is registered either, for example
through kunit_add_action_or_reset().

A failure after the server socket is created leaks that MPTCP socket and
its subflow. A later failure also leaks the client and accepted sockets,
plus the established loopback connection in init_net.

In a built-in run with lo down, the kernel_bind() assertion above already
fails, so server_sock leaks in both the v4 and v6 cases.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792378794.git.tanggeliang%40kylinos.cn