[RFC PATCH mptcp-next v2 0/3] mptcp: pm: kernel: retry subflows closed after an error

[RFC PATCH mptcp-next v2 0/3] mptcp: pm: kernel: retry subflows closed after an error

[Anil Kaushik]

A subflow closed after an error is never re-created, even once the path
is back (#440).

v2 has no extra list or timer. The PM opens a new subflow to the same
addresses right away, and if it fails, the same socket is used for the
next try. A new sysctl, net.mptcp.subflow_retries, sets the max tries.

Is a sysctl fine, or should it be a netlink setting?

Patch 1: I saw the EAGAIN error only with the new tests, also with the
retries disabled.

Changes in v2:
 - use the subflow sockets, no list or timer
 - sysctl is now the max number of tries
 - code moved to pm_kernel.c
 - Fixes tag on patch 1
 - one selftest with netfilter
 - Assisted-by on all patches

v1: https://lore.kernel.org/mptcp/20261012171740.464517-1-anilkaushikwireless@gmail.com/

Anil Kaushik (3):
  selftests: mptcp: connect: retry write() on EAGAIN
  mptcp: pm: kernel: retry subflows closed after an error
  selftests: mptcp: join: validate subflow retry after link failure

 Documentation/networking/mptcp-sysctl.rst     |  15 +++
 net/mptcp/ctrl.c                              |  13 ++
 net/mptcp/mib.c                               |   1 +
 net/mptcp/mib.h                               |   1 +
 net/mptcp/pm_kernel.c                         | 116 ++++++++++++++++++
 net/mptcp/protocol.c                          |   9 ++
 net/mptcp/protocol.h                          |  20 +++
 net/mptcp/subflow.c                           |  53 ++++++++
 .../selftests/net/mptcp/mptcp_connect.c       |   8 ++
 .../testing/selftests/net/mptcp/mptcp_join.sh |  90 ++++++++++++++
 10 files changed, 326 insertions(+)

-- 
2.25.1

[RFC PATCH mptcp-next v2 1/3] selftests: mptcp: connect: retry write() on EAGAIN

[Anil Kaushik]

mptcp_connect treats any write() error as fatal. But write() can fail
with EAGAIN even after poll() said the socket was writable: the MPTCP
send buffer shrinks when a subflow is closed.

The tool then exits and the transfer fails on both sides. Instead, go
back to poll() and wait for more space.

Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp")
Assisted-by: LLM
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 178d98d91fea..5bcebc721864 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -711,6 +711,14 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 
 				bw = do_rnd_write(peerfd, winfo->buf + winfo->off, winfo->len);
 				if (bw < 0) {
+					/* POLLOUT does not guarantee the next write
+					 * will not block: the MPTCP send buffer can
+					 * shrink in between, e.g. when a subflow is
+					 * closed. Wait for more space.
+					 */
+					if (errno == EAGAIN)
+						continue;
+
 					/* expected reset, continue to read */
 					if (cfg_rcv_trunc &&
 					    (errno == ECONNRESET ||
-- 
2.25.1

[RFC PATCH mptcp-next v2 2/3] mptcp: pm: kernel: retry subflows closed after an error

[Anil Kaushik]

With the in-kernel PM, a subflow closed after an error, e.g. a timeout
when a path silently drops packets, is never re-created, even once the
path works again. For example, with a main link and a backup link,
connections move to the backup after an outage but never come back.

Add an opt-in sysctl, net.mptcp.subflow_retries (0 by default): the
maximum number of attempts to re-create such a subflow. When TCP closes
a subflow created by this side after an error, the PM creates a new one
with the same addresses right away. Its SYN retransmissions space out
the attempts, so no extra state or timer is needed. If it cannot
connect, the same socket is used for the next attempt. The number of
attempts is kept in the subflow context, and counted from scratch once
a re-created subflow is established. A subflow the peer refused is not
retried.

A new counter, SubflowRetry, counts the attempts.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/440
Assisted-by: LLM
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---
 Documentation/networking/mptcp-sysctl.rst |  15 +++
 net/mptcp/ctrl.c                          |  13 +++
 net/mptcp/mib.c                           |   1 +
 net/mptcp/mib.h                           |   1 +
 net/mptcp/pm_kernel.c                     | 116 ++++++++++++++++++++++
 net/mptcp/protocol.c                      |   9 ++
 net/mptcp/protocol.h                      |  20 ++++
 net/mptcp/subflow.c                       |  53 ++++++++++
 8 files changed, 228 insertions(+)

diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
index b9b5f58e0625..490ed4a20ce7 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -149,6 +149,21 @@ stale_loss_cnt - INTEGER
 
 	Default: 4
 
+subflow_retries - INTEGER
+	Maximum number of attempts to re-create a subflow closed by TCP after
+	an error, e.g. a timeout or an unreachable path, while the MPTCP
+	connection is still established. The in-kernel path manager creates a
+	new subflow with the same local and remote addresses: its SYN
+	retransmissions, see tcp_syn_retries, space out the attempts. If it
+	cannot connect, the same socket is used for the next attempt. The
+	count starts again once a re-created subflow is established. Only
+	subflows initiated by this side are re-created, within the limits set
+	for the path manager. A subflow refused by the peer is not retried.
+
+	0 disables the retries. This is a per-namespace sysctl.
+
+	Default: 0
+
 syn_retrans_before_tcp_fallback - INTEGER
 	The number of SYN + MP_CAPABLE retransmissions before falling back to
 	TCP, i.e. dropping the MPTCP options. In other words, if all the packets
diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 63c5747f0f63..37d96f89c4b1 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -34,6 +34,7 @@ struct mptcp_pernet {
 	atomic_t active_disable_times;
 	unsigned long active_disable_stamp;
 	u8 syn_retrans_before_tcp_fallback;
+	u8 subflow_retries;
 	u8 mptcp_enabled;
 	u8 checksum_enabled;
 	u8 allow_join_initial_addr_port;
@@ -73,6 +74,11 @@ unsigned int mptcp_stale_loss_cnt(const struct net *net)
 	return mptcp_get_pernet(net)->stale_loss_cnt;
 }
 
+u8 mptcp_subflow_retries(const struct net *net)
+{
+	return READ_ONCE(mptcp_get_pernet(net)->subflow_retries);
+}
+
 unsigned int mptcp_close_timeout(const struct sock *sk)
 {
 	if (sock_flag(sk, SOCK_DEAD))
@@ -377,6 +383,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.extra1       = SYSCTL_ZERO,
 		.extra2       = SYSCTL_ONE
 	},
+	{
+		.procname = "subflow_retries",
+		.maxlen = sizeof(u8),
+		.mode = 0644,
+		.proc_handler = proc_dou8vec_minmax,
+	},
 };
 
 static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
@@ -405,6 +417,7 @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
 	table[11].data = &pernet->path_manager;
 	/* table[12] is for available_path_managers which is read-only info */
 	table[13].data = &pernet->add_addr_v6_port_drop_ts;
+	table[14].data = &pernet->subflow_retries;
 
 	hdr = register_net_sysctl_sz(net, MPTCP_SYSCTL_PATH, table,
 				     ARRAY_SIZE(mptcp_sysctl_table));
diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 608cb568897c..5263ac6f2c43 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -95,6 +95,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("BacklogDrop", MPTCP_MIB_BACKLOGDROP),
 	SNMP_MIB_ITEM("RcvPruned", MPTCP_MIB_RCVPRUNED),
 	SNMP_MIB_ITEM("OFOPruned", MPTCP_MIB_OFOPRUNED),
+	SNMP_MIB_ITEM("SubflowRetry", MPTCP_MIB_SUBFLOWRETRY),
 };
 
 /* mptcp_mib_alloc - allocate percpu mib counters
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 1ebdb55e9534..1359839f4bbe 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -98,6 +98,7 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_BACKLOGDROP,		/* Backlog over memory limit */
 	MPTCP_MIB_RCVPRUNED,		/* Dropped due to memory constraints */
 	MPTCP_MIB_OFOPRUNED,		/* MPTCP-level OoO queue pruned */
+	MPTCP_MIB_SUBFLOWRETRY,		/* Retried a subflow closed on error */
 	__MPTCP_MIB_MAX
 };
 
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..c9199727c88c 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1591,6 +1591,122 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
 	return true;
 }
 
