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

[RFC PATCH mptcp-next v3 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).

When TCP closes a subflow we created because of an error, the PM opens
a new one to the same addresses, and if it fails, the same socket is
used for the next try. A new sysctl, net.mptcp.subflow_retries, sets the
max tries.

Open question: an ICMP unreachable aborts the SYN right away, so the
tries can be used up in a few RTTs. Adding a delay would need a timer
again. Is that acceptable, or is there a better way?

Changes in v3 (Sashiko and CI reviews):
 - no retry after MP_RST without the T flag, local abort or EBADMSG
 - no retry on a removed endpoint or address
 - count the tries from scratch only after a minute of uptime
 - use only request_bkup for the backup flag
 - reused socket: SUB_CLOSED event, MIB counter and BPF pre_connect
 - lock_sock_nested() when reconnecting
 - patch 1: don't wait forever on EAGAIN
 - patch 3: check the subflow address, create the files before the
   background job, bigger file for slow debug kernels
 - wrap lines over 80 columns

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

v2: https://lore.kernel.org/mptcp/20261013025653.523755-1-anilkaushikwireless@gmail.com/
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     |  18 +++
 net/mptcp/ctrl.c                              |  13 ++
 net/mptcp/mib.c                               |   1 +
 net/mptcp/mib.h                               |   1 +
 net/mptcp/pm.c                                |   8 +
 net/mptcp/pm_kernel.c                         | 145 ++++++++++++++++++
 net/mptcp/protocol.c                          |   9 ++
 net/mptcp/protocol.h                          |  24 +++
 net/mptcp/subflow.c                           |  63 ++++++++
 .../selftests/net/mptcp/mptcp_connect.c       |  16 ++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 106 +++++++++++++
 11 files changed, 404 insertions(+)

-- 
2.25.1

[RFC PATCH mptcp-next v3 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. If EAGAIN lasts longer than the
poll timeout, still fail: poll() does not check every cause of EAGAIN.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 178d98d91fea..03fff859e4d1 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -638,6 +638,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 		.events = POLLIN | POLLOUT,
 	};
 	unsigned int total_wlen = 0, total_rlen = 0;
+	int eagain_ms = 0;
 
 	set_nonblock(peerfd, true);
 