+static bool mptcp_pm_retry_path_error(int err)
+{
+	switch (err) {
+	case ETIMEDOUT:
+	case EHOSTUNREACH:
+	case ENETUNREACH:
+	case EHOSTDOWN:
+	case ENETDOWN:
+		return true;
+	}
+
+	return false;
+}
+
+/* A subflow initiated by this side and closed by TCP after an error can be
+ * re-created with the same addresses, see the 'subflow_retries' sysctl. The
+ * SYN retransmissions of the new subflow space out the attempts. Called with
+ * the msk socket lock held, before the subflow is closed.
+ */
+bool mptcp_pm_kernel_retry_check(struct mptcp_sock *msk,
+				 const struct mptcp_subflow_context *subflow,
+				 struct mptcp_pm_retry *retry)
+{
+	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+	bool established = READ_ONCE(subflow->fully_established);
+	struct sock *sk = (struct sock *)msk;
+	int err = READ_ONCE(ssk->sk_err);
+
+	if (!mptcp_pm_is_kernel(msk) || !err ||
+	    sk->sk_state != TCP_ESTABLISHED || __mptcp_check_fallback(msk))
+		return false;
+
+	/* mptcp_close_ssk() will not handle it again, e.g. the initial
+	 * subflow, kept in the list once closed
+	 */
+	if (subflow->close_event_done || READ_ONCE(subflow->local_id) < 0)
+		return false;
+
+	/* A subflow that never got established is retried only after network
+	 * errors: if the peer rejected it, trying again will not help.
+	 */
+	if (!established && !mptcp_pm_retry_path_error(err))
+		return false;
+
+	/* Only re-create subflows initiated by this side: MP_JOIN ones, or the
+	 * initial one on the client side.
+	 */
+	if (!subflow->request_join &&
+	    (subflow->mp_join || READ_ONCE(msk->pm.server_side)))
+		return false;
+
+	/* the path worked for a while: count the attempts from scratch */
+	retry->retries = established ? 0 : subflow->retries;
+	/* a failed attempt is retried with the same socket */
+	retry->reuse = !established && subflow->retries;
+	if (retry->retries >= mptcp_subflow_retries(sock_net(sk)))
+		return false;
+
+	mptcp_remote_address((struct sock_common *)ssk, &retry->remote);
+	retry->remote.id = READ_ONCE(subflow->remote_id);
+	if (!retry->remote.id && READ_ONCE(msk->pm.remote_deny_join_id0))
+		return false;
+
+	mptcp_local_address((struct sock_common *)ssk, &retry->local.addr);
+	retry->local.addr.port = 0;
+	retry->local.addr.id = subflow_get_local_id(subflow);
+	retry->local.flags = subflow->request_bkup || subflow->backup ?
+			     MPTCP_PM_ADDR_FLAG_BACKUP : 0;
+	retry->local.ifindex = ssk->sk_bound_dev_if;
+
+	return true;
+}
+
+/* Called with the msk socket lock held, instead of closing the subflow */
+bool mptcp_pm_kernel_retry_reconnect(struct mptcp_sock *msk, struct sock *ssk,
+				     const struct mptcp_pm_retry *retry)
+{
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+	struct sock *sk = (struct sock *)msk;
+
+	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
+	if (mptcp_subflow_reconnect(sk, ssk, &retry->local, &retry->remote))
+		return false;
+
+	/* the reset cleared it */
+	subflow->retries = retry->retries + 1;
+	return true;
+}
+
+/* Called with the msk socket lock held, after the subflow has been closed */
+void mptcp_pm_kernel_retry(struct mptcp_sock *msk,
+			   const struct mptcp_pm_retry *retry)
+{
+	struct mptcp_subflow_context *subflow;
+	struct sock *sk = (struct sock *)msk;
+	bool allowed;
+
+	spin_lock_bh(&msk->pm.lock);
+	allowed = msk->pm.extra_subflows < mptcp_pm_get_limit_extra_subflows(msk);
+	if (allowed)
+		msk->pm.extra_subflows++;
+	spin_unlock_bh(&msk->pm.lock);
+
+	if (!allowed)
+		return;
+
+	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
+	if (__mptcp_subflow_connect(sk, &retry->local, &retry->remote))
+		return;
+
+	/* the new subflow is the last one: it carries the number of attempts */
+	subflow = list_last_entry(&msk->conn_list, struct mptcp_subflow_context,
+				  node);
+	subflow->retries = retry->retries + 1;
+}
+
 /* Called under PM lock */
 void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
 {
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 60cbd91be632..f29cfd2ad69b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2816,6 +2816,8 @@ static void __mptcp_close_subflow(struct sock *sk)
 	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 		int ssk_state = inet_sk_state_load(ssk);
+		struct mptcp_pm_retry retry;
+		bool do_retry;
 
 		if (ssk_state != TCP_CLOSE &&
 		    (ssk_state != TCP_CLOSE_WAIT ||
@@ -2827,7 +2829,14 @@ static void __mptcp_close_subflow(struct sock *sk)
 		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
 			continue;
 
+		do_retry = mptcp_pm_kernel_retry_check(msk, subflow, &retry);
+		if (do_retry && retry.reuse &&
+		    mptcp_pm_kernel_retry_reconnect(msk, ssk, &retry))
+			continue;
+
 		mptcp_close_ssk(sk, ssk, subflow);
+		if (do_retry && !retry.reuse)
+			mptcp_pm_kernel_retry(msk, &retry);
 	}
 
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ed2f363bb915..5a7a33e4ade8 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -622,6 +622,7 @@ struct mptcp_subflow_context {
 	u8	reset_transient:1;
 	u8	reset_reason:4;
 	u8	stale_count;	    /* Protected by the msk socket lock */
+	u8	retries;	    /* re-creation attempts, msk socket lock */
 	u8	stale;		    /* Protected by the msk socket lock,
 				     * if set the subflow is unable to snd/rcv
 				     * data, the schedule should skip it
@@ -818,6 +819,7 @@ int mptcp_is_checksum_enabled(const struct net *net);
 int mptcp_allow_join_id0(const struct net *net);
 unsigned int mptcp_stale_loss_cnt(const struct net *net);
 unsigned int mptcp_close_timeout(const struct sock *sk);
+u8 mptcp_subflow_retries(const struct net *net);
 int mptcp_get_pm_type(const struct net *net);
 const char *mptcp_get_path_manager(const struct net *net);
 const char *mptcp_get_scheduler(const struct net *net);
@@ -859,6 +861,9 @@ void mptcp_remote_address(const struct sock_common *skc,
 			  struct mptcp_addr_info *addr);
 
 /* called with sk socket lock held */
+int mptcp_subflow_reconnect(struct sock *sk, struct sock *ssk,
+			    const struct mptcp_pm_local *local,
+			    const struct mptcp_addr_info *remote);
 int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 			    const struct mptcp_addr_info *remote);
 int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
@@ -1258,6 +1263,21 @@ void __init mptcp_pm_userspace_register(void);
 void __init mptcp_pm_nl_init(void);
 void mptcp_pm_worker(struct mptcp_sock *msk);
 void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
+
+struct mptcp_pm_retry {
+	struct mptcp_pm_local	local;
+	struct mptcp_addr_info	remote;
+	u8			retries;
+	bool			reuse;	/* failed attempt: same socket */
+};
+
+bool mptcp_pm_kernel_retry_check(struct mptcp_sock *msk,
+				 const struct mptcp_subflow_context *subflow,
+				 struct mptcp_pm_retry *retry);
+void mptcp_pm_kernel_retry(struct mptcp_sock *msk,
+			   const struct mptcp_pm_retry *retry);
+bool mptcp_pm_kernel_retry_reconnect(struct mptcp_sock *msk, struct sock *ssk,
+				     const struct mptcp_pm_retry *retry);
 u8 mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk);
 u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk);
 u8 mptcp_pm_get_endp_laminar_max(const struct mptcp_sock *msk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..0f1d3fae446e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1766,6 +1766,59 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
 	return err;
 }
 
+/* Connect again an MPJ subflow that failed to connect, with the same socket.
+ * The subflow never carried data: like for the initial subflow in
+ * __mptcp_subflow_disconnect(), resetting the TCP and the subflow states is
+ * enough. Called with the msk socket lock held.
+ */
+int mptcp_subflow_reconnect(struct sock *sk, struct sock *ssk,
+			    const struct mptcp_pm_local *local,
+			    const struct mptcp_addr_info *remote)
+{
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct sockaddr_storage addr;
+	u32 remote_token;
+	int addrlen;
+	int err;
+
+	lock_sock(ssk);
+	err = tcp_disconnect(ssk, 0);
+	if (err)
+		goto out;
+
+	mptcp_subflow_ctx_reset(subflow);
+	subflow->request_mptcp = 0;
+	do {
+		subflow->local_nonce = get_random_u32();
+	} while (!subflow->local_nonce);
+	subflow_set_local_id(subflow, local->addr.id);
+	subflow->remote_key_valid = 1;
+	subflow->remote_key = READ_ONCE(msk->remote_key);
+	subflow->local_key = READ_ONCE(msk->local_key);
+	subflow->token = msk->token;
+	mptcp_crypto_key_sha(subflow->remote_key, &remote_token, NULL);
+	subflow->remote_token = remote_token;
+	WRITE_ONCE(subflow->remote_id, remote->id);
+	subflow->request_join = 1;
+	subflow->request_bkup = !!(local->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+	subflow->subflow_id = msk->subflow_id++;
+
+	mptcp_info2sockaddr(remote, &addr, ssk->sk_family);
+	addrlen = sizeof(struct sockaddr_in);
+#if IS_ENABLED(CONFIG_MPTCP_IPV6)
+	if (addr.ss_family == AF_INET6)
+		addrlen = sizeof(struct sockaddr_in6);
+#endif
+	err = ssk->sk_prot->connect(ssk, (struct sockaddr_unsized *)&addr, addrlen);
+
+out:
+	release_sock(ssk);
+	if (!err)
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTX);
+	return err;
+}
+
 void __mptcp_inherit_memcg(struct sock *sk, struct sock *ssk, gfp_t gfp)
 {
 	/* Only if the msk has been accepted already (and not orphaned).*/
-- 
2.25.1

[RFC PATCH mptcp-next v2 3/3] selftests: mptcp: join: validate subflow retry after link failure

[Anil Kaushik]

Test the new sysctl with a long connection in the background: drop the
packets the client sends from the additional subflow address with
netfilter, without bringing the link down, until the subflow is closed
and a retry happens. Then let them through again, and check the subflow
comes back. Then do the same with the initial subflow.

Assisted-by: LLM
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaeda4f..10c431304d78 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2151,6 +2151,68 @@ wait_attempt_fail()
 	return 1
 }
 