@@ -711,6 +712,20 @@ 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 that
+					 * 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,
+					 * but not forever: poll() does not
+					 * check every cause of EAGAIN.
+					 */
+					if (errno == EAGAIN &&
+					    ++eagain_ms <= poll_timeout) {
+						usleep(1000);
+						continue;
+					}
+
 					/* expected reset, continue to read */
 					if (cfg_rcv_trunc &&
 					    (errno == ECONNRESET ||
@@ -723,6 +738,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 					return 111;
 				}
 
+				eagain_ms = 0;
 				winfo->off += bw;
 				winfo->len -= bw;
 				total_wlen += bw;
-- 
2.25.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: mptcp_connect fails on EAGAIN when poll_timeout is infinite
--

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

selftests: mptcp: connect: retry write() on EAGAIN

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.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..03fff859e4d1d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
[ ... ]
> @@ -711,6 +712,20 @@ 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 that
> +					 * 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,
> +					 * but not forever: poll() does not
> +					 * check every cause of EAGAIN.
> +					 */
> +					if (errno == EAGAIN &&
> +					    ++eagain_ms <= poll_timeout) {
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Will this condition incorrectly abort the test immediately if poll_timeout
is set to -1 (infinite timeout)?

If the tool is invoked with a timeout of 0 or less (e.g., -t 0), poll_timeout
is set to -1. When write() returns EAGAIN, the new condition evaluates to
1 <= -1, which is false. This bypasses the intended retry logic, immediately
failing the transfer instead of waiting.

I noticed a similar issue in the preexisting code in xdisconnect():

tools/testing/selftests/net/mptcp/mptcp_connect.c:xdisconnect() {
	...
	for (i = 0; ; i += msec_sleep) {
		/* closed socket are not listed by 'ss' */
		if (system(cmd) != 0)
			break;

		if (i > poll_timeout)
			xerror("timeout while waiting for spool to complete");
		usleep(msec_sleep * 1000);
	}
	...
}

Does this code also incorrectly assume poll_timeout is positive, causing an
immediate crash if poll_timeout is -1?

> +						usleep(1000);
> +						continue;
> +					}

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

[RFC PATCH mptcp-next v3 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 stayed established for a minute. Re-creating the
initial subflow uses one of the additional subflows allowed by the
limits.

Subflows refused by the peer, reset by the peer without the transient
flag (RFC 8684 section 3.6), aborted locally, or using a removed address
are 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 |  18 +++
 net/mptcp/ctrl.c                          |  13 ++
 net/mptcp/mib.c                           |   1 +
 net/mptcp/mib.h                           |   1 +
 net/mptcp/pm.c                            |   8 ++
 net/mptcp/pm_kernel.c                     | 145 ++++++++++++++++++++++
 net/mptcp/protocol.c                      |   9 ++
 net/mptcp/protocol.h                      |  24 ++++
 net/mptcp/subflow.c                       |  63 ++++++++++
 9 files changed, 282 insertions(+)

diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
index b9b5f58e0625..544f6b84d4af 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -149,6 +149,24 @@ 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 stayed established for a
+	minute. Only subflows initiated by this side are re-created, within
+	the limits set for the path manager: re-creating the initial subflow
+	uses one of the additional subflows. Subflows refused by the peer,
+	reset by the peer without the transient flag, aborted locally, or
+	using a removed address are 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.c b/net/mptcp/pm.c
index d7c5b50b34cc..b3f7f7b2ef84 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -794,6 +794,14 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 			int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
 			u8 id = subflow_get_local_id(subflow);
 
+			/* a dead subflow still waiting to be closed must not
+			 * be re-created on a removed address
+			 */
+			if ((rm_type == MPTCP_MIB_RMADDR &&
+			     remote_id == rm_id) ||
+			    (rm_type == MPTCP_MIB_RMSUBFLOW && id == rm_id))
+				subflow->retries = MPTCP_SUBFLOW_NO_RETRY;
+
 			if ((1 << inet_sk_state_load(ssk)) &
 			    (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
 				continue;
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..d020e5f69c01 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1591,6 +1591,151 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
 	return true;
 }
 
+/* a re-created subflow reset sooner counts as a failed attempt */
+#define MPTCP_PM_RETRY_STABLE	(60 * HZ)
+
+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;
+
+	/* removed address, see mptcp_pm_rm_addr_or_subflow() */
+	if (subflow->retries == MPTCP_SUBFLOW_NO_RETRY)
+		return false;
+
+	/* The peer asked not to use this path again (MP_RST without the T
+	 * flag), the subflow has been aborted locally, or it hit an MPTCP
+	 * protocol error.
+	 */
+	if ((subflow->reset_seen && !subflow->reset_transient) ||
+	    err == ECONNABORTED || err == EBADMSG)
+		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;
+
+	/* Count the attempts from scratch only if the path worked for a
+	 * while: a re-created subflow reset soon after its creation counts
+	 * as a failed attempt.
+	 */
+	retry->retries = subflow->retries;
+	if (established &&
+	    (!subflow->retries ||
+	     (u32)(tcp_jiffies32 - subflow->retry_stamp) >=
+	     MPTCP_PM_RETRY_STABLE))
+		retry->retries = 0;
+	/* 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 ?
+			     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;
+
+	/* like mptcp_close_ssk() does for the failed attempt */
+	mptcp_event(MPTCP_EVENT_SUB_CLOSED, msk, ssk, GFP_KERNEL);
+
+	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
+	if (mptcp_subflow_reconnect(sk, ssk, &retry->local, &retry->remote))
+		return false;
+
+	/* the reset cleared them */
+	subflow->retries = retry->retries + 1;
+	subflow->retry_stamp = tcp_jiffies32;
+	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;
+	subflow->retry_stamp = tcp_jiffies32;
+}
+
 /* 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..1aa92c9614cd 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -622,6 +622,8 @@ 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 */
+	u32	retry_stamp;	    /* creation of a re-created subflow */
 	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 +820,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 +862,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 +1264,24 @@ 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);
+
+/* subflow->retries value preventing any re-creation */
+#define MPTCP_SUBFLOW_NO_RETRY	U8_MAX
+
+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..0cdeae4b45fa 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1766,6 +1766,69 @@ 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 sockaddr_unsized *uaddr;
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct sockaddr_storage addr;
+	u32 remote_token;
+	int addrlen;
+	int err;
+
+	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
+	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
+	uaddr = (struct sockaddr_unsized *)&addr;
+
+	/* like __inet_stream_connect() for __mptcp_subflow_connect() */
+	if (BPF_CGROUP_PRE_CONNECT_ENABLED(ssk)) {
+		err = ssk->sk_prot->pre_connect(ssk, uaddr, addrlen);
+		if (err)
+			goto out;
+	}
+
+	err = ssk->sk_prot->connect(ssk, uaddr, addrlen);
+
+out:
+	release_sock(ssk);
+	MPTCP_INC_STATS(sock_net(sk), err ? MPTCP_MIB_JOINSYNTXCONNECTERR :
+					    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

Re: [RFC PATCH mptcp-next v3 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: duplicate MPTCP_EVENT_SUB_CLOSED event emitted on reconnect failure
--

commit 66071f5b3caa5390ad8bce4ae347cb2285123fa3
Author: Anil Kaushik <anilkaushikwireless@gmail.com>

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

This commit adds an opt-in sysctl to retry creating subflows closed by
TCP after an error. The PM immediately creates a new subflow using the
same addresses, and spaces out attempts via SYN retransmissions. A new
SubflowRetry counter tracks these 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..d020e5f69c013 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
[ ... ]
> @@ -1591,6 +1591,151 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
[ ... ]
> +/* 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;
> +
> +	/* like mptcp_close_ssk() does for the failed attempt */
> +	mptcp_event(MPTCP_EVENT_SUB_CLOSED, msk, ssk, GFP_KERNEL);
> +
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
> +	if (mptcp_subflow_reconnect(sk, ssk, &retry->local, &retry->remote))
> +		return false;

[Severity: Medium]
Will this sequence emit a duplicate MPTCP_EVENT_SUB_CLOSED event if the
reconnect attempt fails?

When mptcp_subflow_reconnect() is called, it clears the subflow context
using mptcp_subflow_ctx_reset(), which zeroes the close_event_done flag:

net/mptcp/subflow.c:mptcp_subflow_reconnect() {
...
	mptcp_subflow_ctx_reset(subflow);
...

If the underlying tcp connect call fails, mptcp_subflow_reconnect() returns
an error, causing mptcp_pm_kernel_retry_reconnect() to return false.

[ ... ]
> 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;
> +
>  		mptcp_close_ssk(sk, ssk, subflow);

[Severity: Medium]
When mptcp_pm_kernel_retry_reconnect() returns false, the fallback path
here calls mptcp_close_ssk().

Since the close_event_done flag was cleared to false by the failed
reconnect attempt, and the parent MPTCP socket is still in TCP_ESTABLISHED,
mptcp_close_ssk() will unconditionally emit a second MPTCP_EVENT_SUB_CLOSED
event for the exact same subflow socket.

Could this duplicate state notification confuse or desynchronize userspace
path managers?

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

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

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

[Matthieu Baerts]

Hi Anil,

Thank you for the new version. Here is a very quick other review, mostly
questions.

Also, please wait for the netdev Sashiko review (24 to 48h) before
sending a new version. You can remove the RFC tag. Ideally, include a
changelog per patch.

On 14/10/2026 16:33, Anil Kaushik wrote:
> 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

I confirm that it would be better to avoid the sysctl for this in-kernel
PM specific setting. Instead, please add another limit that can be
configured via netlink. Add a sensible default value (what do you
recommend?), but on by default.

By default, I think the kernel tries to establish a connection for 67
seconds.

A (maybe bad?) idea: I wonder if we shouldn't modify icsk_syn_retries
instead: by doing that, we would do only one retry, but with a long
exponential backoff timeout. The config would set this value directly,
with 0 meaning disabled (and not "no retransmission after the retry"). WDYT?

> 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 stayed established for a minute. Re-creating the
> initial subflow uses one of the additional subflows allowed by the
> limits.
> 
> Subflows refused by the peer, reset by the peer without the transient
> flag (RFC 8684 section 3.6), aborted locally, or using a removed address
> are not retried.
> 
> A new counter, SubflowRetry, counts the attempts.

I'm not a fan of a MIB counter only for the in-kernel PM. Do we have
other similar ones? We already expose some diag counters per subflow (ss
-Mte), would that work?

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d7c5b50b34cc..b3f7f7b2ef84 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -794,6 +794,14 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
>  			int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
>  			u8 id = subflow_get_local_id(subflow);
>  
> +			/* a dead subflow still waiting to be closed must not
> +			 * be re-created on a removed address
> +			 */
> +			if ((rm_type == MPTCP_MIB_RMADDR &&
> +			     remote_id == rm_id) ||
> +			    (rm_type == MPTCP_MIB_RMSUBFLOW && id == rm_id))
> +				subflow->retries = MPTCP_SUBFLOW_NO_RETRY;

Is it specific for your case?

BTW, it would be interesting to test cases where the right actions are
taken when a subflow is disconnected or it is trying to reconnect, e.g.
closing the socket, etc. Maybe easier with Packetdrill to get the right
timing? (or extend the existing test to end with a drop, then close the
connetion

Also we should (manually?) check the right netlink events are being
sent, e.g. not having a "closed subflow" event if the subflow wasn't
established before (but we are probably already doing the right thing
there, just to be sure, I didn't check). I guess your current test can
also check the netlink events.

>  			if ((1 << inet_sk_state_load(ssk)) &
>  			    (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
>  				continue;
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index d3014bf57bf3..d020e5f69c01 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1591,6 +1591,151 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
>  	return true;
>  }
>  
> +/* a re-created subflow reset sooner counts as a failed attempt */
> +#define MPTCP_PM_RETRY_STABLE	(60 * HZ)

That feels "fragile" (e.g. with net.ipv4.tcp_syn_retries=0): can we not
check why it failed instead?

> +static bool mptcp_pm_retry_path_error(int err)
> +{
> +	switch (err) {
> +	case ETIMEDOUT:
> +	case EHOSTUNREACH:
> +	case ENETUNREACH:
> +	case EHOSTDOWN:
> +	case ENETDOWN:

Out-of-curiosity: what error do you get when the issue is detected via
TCP Keeplive?

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

(probably only the last sentence is useful)

> +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;
> +
> +	/* removed address, see mptcp_pm_rm_addr_or_subflow() */
> +	if (subflow->retries == MPTCP_SUBFLOW_NO_RETRY)
> +		return false;
> +
> +	/* The peer asked not to use this path again (MP_RST without the T
> +	 * flag), the subflow has been aborted locally, or it hit an MPTCP
> +	 * protocol error.
> +	 */
> +	if ((subflow->reset_seen && !subflow->reset_transient) ||
> +	    err == ECONNABORTED || err == EBADMSG)
> +		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;
> +
> +	/* Count the attempts from scratch only if the path worked for a
> +	 * while: a re-created subflow reset soon after its creation counts
> +	 * as a failed attempt.
> +	 */
> +	retry->retries = subflow->retries;
> +	if (established &&
> +	    (!subflow->retries ||
> +	     (u32)(tcp_jiffies32 - subflow->retry_stamp) >=
> +	     MPTCP_PM_RETRY_STABLE))
> +		retry->retries = 0;
> +	/* a failed attempt is retried with the same socket */
> +	retry->reuse = !established && subflow->retries;

Why can we not always re-used?

If we cannot always do that, maybe we should always re-create one? It
might be better to avoid having multiple paths for that in the code.

> +	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 ?
> +			     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;
> +
> +	/* like mptcp_close_ssk() does for the failed attempt */
> +	mptcp_event(MPTCP_EVENT_SUB_CLOSED, msk, ssk, GFP_KERNEL);
> +
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
> +	if (mptcp_subflow_reconnect(sk, ssk, &retry->local, &retry->remote))
> +		return false;
> +
> +	/* the reset cleared them */
> +	subflow->retries = retry->retries + 1;
> +	subflow->retry_stamp = tcp_jiffies32;
> +	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)

Yes, probably better to avoid having both retry_reconnect and 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;
> +	subflow->retry_stamp = tcp_jiffies32;
> +}
> +
>  /* 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);

Avoid calling in-kernel specific functions directly: it should be done
via the PM "interface". IOW, call something in pm.c, which would call
the in-kernel function if this pm is being used.

(later, we should do that via a pm ops, but we are not there yet --
still best to avoid calling pm kernel functions directly here in all
conditions)


>  	}
>  
>  }
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index ed2f363bb915..1aa92c9614cd 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -622,6 +622,8 @@ 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 */
> +	u32	retry_stamp;	    /* creation of a re-created subflow */

Probably best not to add that between stale_count and stale + avoid
creating new "holes" in structures (I didn't check with pahole, but I
guess that's the case)

>  	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 +820,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 +862,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 +1264,24 @@ 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);
> +
> +/* subflow->retries value preventing any re-creation */
> +#define MPTCP_SUBFLOW_NO_RETRY	U8_MAX
> +
> +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..0cdeae4b45fa 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1766,6 +1766,69 @@ 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 sockaddr_unsized *uaddr;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sockaddr_storage addr;
> +	u32 remote_token;
> +	int addrlen;
> +	int err;
> +
> +	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
> +	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
> +	uaddr = (struct sockaddr_unsized *)&addr;
> +
> +	/* like __inet_stream_connect() for __mptcp_subflow_connect() */
> +	if (BPF_CGROUP_PRE_CONNECT_ENABLED(ssk)) {
> +		err = ssk->sk_prot->pre_connect(ssk, uaddr, addrlen);
> +		if (err)
> +			goto out;
> +	}
> +
> +	err = ssk->sk_prot->connect(ssk, uaddr, addrlen);
> +
> +out:
> +	release_sock(ssk);
> +	MPTCP_INC_STATS(sock_net(sk), err ? MPTCP_MIB_JOINSYNTXCONNECTERR :
> +					    MPTCP_MIB_JOINSYNTX);
> +	return err;
> +}

This might not be used if we always call close_subflow then connect, but
in general: avoid this bunch of duplicated code. Here you could extract
code from the normal connect case and call that from here instead of
re-written with a different way (i.e. not calling kernel_connect()).

> +
>  void __mptcp_inherit_memcg(struct sock *sk, struct sock *ssk, gfp_t gfp)
>  {
>  	/* Only if the msk has been accepted already (and not orphaned).*/

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[RFC PATCH mptcp-next v3 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, from the same address. 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 | 106 ++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaeda4f..1048c9cfceb7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2151,6 +2151,76 @@ 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} 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}"
+
+	print_check "subflow from ${1}"
+	if ip netns exec $ns2 ss -tnH state established src "${1}" |
+	   grep -q .; then
+		print_ok
+	else
+		fail_test "no established subflow from ${1}"
+	fi
+}
+
 set_userspace_pm()
 {
 	local ns=$1
@@ -2585,6 +2655,42 @@ 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
+		# created here, not in the background job, for cleanup()
+		if [ -z "$cinfail" ]; then
+			cinfail=$(mktemp)
+		fi
+		if [ -z "$sinfail" ]; then
+			sinfail=$(mktemp)
+		fi
+		{ timeout_test=120 test_linkfail=131072 \
+			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 v3 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/38583978482

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


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)