+# $1: ns ; $2: minimum number of retries
+wait_sf_retry()
+{
+	local ns="${1}"
+	local retry_min="${2}"
+	local cnt
+
+	for _ in $(seq $((timeout_poll * 10))); do
+		cnt=$(mptcp_lib_get_counter "${ns}" "MPTcpExtSubflowRetry")
+		[ "${cnt:-0}" -ge "${retry_min}" ] && return 0
+		sleep 0.1
+	done
+
+	return 1
+}
+
+# $1: expected number of MPJ ACK received by the server
+wait_mpj_ack()
+{
+	local exp_cnt="${1}"
+	local cnt
+
+	for _ in $(seq $((timeout_poll * 10))); do
+		cnt=$(mptcp_lib_get_counter "${ns1}" "MPTcpExtMPJoinAckRx")
+		[ "${cnt:-0}" -ge "${exp_cnt}" ] && return 0
+		sleep 0.1
+	done
+
+	return 1
+}
+
+# $1: ns ; $2: minimum number of retries
+chk_sf_retry_nr()
+{
+	local ns="${1}"
+	local retry_min="${2}"
+	local count
+
+	print_check "subflow retry"
+	count=$(mptcp_lib_get_counter "${ns}" "MPTcpExtSubflowRetry")
+	if [ -z "${count}" ]; then
+		print_skip
+	elif [ "${count}" -lt "${retry_min}" ]; then
+		fail_test "got ${count} subflow retry[s] expected at least ${retry_min}"
+	else
+		print_ok
+	fi
+}
+
+# $1: client address to block ; $2: retries ; $3: MPJ, expected after it
+#
+# The link stays up, but the client packets from this address are dropped:
+# the subflow using it is closed after a few retransmissions, then re-created
+# once the packets go through again.
+sf_retry_after_block()
+{
+	ip netns exec $ns2 nft add rule ip filter OUTPUT ip saddr "${1}" drop
+	wait_sf_retry $ns2 "${2}"
+	ip netns exec $ns2 nft flush chain ip filter OUTPUT
+	wait_mpj_ack "${3}"
+}
+
 set_userspace_pm()
 {
 	local ns=$1
@@ -2585,6 +2647,34 @@ link_failure_tests()
 		chk_stale_nr $ns2 1 -1 2
 		chk_link_usage $ns2 ns2eth3 $cinsent 50
 	fi
+
+	# subflows closed after a silent link failure, then re-created: the
+	# additional one, then the initial one, with an MPJ using ID 0
+	if reset "subflow retry after link failure" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/subflow_retries'; then
+		init_nftables
+		init_shapers
+		ip netns exec $ns2 sysctl -q net.mptcp.subflow_retries=15
+		ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
+		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
+		pm_nl_set_limits $ns1 0 4
+		pm_nl_set_limits $ns2 0 2
+		pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
+		{ timeout_test=120 test_linkfail=65536 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
+		local tests_pid=$!
+
+		wait_mpj_ack 1
+		sf_retry_after_block 10.0.3.2 2 2
+		sf_retry_after_block 10.0.1.2 3 3
+		mptcp_lib_kill_group_wait $tests_pid
+
+		local retry_nr
+		retry_nr=$(mptcp_lib_get_counter $ns2 "MPTcpExtSubflowRetry")
+		join_syn_tx=$((1 + ${retry_nr:-0})) \
+			chk_join_nr 3 3 3
+		chk_sf_retry_nr $ns2 3
+	fi
 }
 
 add_addr_timeout_tests()
-- 
2.25.1

Re: [RFC PATCH mptcp-next v2 2/3] mptcp: pm: kernel: retry subflows closed after an error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: lockdep warning due to missing nested annotation in mptcp_subflow_reconnect()
--

commit 0bd36d5db0e57b64ed74ede4cce6a078baf37f44
Author: Anil Kaushik <anilkaushikwireless@gmail.com>

mptcp: pm: kernel: retry subflows closed after an error

This commit adds an opt-in sysctl to retry MPTCP subflows closed after
an error. It reuses the same socket for the next attempt if it cannot
connect, avoiding the need for extra state or timers.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/440

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..0f1d3fae446e2 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c

[ ... ]

> @@ -1766,6 +1766,59 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_pm_local *local,
>  	return err;
>  }
>  
> +/* Connect again an MPJ subflow that failed to connect, with the same socket.
> + * The subflow never carried data: like for the initial subflow in
> + * __mptcp_subflow_disconnect(), resetting the TCP and the subflow states is
> + * enough. Called with the msk socket lock held.
> + */
> +int mptcp_subflow_reconnect(struct sock *sk, struct sock *ssk,
> +			    const struct mptcp_pm_local *local,
> +			    const struct mptcp_addr_info *remote)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sockaddr_storage addr;
> +	u32 remote_token;
> +	int addrlen;
> +	int err;
> +
> +	lock_sock(ssk);

[Severity: Medium]
Will this lock_sock(ssk) trigger a lockdep splat?

The function documentation states it is called with the msk socket lock
held. Since MPTCP sockets and their TCP subflows share the same lockdep
class, acquiring the child subflow lock while the parent socket lock is
already held might be seen as a recursive locking deadlock by lockdep.

Should this use lock_sock_nested(ssk, SINGLE_DEPTH_NESTING) to avoid
system panics when CONFIG_PANIC_ON_WARN is enabled?

> +	err = tcp_disconnect(ssk, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013025653.523755-1-anilkaushikwireless@gmail.com?part=2

Re: [RFC PATCH mptcp-next v2 0/3] mptcp: pm: kernel: retry subflows closed after an error

[MPTCP CI]

Hi Anil,

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): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/38341335050

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/067b2bf37cb1
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1184306


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [RFC PATCH mptcp-next v2 3/3] selftests: mptcp: join: validate subflow retry after link failure

[netdev-bot+sashiko]

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

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

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] sf_retry_after_block() in
  tools/testing/selftests/net/mptcp/mptcp_join.sh compares the…
- [Medium] The new subtest runs `run_tests` with test_linkfail=65536
  inside a background subshell (`{ timeout_test=120 test_linkfail=65536…

--

selftests: mptcp: join: validate subflow retry after link failure

This adds a mptcp_join.sh subtest for the new net.mptcp.subflow_retries
sysctl. It runs a long transfer in the background and uses nftables to
drop client packets, first from the additional subflow address and then
from the initial one. It then checks that each subflow is retried and
comes back once the packets are allowed again.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f4d8afaeda4f3..10c431304d783 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh

[ ... ]

> @@ -2585,6 +2647,34 @@ link_failure_tests()

[ ... ]

> +	if reset "subflow retry after link failure" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/subflow_retries'; then
> +		init_nftables
> +		init_shapers
> +		ip netns exec $ns2 sysctl -q net.mptcp.subflow_retries=15
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
> +		pm_nl_set_limits $ns1 0 4
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
> +		{ timeout_test=120 test_linkfail=65536 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null

[Severity: Medium]
If this subtest is selected on its own, does it leak the 64 MiB cinfail
and sinfail temp files?

Here run_tests() runs in a background subshell. If $cinfail or $sinfail is
still empty in the parent shell, run_tests() creates the file inside the
subshell:

		if [ -z "$cinfail" ]; then
			cinfail=$(mktemp)
		fi
		make_file "$cinfail" "client" $size

It does the same for $sinfail. With test_linkfail=65536, make_file fills
each file with 65536 x 1024 bytes.

Those assignments never reach the parent shell. As a result, cleanup()
only sees the empty strings:

	rm -f "$cin" "$cout" "$sinfail"
	rm -f "$sin" "$sout" "$cinsent" "$cinfail"

In a full run, the earlier foreground link failure subtests
(test_linkfail=1 and test_linkfail=2) have already set both variables in
the parent. When this subtest is picked by ID or by name, those subtests
are skipped, and about 128 MiB is left in /tmp on every run.

The existing userspace PM tests use the same background pattern. With
test_linkfail=128, they only leave about 128 KiB behind.

Could cinfail and sinfail be created in the parent shell before the
background run_tests is started?

> +		local tests_pid=$!
> +
> +		wait_mpj_ack 1
> +		sf_retry_after_block 10.0.3.2 2 2
> +		sf_retry_after_block 10.0.1.2 3 3

[Severity: Medium]
Can these fixed running totals turn the second block into a no-op? A
threshold of 3 for 10.0.1.2 is only right if the 10.0.3.2 block produced
exactly 2 retries.

In mptcp_pm_kernel_retry_reconnect(), the counter goes up before the SYN
is sent:

net/mptcp/pm_kernel.c:mptcp_pm_kernel_retry_reconnect() {
    ...
	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
	if (mptcp_subflow_reconnect(sk, ssk, &retry->local, &retry->remote))
		return false;
    ...
}

So when wait_sf_retry() sees the counter reach 2, the nft rule is still
dropping that SYN. With tcp_syn_retries=1, there is only one SYN
retransmission, about 1s later.

Before the rule is flushed, the shell still has to:

- finish sleep 0.1
- read the counter with nstat in mptcp_lib_get_counter()
- run nft flush

On a loaded or debug-kernel CI machine this can take more than about 1s.
Couldn't the retransmission be dropped too in that case?

The attempt would then fail with ETIMEDOUT. Since 2 is below
subflow_retries=15, a third attempt raises MPTcpExtSubflowRetry to 3.

In the second block, wait_sf_retry $ns2 3 then returns on its first poll,
and the drop rule on 10.0.1.2 is flushed within milliseconds. The initial
subflow never reaches the tcp_retries2 timeout, so the ID 0 re-creation
never runs.

sf_retry_after_block() ignores the return values of both wait helpers:

	wait_sf_retry $ns2 "${2}"
	ip netns exec $ns2 nft flush chain ip filter OUTPUT
	wait_mpj_ack "${3}"

So wait_mpj_ack 3 times out after 30s without reporting anything. Then
chk_join_nr 3 3 3 fails with only 2 joins.

The counter also can't separate retries from the two blocks.
mptcp_pm_kernel_retry() raises the same namespace-wide counter when it
creates a new subflow:

	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
	if (__mptcp_subflow_connect(sk, &retry->local, &retry->remote))
		return;

Could each block save the counter before adding its drop rule and wait for
a delta instead? Could the test also fail when wait_sf_retry() or
wait_mpj_ack() time out?

> +		mptcp_lib_kill_group_wait $tests_pid

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013025653.523755-1-anilkaushikwireless%40gmail.com

Re: [RFC PATCH mptcp-next v2 2/3] mptcp: pm: kernel: retry subflows closed after an error

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 4 · Low: 1

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

- [High] For a subflow that reached fully_established,
  mptcp_pm_kernel_retry_check() accepts any non-zero sk_err, because the…
- [Medium] The commit message says the SYN retransmissions of the new
  subflow space out the attempts, so no extra state or timer is needed.
- [Medium] Failed subflow attempts are reported differently depending on
  which retry path handles them.
- [Medium] mptcp_subflow_reconnect() calls ssk->sk_prot->connect()
  directly after tcp_disconnect() and never calls sk_prot->pre_connect().
- [Medium] mptcp_pm_kernel_retry_check() builds the new subflow's local
  backup request from `subflow->request_bkup || subflow->backup`.
- [Low] Re-creating the client's initial MP_CAPABLE subflow needs a free
  extra-subflow slot, and the motivating main-link/backup example does…

--

mptcp: pm: kernel: retry subflows closed after an error

This adds an opt-in net.mptcp.subflow_retries sysctl. When it is set, the
in-kernel PM re-creates locally initiated subflows that TCP closed after an
error, using the same addresses. Failed attempts reuse the same socket
through a new mptcp_subflow_reconnect() helper, and a new SubflowRetry MIB
counter tracks the attempts.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/440

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index d3014bf57bf3c..c9199727c88cc 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1591,6 +1591,122 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
>  	return true;
>  }
>  
> +static bool mptcp_pm_retry_path_error(int err)
> +{
> +	switch (err) {
> +	case ETIMEDOUT:
> +	case EHOSTUNREACH:
> +	case ENETUNREACH:
> +	case EHOSTDOWN:
> +	case ENETDOWN:
> +		return true;
> +	}
> +
> +	return false;
> +}

[Severity: Medium]
The commit message says:

    Its SYN retransmissions space out the attempts, so no extra state or
    timer is needed. If it cannot connect, the same socket is used for the
    next attempt.

The subflow_retries documentation says the same and points to
tcp_syn_retries. Is that true for any error other than ETIMEDOUT?

EHOSTUNREACH, ENETUNREACH, EHOSTDOWN and ENETDOWN usually come from ICMP.
In SYN_SENT, tcp_v4_err() aborts the connect as soon as the ICMP arrives:

net/ipv4/tcp_ipv4.c:tcp_v4_err() {
    ...
		if (!sock_owned_by_user(sk))
			tcp_done_with_error(sk, err);
    ...
}

So the SYN is never retransmitted with backoff. The worker then runs
__mptcp_close_subflow() and reconnects right away.

During an outage, wouldn't each attempt fail within about one RTT? The
whole subflow_retries budget would then be used up in a few RTTs.

A synchronous connect failure also seems to end the retry chain. For
example, tcp_v4_connect() returns -ENETUNREACH from the route lookup when
no route exists while the link is down:

net/ipv4/tcp_ipv4.c:tcp_v4_connect() {
    ...
	if (IS_ERR(rt)) {
		err = PTR_ERR(rt);
		if (err == -ENETUNREACH)
			IP_INC_STATS(net, IPSTATS_MIB_OUTNOROUTES);
		return err;
	}
    ...
}

On the fresh socket path, mptcp_pm_kernel_retry() just returns when
__mptcp_subflow_connect() fails.

On the reuse path, mptcp_pm_kernel_retry_reconnect() returns false and
__mptcp_close_subflow() falls through to mptcp_close_ssk(). Because
retry.reuse is true, mptcp_pm_kernel_retry() is not called either.

No retry state is left anywhere in that case. Is the path ever re-created
once it recovers, even when budget is still left?

> +
> +/* A subflow initiated by this side and closed by TCP after an error can be
> + * re-created with the same addresses, see the 'subflow_retries' sysctl. The
> + * SYN retransmissions of the new subflow space out the attempts. Called with
> + * the msk socket lock held, before the subflow is closed.
> + */

[ ... ]

> +	/* A subflow that never got established is retried only after network
> +	 * errors: if the peer rejected it, trying again will not help.
> +	 */
> +	if (!established && !mptcp_pm_retry_path_error(err))
> +		return false;
> +
> +	/* Only re-create subflows initiated by this side: MP_JOIN ones, or the
> +	 * initial one on the client side.
> +	 */
> +	if (!subflow->request_join &&
> +	    (subflow->mp_join || READ_ONCE(msk->pm.server_side)))
> +		return false;
> +
> +	/* the path worked for a while: count the attempts from scratch */
> +	retry->retries = established ? 0 : subflow->retries;

[Severity: High]
For a fully established subflow, any non-zero sk_err passes the error
check above, and the attempt count is reset to 0. Can this turn into an
unbounded reconnect loop?

ECONNRESET from a peer RST or MP_RST would go down this path. So would
ECONNABORTED from a local ss -K or bpf_sock_destroy, through
tcp_abort() -> tcp_done_with_error():

__mptcp_close_subflow()
  mptcp_pm_kernel_retry_check()    /* retries = 0 */
  mptcp_close_ssk()                /* frees the extra_subflows slot */
  mptcp_pm_kernel_retry()
    __mptcp_subflow_connect()      /* no delay */

A peer, or an on-path attacker able to inject an in-window RST, could
accept each MP_JOIN, let it become fully established, and then reset it.
The client would then reconnect every RTT, and the subflow_retries cap
would never apply.

This check also ignores subflow->reset_seen and subflow->reset_transient,
which mptcp_incoming_options() already records from MP_RST. Per RFC 8684,
a subflow reset with T=0 should not be re-established on that path.

Should a non-transient MP_RST, or a local administrative abort, be
excluded from the retry?

> +	/* a failed attempt is retried with the same socket */
> +	retry->reuse = !established && subflow->retries;
> +	if (retry->retries >= mptcp_subflow_retries(sock_net(sk)))
> +		return false;

[ ... ]

> +	mptcp_local_address((struct sock_common *)ssk, &retry->local.addr);
> +	retry->local.addr.port = 0;
> +	retry->local.addr.id = subflow_get_local_id(subflow);
> +	retry->local.flags = subflow->request_bkup || subflow->backup ?
> +			     MPTCP_PM_ADDR_FLAG_BACKUP : 0;

[Severity: Medium]
Is subflow->backup the right input here? It holds the peer's backup flag.
subflow_finish_connect() sets it from the SYN-ACK, and
mptcp_pm_mp_prio_received() sets it from a received MP_PRIO. The local
preference is request_bkup.

Both __mptcp_subflow_connect() and mptcp_subflow_reconnect() turn
MPTCP_PM_ADDR_FLAG_BACKUP into request_bkup, which is sent as the MP_JOIN
B bit. A subflow that only the peer marked as backup would therefore be
re-created with request_bkup=1.

The local side would then advertise a backup preference that was never
configured for the endpoint. The local scheduler would also keep treating
the path as backup, even after the peer clears its own flag with MP_PRIO
B=0.

Should this use only subflow->request_bkup?

> +	retry->local.ifindex = ssk->sk_bound_dev_if;
> +
> +	return true;
> +}

[ ... ]

> +/* Called with the msk socket lock held, after the subflow has been closed */
> +void mptcp_pm_kernel_retry(struct mptcp_sock *msk,
> +			   const struct mptcp_pm_retry *retry)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *sk = (struct sock *)msk;
> +	bool allowed;
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	allowed = msk->pm.extra_subflows < mptcp_pm_get_limit_extra_subflows(msk);
> +	if (allowed)
> +		msk->pm.extra_subflows++;
> +	spin_unlock_bh(&msk->pm.lock);
> +
> +	if (!allowed)
> +		return;

[Severity: Low]
mptcp_pm_kernel_retry_check() explicitly allows retrying the client's
initial subflow. When that MP_CAPABLE subflow closes,
mptcp_pm_subflow_check_next() does not decrement pm.extra_subflows:

net/mptcp/pm.c:mptcp_pm_subflow_check_next() {
    ...
	update_subflows = subflow->request_join || subflow->mp_join;
    ...
}

Its MP_JOIN replacement therefore needs a free extra-subflow slot. What
happens when the existing MPJ subflows already use the whole
limit_extra_subflows quota? One example is a limit of 1 with one backup
endpoint.

mptcp_pm_kernel_retry_check() would accept the initial subflow, but this
check would then return without retrying. The connection would stay on the
backup, which is the main-link/backup case the commit message describes.

Could the documentation say that replacing the initial subflow uses an
extra subflow slot?

> +
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
> +	if (__mptcp_subflow_connect(sk, &retry->local, &retry->remote))
> +		return;

[ ... ]

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 60cbd91be6320..f29cfd2ad69b9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -2827,7 +2829,14 @@ static void __mptcp_close_subflow(struct sock *sk)
>  		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
>  			continue;
>  
> +		do_retry = mptcp_pm_kernel_retry_check(msk, subflow, &retry);
> +		if (do_retry && retry.reuse &&
> +		    mptcp_pm_kernel_retry_reconnect(msk, ssk, &retry))
> +			continue;

[Severity: Medium]
When a fresh socket attempt fails, mptcp_close_ssk() runs and emits
MPTCP_EVENT_SUB_CLOSED, since the msk is TCP_ESTABLISHED:

net/mptcp/protocol.c:mptcp_close_ssk() {
    ...
	if (sk->sk_state == TCP_ESTABLISHED)
		mptcp_event(MPTCP_EVENT_SUB_CLOSED, mptcp_sk(sk), ssk, GFP_KERNEL);
    ...
}

When later attempts fail and take the reuse path, this continue skips
mptcp_close_ssk(). mptcp_subflow_reconnect() then resets the context and
assigns a new subflow_id without sending a SUB_CLOSED event.

Will userspace event listeners miss those failed attempts?

The MIB counters also look inconsistent. mptcp_subflow_reconnect() counts
MPTCP_MIB_JOINSYNTX on success:

	if (!err)
		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTX);

But unlike __mptcp_subflow_connect(), it does not count
MPTCP_MIB_JOINSYNTXCONNECTERR when the connect fails.

> +
>  		mptcp_close_ssk(sk, ssk, subflow);
> +		if (do_retry && !retry.reuse)
> +			mptcp_pm_kernel_retry(msk, &retry);
>  	}

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..0f1d3fae446e2 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c

[ ... ]

> +	mptcp_info2sockaddr(remote, &addr, ssk->sk_family);
> +	addrlen = sizeof(struct sockaddr_in);
> +#if IS_ENABLED(CONFIG_MPTCP_IPV6)
> +	if (addr.ss_family == AF_INET6)
> +		addrlen = sizeof(struct sockaddr_in6);
> +#endif
> +	err = ssk->sk_prot->connect(ssk, (struct sockaddr_unsized *)&addr, addrlen);

[Severity: Medium]
This calls sk_prot->connect() directly without running
sk_prot->pre_connect() first. Are the cgroup BPF connect4/connect6
programs skipped for every reused socket attempt?

__mptcp_subflow_connect() goes through kernel_connect() ->
__inet_stream_connect(), which runs the hook:

net/ipv4/af_inet.c:__inet_stream_connect() {
    ...
		if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) {
			err = sk->sk_prot->pre_connect(sk, uaddr, addr_len);
    ...
}

mptcp_connect() also calls it explicitly before its own direct
sk_prot->connect():

net/mptcp/protocol.c:mptcp_connect() {
    ...
	if (BPF_CGROUP_PRE_CONNECT_ENABLED(ssk)) {
		err = ssk->sk_prot->pre_connect(ssk, uaddr, addr_len);
    ...
}

Subflow sockets inherit the msk's cgroup. With subflow_retries >= 2, a
connect4/connect6 program that applies deny policy, accounting or backend
selection would never see the second and later attempts.

> +
> +out:
> +	release_sock(ssk);
> +	if (!err)
> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNTX);
> +	return err;
> +}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013025653.523755-1-anilkaushikwireless%40gmail.com