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

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

[Anil Kaushik]

With the in-kernel PM, a subflow closed after an error is never
re-created, even once the path is back (#440). For example, with fiber
as main link and Starlink as backup, connections never return to the
fiber after an outage.

Patch 1 fixes a selftest flake: write() can return EAGAIN after POLLOUT
when a subflow closes.
Patch 2 adds an opt-in sysctl, net.mptcp.subflow_retry_timeout, to
re-create such subflows with an exponential backoff.
Patch 3 adds the selftests.

Questions:
 - sysctl, or an endpoint flag?
 - the re-created initial subflow counts in the subflows limit, OK?
 - should retries stop after some attempts?

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

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.c                                | 272 ++++++++++++++++++
 net/mptcp/pm_kernel.c                         |   3 +
 net/mptcp/protocol.h                          |   5 +
 .../selftests/net/mptcp/mptcp_connect.c       |   8 +
 .../testing/selftests/net/mptcp/mptcp_join.sh |  84 ++++++
 9 files changed, 402 insertions(+)

-- 
2.25.1

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

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 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_retry_timeout (0 by default).
When set, the PM tries to re-create such subflows with the same
addresses: first after 1 second, then doubling the wait after each
failure, up to the sysctl value. Only subflows created by this side are
retried. Retries stop when the endpoint or the peer's address is
removed. 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
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.c                            | 272 ++++++++++++++++++++++
 net/mptcp/pm_kernel.c                     |   3 +
 net/mptcp/protocol.h                      |   5 +
 7 files changed, 310 insertions(+)

diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
index b9b5f58e0625..5144319923f5 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -149,6 +149,21 @@ stale_loss_cnt - INTEGER
 
 	Default: 4
 
+subflow_retry_timeout - INTEGER (seconds)
+	Maximum interval between two attempts to re-create a subflow closed
+	after an error, e.g. a timeout or an unreachable path, while the MPTCP
+	connection is still established. When set, the in-kernel path manager
+	tries again to create such subflows, using the same local and remote
+	addresses, one second after the closure, then doubling the interval
+	after each failed attempt, up to this value. Only subflows initiated
+	by this side are re-created, within the limits set for the path
+	manager. Attempts stop when the corresponding endpoint is removed, or
+	when the peer removes the corresponding address.
+
+	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..f90850f06794 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -31,6 +31,7 @@ struct mptcp_pernet {
 	unsigned int blackhole_timeout;
 	unsigned int close_timeout;
 	unsigned int stale_loss_cnt;
+	unsigned int subflow_retry_timeout;
 	atomic_t active_disable_times;
 	unsigned long active_disable_stamp;
 	u8 syn_retrans_before_tcp_fallback;
@@ -73,6 +74,11 @@ unsigned int mptcp_stale_loss_cnt(const struct net *net)
 	return mptcp_get_pernet(net)->stale_loss_cnt;
 }
 
+unsigned int mptcp_subflow_retry_timeout(const struct net *net)
+{
+	return READ_ONCE(mptcp_get_pernet(net)->subflow_retry_timeout);
+}
+
 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_retry_timeout",
+		.maxlen = sizeof(unsigned int),
+		.mode = 0644,
+		.proc_handler = proc_dointvec_jiffies,
+	},
 };
 
 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_retry_timeout;
 
 	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..2683e359695c 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -22,6 +22,20 @@ struct mptcp_pm_add_addr {
 	struct rcu_head		rcu;
 };
 
+struct mptcp_pm_retry {
+	struct list_head	list;
+	struct mptcp_pm_local	local;
+	struct mptcp_addr_info	remote;
+	u8			attempts;
+	bool			pending;	/* timer fired, worker not run yet */
+	bool			disabled;	/* local or remote address removed */
+	bool			timer_done;
+	struct timer_list	timer;
+	struct mptcp_sock	*sock;
+};
+
+#define MPTCP_PM_RETRY_MAX	8
+
 static DEFINE_SPINLOCK(mptcp_pm_list_lock);
 static LIST_HEAD(mptcp_pm_list);
 
@@ -261,6 +275,8 @@ static bool subflow_in_rm_list(const struct mptcp_subflow_context *subflow,
 }
 
 static void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk);
+static bool mptcp_pm_schedule_work(struct mptcp_sock *msk,
+				   enum mptcp_pm_status new_status);
 static void
 mptcp_pm_addr_send_ack_avoid_list(struct mptcp_sock *msk,
 				  const struct mptcp_rm_list *rm_list)
@@ -493,6 +509,250 @@ static void mptcp_pm_free_announced_list(struct mptcp_sock *msk)
 	}
 }
 
+/* Subflows closed after an error can be re-created later on, see the
+ * 'subflow_retry_timeout' sysctl. Entries are kept until the msk is destroyed
+ * to track the exponential backoff, and are disabled when the corresponding
+ * local endpoint or remote address is removed.
+ */
+static void mptcp_pm_retry_timer(struct timer_list *timer)
+{
+	struct mptcp_pm_retry *entry = timer_container_of(entry, timer, timer);
+	struct mptcp_sock *msk = entry->sock;
+	struct sock *sk = (struct sock *)msk;
+
+	pr_debug("msk=%p\n", msk);
+
+	spin_lock_bh(&msk->pm.lock);
+	/* the timer might have been re-armed while this callback was waiting
+	 * for the PM lock: let the next expiration handle the entry.
+	 */
+	if (!timer_pending(&entry->timer)) {
+		if (!entry->disabled &&
+		    !(msk->pm.status & BIT(MPTCP_PM_DESTROYING))) {
+			entry->pending = true;
+			mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_RETRY);
+		}
+
+		/* if sock_put calls sk_free: avoid waiting for this timer */
+		entry->timer_done = true;
+	}
+	spin_unlock_bh(&msk->pm.lock);
+
+	sock_put(sk);
+}
+
+/* To be called while pm->lock is held */
+static void mptcp_pm_retry_arm(struct mptcp_sock *msk,
+			       struct mptcp_pm_retry *entry)
+{
+	unsigned long max = mptcp_subflow_retry_timeout(sock_net((struct sock *)msk));
+	unsigned long timeout = TCP_TIMEOUT_INIT << min_t(u8, entry->attempts, 16);
+
+	entry->pending = false;
+	entry->timer_done = false;
+	if (entry->attempts < U8_MAX)
+		entry->attempts++;
+
+	sk_reset_timer((struct sock *)msk, &entry->timer,
+		       jiffies + min(timeout, max));
+}
+
+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;
+}
+
+static void mptcp_pm_retry_add(struct mptcp_sock *msk,
+			       const struct mptcp_subflow_context *subflow)
+{
+	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+	bool established = READ_ONCE(subflow->fully_established);
+	struct mptcp_addr_info local, remote;
+	int err = READ_ONCE(ssk->sk_err);
+	struct sock *sk = (struct sock *)msk;
+	struct mptcp_pm_retry *entry;
+	int nr = 0;
+
+	if (!err || !mptcp_subflow_retry_timeout(sock_net(sk)))
+		return;
+
+	/* 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;
+
+	/* 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;
+
+	if (sk->sk_state != TCP_ESTABLISHED || __mptcp_check_fallback(msk))
+		return;
+
+	mptcp_local_address((struct sock_common *)ssk, &local);
+	local.port = 0;
+	local.id = subflow_get_local_id(subflow);
+	mptcp_remote_address((struct sock_common *)ssk, &remote);
+	remote.id = READ_ONCE(subflow->remote_id);
+
+	if (!remote.id && READ_ONCE(msk->pm.remote_deny_join_id0))
+		return;
+
+	spin_lock_bh(&msk->pm.lock);
+	if (msk->pm.status & BIT(MPTCP_PM_DESTROYING))
+		goto unlock;
+
+	list_for_each_entry(entry, &msk->pm.retry_list, list) {
+		if (mptcp_addresses_equal(&entry->local.addr, &local, false) &&
+		    mptcp_addresses_equal(&entry->remote, &remote, true))
+			goto found;
+		nr++;
+	}
+
+	if (nr >= MPTCP_PM_RETRY_MAX)
+		goto unlock;
+
+	entry = kzalloc_obj(*entry, GFP_ATOMIC);
+	if (!entry)
+		goto unlock;
+
+	entry->sock = msk;
+	timer_setup(&entry->timer, mptcp_pm_retry_timer, 0);
+	list_add_tail(&entry->list, &msk->pm.retry_list);
+
+found:
+	/* the path worked for a while: restart the backoff */
+	if (established)
+		entry->attempts = 0;
+
+	entry->local.addr = local;
+	entry->local.flags = subflow->request_bkup || subflow->backup ?
+			     MPTCP_PM_ADDR_FLAG_BACKUP : 0;
+	entry->local.ifindex = ssk->sk_bound_dev_if;
+	entry->remote = remote;
+	entry->disabled = false;
+
+	pr_debug("msk=%p local_id=%d remote_id=%d err=%d attempts=%u\n", msk,
+		 local.id, remote.id, err, entry->attempts);
+	mptcp_pm_retry_arm(msk, entry);
+
+unlock:
+	spin_unlock_bh(&msk->pm.lock);
+}
+
+static bool mptcp_pm_retry_has_subflow(const struct mptcp_sock *msk,
+				       const struct mptcp_pm_retry *entry)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_addr_info local, remote;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+		if (!((1 << inet_sk_state_load(ssk)) &
+		      (TCPF_ESTABLISHED | TCPF_SYN_SENT | TCPF_SYN_RECV)))
+			continue;
+
+		mptcp_local_address((struct sock_common *)ssk, &local);
+		mptcp_remote_address((struct sock_common *)ssk, &remote);
+		if (mptcp_addresses_equal(&entry->local.addr, &local, false) &&
+		    mptcp_addresses_equal(&entry->remote, &remote, true))
+			return true;
+	}
+
+	return false;
+}
+
+/* Called under PM lock */
+static void mptcp_pm_retry_subflows(struct mptcp_sock *msk)
+{
+	struct sock *sk = (struct sock *)msk;
+	struct mptcp_pm_retry *entry;
+
+	/* Entries are only freed at destroy time, and new ones are added at
+	 * the tail: it is safe to release the PM lock while iterating.
+	 */
+	list_for_each_entry(entry, &msk->pm.retry_list, list) {
+		struct mptcp_addr_info remote;
+		struct mptcp_pm_local local;
+
+		if (!entry->pending || entry->disabled)
+			continue;
+
+		entry->pending = false;
+		if (sk->sk_state != TCP_ESTABLISHED ||
+		    mptcp_pm_retry_has_subflow(msk, entry))
+			continue;
+
+		if (msk->pm.extra_subflows >= mptcp_pm_get_limit_extra_subflows(msk)) {
+			mptcp_pm_retry_arm(msk, entry);
+			continue;
+		}
+
+		msk->pm.extra_subflows++;
+		local = entry->local;
+		remote = entry->remote;
+
+		spin_unlock_bh(&msk->pm.lock);
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_SUBFLOWRETRY);
+		if (__mptcp_subflow_connect(sk, &local, &remote)) {
+			spin_lock_bh(&msk->pm.lock);
+			mptcp_pm_retry_arm(msk, entry);
+			continue;
+		}
+		spin_lock_bh(&msk->pm.lock);
+	}
+}
+
+/* To be called while pm->lock is held. A NULL local address and a negative
+ * remote ID disable all entries.
+ */
+void mptcp_pm_retry_disable(struct mptcp_sock *msk,
+			    const struct mptcp_addr_info *local, int remote_id)
+{
+	struct mptcp_pm_retry *entry;
+
+	list_for_each_entry(entry, &msk->pm.retry_list, list) {
+		if (local && !mptcp_addresses_equal(&entry->local.addr, local, false))
+			continue;
+		if (remote_id >= 0 && entry->remote.id != remote_id)
+			continue;
+
+		entry->disabled = true;
+		entry->pending = false;
+		sk_stop_timer((struct sock *)msk, &entry->timer);
+	}
+}
+
+static void mptcp_pm_free_retry_list(struct mptcp_sock *msk)
+{
+	struct mptcp_pm_retry *entry, *tmp;
+	struct sock *sk = (struct sock *)msk;
+	LIST_HEAD(free_list);
+
+	spin_lock_bh(&msk->pm.lock);
+	list_splice_init(&msk->pm.retry_list, &free_list);
+	spin_unlock_bh(&msk->pm.lock);
+
+	list_for_each_entry_safe(entry, tmp, &free_list, list) {
+		if (!entry->timer_done)
+			sk_stop_timer_sync(sk, &entry->timer);
+		kfree(entry);
+	}
+}
+
 /* path manager command handlers */
 
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
@@ -667,6 +927,9 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 	bool update_subflows;
 
 	update_subflows = subflow->request_join || subflow->mp_join;
+	if (mptcp_pm_is_kernel(msk))
+		mptcp_pm_retry_add(msk, subflow);
+
 	if (mptcp_pm_is_userspace(msk)) {
 		if (update_subflows) {
 			spin_lock_bh(&pm->lock);
@@ -788,6 +1051,9 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 		u8 rm_id = rm_list->ids[i];
 		bool removed = false;
 
+		if (rm_type == MPTCP_MIB_RMADDR)
+			mptcp_pm_retry_disable(msk, NULL, rm_id);
+
 		mptcp_for_each_subflow_safe(msk, subflow, tmp) {
 			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 			u8 remote_id = READ_ONCE(subflow->remote_id);
@@ -1150,6 +1416,10 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 		pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);
 		mptcp_pm_rm_addr_recv(msk);
 	}
+	if (pm->status & BIT(MPTCP_PM_SUBFLOW_RETRY)) {
+		pm->status &= ~BIT(MPTCP_PM_SUBFLOW_RETRY);
+		mptcp_pm_retry_subflows(msk);
+	}
 	__mptcp_pm_kernel_worker(msk);
 
 	spin_unlock_bh(&msk->pm.lock);
@@ -1192,6 +1462,7 @@ void mptcp_pm_destroy(struct mptcp_sock *msk)
 	spin_unlock_bh(&msk->pm.lock);
 
 	mptcp_pm_free_announced_list(msk);
+	mptcp_pm_free_retry_list(msk);
 
 	/* Free the userspace local address list unconditionally: the socket
 	 * can be reused (mptcp_disconnect()) and re-selected to a different PM
@@ -1222,6 +1493,7 @@ void mptcp_pm_data_init(struct mptcp_sock *msk)
 {
 	spin_lock_init(&msk->pm.lock);
 	INIT_LIST_HEAD(&msk->pm.anno_list);
+	INIT_LIST_HEAD(&msk->pm.retry_list);
 	INIT_LIST_HEAD(&msk->pm.userspace_pm_local_addr_list);
 	mptcp_pm_data_reset(msk);
 }
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..e3dce702fad2 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1106,6 +1106,7 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 		list.ids[0] = mptcp_endp_get_local_id(msk, addr);
 
 		spin_lock_bh(&msk->pm.lock);
+		mptcp_pm_retry_disable(msk, addr, -1);
 		if (remove_subflow)
 			mptcp_pm_rm_subflow(msk, &list);
 		if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
@@ -1156,6 +1157,7 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
 		spin_lock_bh(&msk->pm.lock);
 		if (announced)
 			msk->pm.add_addr_signaled--;
+		mptcp_pm_retry_disable(msk, &msk_local, -1);
 		mptcp_pm_remove_addr(msk, &list);
 		mptcp_pm_rm_subflow(msk, &list);
 		__mark_subflow_endp_available(msk, 0);
@@ -1267,6 +1269,7 @@ static void mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
 		mptcp_pm_rm_subflow(msk, &slist);
 	/* Reset counters: maybe some subflows have been removed before */
 	if (!more) {
+		mptcp_pm_retry_disable(msk, NULL, -1);
 		bitmap_fill(msk->pm.id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 		msk->pm.local_addr_used = 0;
 	}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ed2f363bb915..d1ff62d1f943 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -204,6 +204,7 @@ enum mptcp_pm_status {
 	MPTCP_PM_RM_ADDR_RECEIVED,
 	MPTCP_PM_ESTABLISHED,
 	MPTCP_PM_SUBFLOW_ESTABLISHED,
+	MPTCP_PM_SUBFLOW_RETRY,
 	MPTCP_PM_ALREADY_ESTABLISHED,	/* persistent status, set after ESTABLISHED event */
 	MPTCP_PM_MPC_ENDPOINT_ACCOUNTED, /* persistent status, set after MPC local address is
 					  * accounted int id_avail_bitmap
@@ -235,6 +236,7 @@ struct mptcp_pm_data {
 	struct mptcp_addr_info local;
 	struct mptcp_addr_info remote;
 	struct list_head anno_list;
+	struct list_head retry_list;
 	struct list_head userspace_pm_local_addr_list;
 	struct mptcp_pm_ops *ops;
 
@@ -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);
+unsigned int mptcp_subflow_retry_timeout(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);
@@ -853,6 +856,8 @@ void mptcp_set_state(struct sock *sk, int state);
 
 bool mptcp_addresses_equal(const struct mptcp_addr_info *a,
 			   const struct mptcp_addr_info *b, bool use_port);
+void mptcp_pm_retry_disable(struct mptcp_sock *msk,
+			    const struct mptcp_addr_info *local, int remote_id);
 void mptcp_local_address(const struct sock_common *skc,
 			 struct mptcp_addr_info *addr);
 void mptcp_remote_address(const struct sock_common *skc,
-- 
2.25.1

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

[Anil Kaushik]

Test the new sysctl: during a transfer, drop all the packets the client
sends on one link, without bringing it down, until the subflow is closed
and a retry happens. Then restore the link and check the subflow comes
back.

Both an extra (MP_JOIN) subflow and the initial subflow are tested.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaeda4f..a4549d8f291e 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2151,6 +2151,78 @@ wait_attempt_fail()
 	return 1
 }
 
+# $1: ns
+wait_sf_retry()
+{
+	local ns="${1}"
+	local cnt
+
+	for _ in $(seq $((timeout_poll * 10))); do
+		cnt=$(mptcp_lib_get_counter "${ns}" "MPTcpExtSubflowRetry")
+		[ "${cnt:-0}" -gt 0 ] && 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: index of the client link to break
+#
+# The link stays up, but all the packets sent by the client on it are dropped:
+# the subflow using it is closed after a few retransmissions, then re-created
+# by the client once the link is back.
+run_sf_retry()
+{
+	local link="${1}"
+	local retry_nr
+	local tests_pid
+
+	init_shapers
+	ip netns exec $ns2 sysctl -q net.mptcp.subflow_retry_timeout=1
+	ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
+	pm_nl_set_limits $ns1 0 2
+	pm_nl_set_limits $ns2 0 2
+	pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
+	test_linkfail=32768 \
+		run_tests $ns1 $ns2 10.0.1.1 &
+	tests_pid=$!
+
+	wait_mpj 1
+	tc -n $ns2 qdisc change dev ns2eth${link} root netem rate 20mbit \
+		delay 1ms loss 100%
+	wait_sf_retry $ns2
+	tc -n $ns2 qdisc change dev ns2eth${link} root netem rate 20mbit \
+		delay 1ms
+	if ! wait ${tests_pid}; then
+		fail_test "transfer failed"
+		return
+	fi
+
+	# one MPJ SYN per attempt, then a single successful re-creation
+	retry_nr=$(mptcp_lib_get_counter $ns2 "MPTcpExtSubflowRetry")
+	join_syn_tx=$((1 + ${retry_nr:-0})) \
+		chk_join_nr 2 2 2
+	chk_sf_retry_nr $ns2 1
+}
+
 set_userspace_pm()
 {
 	local ns=$1
@@ -2585,6 +2657,18 @@ link_failure_tests()
 		chk_stale_nr $ns2 1 -1 2
 		chk_link_usage $ns2 ns2eth3 $cinsent 50
 	fi
+
+	# MPJ subflow closed after a silent link failure, then re-created
+	if reset "subflow retry after link failure" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/subflow_retry_timeout'; then
+		run_sf_retry 3
+	fi
+
+	# same with the initial subflow: re-created with an MPJ using ID 0
+	if reset "initial subflow retry after link failure" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/subflow_retry_timeout'; then
+		run_sf_retry 1
+	fi
 }
 
 add_addr_timeout_tests()
-- 
2.25.1

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

[Matthieu Baerts]

Hi Anil,

On 12/10/2026 19:17, Anil Kaushik wrote:
> With the in-kernel PM, a subflow closed after an error is never
> re-created, even once the path is back (#440). For example, with fiber
> as main link and Starlink as backup, connections never return to the
> fiber after an outage.

Thank you for looking at that. Here is a quick review (based on the
preview you sent earlier). I might not have the opportunity to do more
before November.

> Patch 1 fixes a selftest flake: write() can return EAGAIN after POLLOUT
> when a subflow closes.

When did you see this? With the other patches applied or before? Just
wonder if this is worth backporting this, but I don't think I saw the
issue before (or maybe it was very rare). Maybe we would need a Fixes tag?

(The "Assisted-by" tag is only used on this patch, did you want to add
it on the other ones as well?)

> Patch 2 adds an opt-in sysctl, net.mptcp.subflow_retry_timeout, to
> re-create such subflows with an exponential backoff.
> Patch 3 adds the selftests.
> 
> Questions:
>  - sysctl, or an endpoint flag?

I still need to think if this should be a per-endpoint setting, or a per
netns one. I guess people might want to use it by default, and disable
it in some cases. Possibly this would need to be disabled per endpoint
with a flag, I don't know. We might not want to dynamically react when
the flag is changed, (except if this is really needed): that caused
enough issues in the past.

With a sysctl, it is a bit "strange" because we have in-kernel PM
options that are all configurable via netlink (ip mptcp). An alternative
is to have general settings per netns configurable via netlink, similar
to how the limits are configured. (In fact, that can also be an extra
limit in fact). Maybe later, we could have a flag to disable retries per
endpoint.

>  - the re-created initial subflow counts in the subflows limit, OK?

The "subflows" limit is for the extra subflows. So yes, if you have 2
paths with 1 extra one and the initial one is recreated, it now counts
as an extra one.

>  - should retries stop after some attempts?

I guess this should be similar to what TCP is doing with the "retrans"
parameters: set a maximum number of attempts. No?

We usually try to reduce the amount of data stored/copied here. I'm not
a fan of these new entries: here, a re-connect will already retry a few
times before giving up (depending on syn_retries). We will need to deal
with two ongoing timers, that doesn't look good. Can we not instead only
deal with subflow sockets and their timers? If an attempt fails (and the
socket creation timer is stopped), retry later with the same socket?

Now, regarding the code, if it is specific to the in-kernel one, it
should go exclusively to pm_kernel.c: pm.c is for shared code between PMs.

For the selftests, maybe better to use netfilter to block links instead
of TC? Maybe can you have a single test where there is a failure on the
extra subflow, then on the initial one? Try to have a "quick" test, it
is not clear how long this one takes, but it would feel better to run a
very long connection in the background, test what you need to test with
the 'wait_*' functions as you did, then stop it, similar to what you can
see in many tests from "endpoint_tests".

Regarding the tests, just in case, there is a docker image to help you
there:

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

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: pm: infinite loop when net.mptcp.subflow_retry_timeout is 0
- [Medium] mptcp: pm: retry list slot exhaustion due to uncollected disabled entries
--

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

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

This commit introduces a mechanism to retry creating subflows that were
closed due to an error, using the in-kernel path manager. It adds a sysctl
to configure the exponential backoff retry interval, up to a defined maximum
delay.

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

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d7c5b50b34cc9..2683e359695cd 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
[ ... ]
> @@ -493,6 +509,250 @@ static void mptcp_pm_free_announced_list(struct mptcp_sock *msk)
>  	}
>  }
>  
> +/* Subflows closed after an error can be re-created later on, see the
> + * 'subflow_retry_timeout' sysctl. Entries are kept until the msk is destroyed
> + * to track the exponential backoff, and are disabled when the corresponding
> + * local endpoint or remote address is removed.
> + */
> +static void mptcp_pm_retry_timer(struct timer_list *timer)
[ ... ]
> +/* To be called while pm->lock is held */
> +static void mptcp_pm_retry_arm(struct mptcp_sock *msk,
> +			       struct mptcp_pm_retry *entry)
> +{
> +	unsigned long max = mptcp_subflow_retry_timeout(sock_net((struct sock *)msk));
> +	unsigned long timeout = TCP_TIMEOUT_INIT << min_t(u8, entry->attempts, 16);
> +
> +	entry->pending = false;
> +	entry->timer_done = false;
> +	if (entry->attempts < U8_MAX)
> +		entry->attempts++;
> +
> +	sk_reset_timer((struct sock *)msk, &entry->timer,
> +		       jiffies + min(timeout, max));

[Severity: High]
What happens if the sysctl is dynamically changed to 0 while there are active
entries in the retry list?

It appears mptcp_pm_retry_arm() calculates max as 0, which results in arming
the timer with jiffies + 0. The timer would then fire immediately and schedule
the PM worker mptcp_pm_retry_subflows(). If the connection fails again, the
worker immediately calls mptcp_pm_retry_arm() again.

Could this create an infinite, zero-delay loop between the timer softirq and
the PM worker queue that monopolizes a CPU core? Perhaps the code should
explicitly check if the sysctl is 0 and halt retries for existing entries,
rather than clamping the timer delay to 0.

> +}
> +
> +static bool mptcp_pm_retry_path_error(int err)
[ ... ]
> +static void mptcp_pm_retry_add(struct mptcp_sock *msk,
> +			       const struct mptcp_subflow_context *subflow)
> +{
[ ... ]
> +	spin_lock_bh(&msk->pm.lock);
> +	if (msk->pm.status & BIT(MPTCP_PM_DESTROYING))
> +		goto unlock;
> +
> +	list_for_each_entry(entry, &msk->pm.retry_list, list) {
> +		if (mptcp_addresses_equal(&entry->local.addr, &local, false) &&
> +		    mptcp_addresses_equal(&entry->remote, &remote, true))
> +			goto found;
> +		nr++;
> +	}
> +
> +	if (nr >= MPTCP_PM_RETRY_MAX)
> +		goto unlock;

[Severity: Medium]
When mptcp_pm_retry_disable() marks retry list entries as disabled, they are
not removed from the retry_list.

During mptcp_pm_retry_add(), the list iteration counts all entries via nr++,
including disabled ones. If normal network events accumulate 8 disabled
entries over the lifetime of a connection, it looks like this limit will be
reached and no new subflows can ever be retried.

Can this lead to permanent slot exhaustion on this connection? Should disabled
entries be evicted or skipped in the count to avoid this?

> +
> +	entry = kzalloc_obj(*entry, GFP_ATOMIC);
> +	if (!entry)
> +		goto unlock;
> +
> +	entry->sock = msk;
> +	timer_setup(&entry->timer, mptcp_pm_retry_timer, 0);
> +	list_add_tail(&entry->list, &msk->pm.retry_list);
[ ... ]

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

Re: [RFC PATCH mptcp-next 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/38285613995

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


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 1/3] selftests: mptcp: connect: retry write() on EAGAIN

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a user-visible selftest failure but carries no
  Fixes: tag.
- [Low] The new `if (errno == EAGAIN) continue;` in copyfd_io_poll()
  jumps straight back to poll() with no retry limit, no backoff and no…

--

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

With this patch, copyfd_io_poll() in mptcp_connect goes back to poll()
when write() fails with EAGAIN, instead of treating it as fatal. The
reason given is that the MPTCP send buffer can shrink after poll() reports
POLLOUT, for example when a subflow is closed.

> Assisted-by: LLM
> Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>

[Severity: Low]
Should this patch have a Fixes: tag?

The poll loop was added in commit 048d19d444be ("mptcp: add basic
kselftest for mptcp_connect"). Since then, copyfd_io_poll() has treated
an EAGAIN from write() on the non-blocking peerfd as fatal. That commit
already calls set_nonblock(peerfd) before the loop, and it added the
perror("write") / return 111 exit.

The window described in the commit message also exists in the current
tree. __mptcp_close_ssk() ends with:

out:
	__mptcp_sync_sndbuf(sk);

This recomputes sk_sndbuf from the remaining subflows only. Existing
cases in mptcp_join.sh (remove and link failure) also close subflows
mid-transfer, so the problem does not look limited to the new test.

Documentation/process/coding-assistants.rst also asks for a Fixes tag
on bug fixes made with an assistant.

Could this get a Fixes: tag for 048d19d444be, or for the kernel change
that first made the sndbuf shrink reachable? It might also be a candidate
for -net.

If the failure only shows up with the new test in this series
("selftests: mptcp: join: validate subflow retry after link failure"),
could the commit message say so? It would also help to say which test or
CI run hit the failure, and how often.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..5bcebc7218647 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;

[Severity: Low]
Can this loop spin forever and never reach the poll_timeout check?

The continue goes straight back to poll() with no retry limit, backoff or
deadline. do_rnd_write() returns as soon as write() fails, so its
usleep(200000) calls are skipped:

do_rnd_write() {
    ...
	bw = write(fd, buf, do_w);
	if (bw < 0)
		return bw;
    ...
}

The sndbuf shrink case from the commit message looks fine. In that case
mptcp_check_writeable() re-runs __mptcp_stream_is_writeable(), so poll()
blocks until mptcp_write_space() wakes it.

But mptcp_sendmsg() can also return -EAGAIN on a non-blocking socket for
reasons that poll does not check:

mptcp_sendmsg() {
    ...
			if (!mptcp_page_frag_refill(sk, pfrag))
				goto wait_for_memory;
    ...
		if (!sk_wmem_schedule(sk, total_ts))
			goto wait_for_memory;
    ...
}

One example is when tcp_mem is over its hard limit:

1. __sk_mem_raise_allocated() calls sk_stream_moderate_sndbuf().

2. It then fails the charge when wmem_queued + size < sk_sndbuf.

3. If only a small part of winfo->len is left and most queued data has
   been acked, every write() of that tail returns EAGAIN.

4. Meanwhile mptcp_check_writeable() still reports POLLOUT.

A memcg charge failure, or any mismatch between poll writability and
sendmsg, would end the same way.

poll() then returns at once on every pass, so the case 0 "poll timed out"
path never runs. The tool busy-loops until mptcp_lib_wait_timeout kills
it. Before this patch it failed right away with "write: Resource
temporarily unavailable".

Would a bounded retry count, a short backoff, or a deadline based on
poll_timeout keep the watchdog and its diagnostic working?

[ ... ]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 4 · 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).

- [High] This patch inserts MPTCP_PM_SUBFLOW_RETRY into the middle of
  enum mptcp_pm_status (net/mptcp/protocol.h:201-213).
- [Medium] mptcp_pm_retry_add() only filters by errno when the subflow
  never became fully established: `if (!established &&…
- [Medium] mptcp_pm_retry_add() counts every entry on msk->pm.retry_list,
  disabled ones included, and stops creating entries once…
- [Medium] mptcp_pm_retry_arm() reads the sysctl again on every call and
  arms the timer at `jiffies + min(timeout, max)`.
- [Medium] mptcp_pm_retry_add() matches an existing entry whether or not
  it is disabled, then sets `entry->disabled = false` and re-arms it.

--

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

This adds an opt-in per-netns sysctl, net.mptcp.subflow_retry_timeout.
When it is set, the in-kernel PM re-creates subflows closed after an
error, using the same addresses and an exponential backoff capped by the
sysctl value. Retry state is kept per msk in a new retry_list, disabled
on endpoint or remote address removal, and freed in mptcp_pm_destroy().

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

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d7c5b50b34cc9..2683e359695cd 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c

[ ... ]

> @@ -493,6 +509,250 @@ static void mptcp_pm_free_announced_list(struct mptcp_sock *msk)

[ ... ]

> +/* To be called while pm->lock is held */
> +static void mptcp_pm_retry_arm(struct mptcp_sock *msk,
> +			       struct mptcp_pm_retry *entry)
> +{
> +	unsigned long max = mptcp_subflow_retry_timeout(sock_net((struct sock *)msk));
> +	unsigned long timeout = TCP_TIMEOUT_INIT << min_t(u8, entry->attempts, 16);
> +
> +	entry->pending = false;
> +	entry->timer_done = false;
> +	if (entry->attempts < U8_MAX)
> +		entry->attempts++;
> +
> +	sk_reset_timer((struct sock *)msk, &entry->timer,
> +		       jiffies + min(timeout, max));
> +}

[Severity: Medium]
What happens here if net.mptcp.subflow_retry_timeout is set back to 0
while retry entries already exist? The documentation says "0 disables
the retries", but only mptcp_pm_retry_add() checks the sysctl for zero.

Timers that are already armed still fire. mptcp_pm_retry_timer() then
schedules MPTCP_PM_SUBFLOW_RETRY without looking at the sysctl, and
mptcp_pm_retry_subflows() re-arms the entry in two cases:

    if (msk->pm.extra_subflows >= mptcp_pm_get_limit_extra_subflows(msk)) {
        mptcp_pm_retry_arm(msk, entry);
    ...
    if (__mptcp_subflow_connect(sk, &local, &remote)) {
        ...
        mptcp_pm_retry_arm(msk, entry);

With max == 0, min(timeout, max) is 0, so the timer expires on the next
tick.

Can this loop every jiffy for each entry (up to 8 per msk)? It would
keep going while the limit is reached or __mptcp_subflow_connect() keeps
failing synchronously, for example when kernel_bind() fails because the
endpoint address is gone. In the connect failure case, each pass
creates, binds and releases a socket.

Also, proc_dointvec_jiffies has no bounds for this entry. Would a
negative write be stored as a large unsigned value that counts as
enabled?

[ ... ]

> +static void mptcp_pm_retry_add(struct mptcp_sock *msk,
> +			       const struct mptcp_subflow_context *subflow)
> +{

[ ... ]

> +	if (!err || !mptcp_subflow_retry_timeout(sock_net(sk)))
> +		return;
> +
> +	/* 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;

[Severity: Medium]
Should established subflows be filtered here too? Once a subflow has
been fully established, any non-zero sk_err passes this check.
subflow->reset_seen, reset_transient and reset_reason are never looked
at.

mptcp_incoming_options() records the peer's MP_RST flags:

    subflow->reset_seen = 1;
    subflow->reset_reason = mp_opt.reset_reason;
    subflow->reset_transient = mp_opt.reset_transient;

So a subflow the peer reset with T=0 would be re-created over the same
path. RFC 8684 section 3.6 says the receiver should not do that when the
T flag is clear.

Subflows this host resets itself for fatal protocol errors also pass.
subflow_check_data_avail() sets reset_transient = 0, a reset_reason of
MPTCP_RST_EMIDDLEBOX or MPTCP_RST_EMPTCP, and sk_err = EBADMSG.

Since established also resets the backoff further down:

    if (established)
        entry->attempts = 0;

could a path where a middlebox or the peer kills the subflow after
establishment get retried about every TCP_TIMEOUT_INIT (1s) for the life
of the connection?

The commit message says "A subflow the peer refused is not retried", and
the documentation mentions "a timeout or an unreachable path". Neither
seems to match this behaviour.

[ ... ]

> +	list_for_each_entry(entry, &msk->pm.retry_list, list) {
> +		if (mptcp_addresses_equal(&entry->local.addr, &local, false) &&
> +		    mptcp_addresses_equal(&entry->remote, &remote, true))
> +			goto found;
> +		nr++;
> +	}
> +
> +	if (nr >= MPTCP_PM_RETRY_MAX)
> +		goto unlock;

[Severity: Medium]
Does this count disabled entries against MPTCP_PM_RETRY_MAX? Entries are
only freed in mptcp_pm_free_retry_list() at destroy time.
mptcp_pm_retry_disable() only marks them disabled, and a successful
recovery doesn't remove an entry either.

Take a long-lived connection with address churn, such as an endpoint
removed and re-added with a new IP, or RM_ADDR/ADD_ADDR from the peer.
Or take a fullmesh setup with more than 8 local/remote pairs (the kernel
PM allows up to MPTCP_PM_SUBFLOWS_MAX = 64 subflows).

Would every failing pair after the 8th silently get no retries? The
commit message and the documentation don't mention this limit.

[ ... ]

> +found:
> +	/* the path worked for a while: restart the backoff */
> +	if (established)
> +		entry->attempts = 0;
> +
> +	entry->local.addr = local;
> +	entry->local.flags = subflow->request_bkup || subflow->backup ?
> +			     MPTCP_PM_ADDR_FLAG_BACKUP : 0;
> +	entry->local.ifindex = ssk->sk_bound_dev_if;
> +	entry->remote = remote;
> +	entry->disabled = false;

[Severity: Medium]
Can this re-enable an entry that an address or endpoint removal has just
disabled?

mptcp_pm_rm_addr_or_subflow() disables the entries first, then skips
subflows that are already in TCP_CLOSE:

    if (rm_type == MPTCP_MIB_RMADDR)
        mptcp_pm_retry_disable(msk, NULL, rm_id);

    mptcp_for_each_subflow_safe(msk, subflow, tmp) {
        ...
        if ((1 << inet_sk_state_load(ssk)) &
            (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
            continue;

Endpoint removal through mptcp_nl_remove_subflow_and_signal_addr() or the
flush path also calls mptcp_pm_retry_disable() and then
mptcp_pm_rm_subflow(), which goes through the same loop.

A subflow that died with ECONNRESET or ETIMEDOUT stays in TCP_CLOSE until
mptcp_worker() calls __mptcp_close_subflow(). That runs after
mptcp_pm_worker() in the same pass, and it can be deferred further while
ssk->sk_receive_queue is not empty.

The later call chain:

mptcp_close_ssk()
  mptcp_pm_subflow_check_next()
    mptcp_pm_retry_add()

then finds the disabled entry (or creates a new one), clears disabled
and re-arms the timer.

Would the PM then keep re-creating subflows towards a remote address ID
the peer withdrew, or from a local endpoint that was deleted? That seems
to contradict "Retries stop when the endpoint or the peer's address is
removed" in the commit message.

[ ... ]

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index ed2f363bb915c..d1ff62d1f9435 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -204,6 +204,7 @@ enum mptcp_pm_status {
>  	MPTCP_PM_RM_ADDR_RECEIVED,
>  	MPTCP_PM_ESTABLISHED,
>  	MPTCP_PM_SUBFLOW_ESTABLISHED,
> +	MPTCP_PM_SUBFLOW_RETRY,
>  	MPTCP_PM_ALREADY_ESTABLISHED,	/* persistent status, set after ESTABLISHED event */
>  	MPTCP_PM_MPC_ENDPOINT_ACCOUNTED, /* persistent status, set after MPC local address is
>  					  * accounted int id_avail_bitmap

[Severity: High]
Does this new enumerator push MPTCP_PM_DESTROYING out of the range of
pm.status?

After this insertion, MPTCP_PM_MPC_ENDPOINT_ACCOUNTED becomes 7 and
MPTCP_PM_DESTROYING becomes 8. struct mptcp_pm_data still has:

    u8		status;

So in mptcp_pm_destroy():

    msk->pm.status |= BIT(MPTCP_PM_DESTROYING);

BIT(8) is truncated and nothing is set. Every test of
status & BIT(MPTCP_PM_DESTROYING) then reads 0. That includes
mptcp_pm_announced_alloc(), mptcp_userspace_pm_append_new_local_addr(),
and the new checks in mptcp_pm_retry_timer() and mptcp_pm_retry_add().

Doesn't this silently undo the teardown fence added by commit
efc33b5102ff ("mptcp: pm: fix memory leak from alloc-during-teardown
race")?

A userspace PM MPTCP_PM_CMD_ANNOUNCE that races with close() or
disconnect() could again add entries after mptcp_pm_destroy() has
spliced and freed the lists. That covers mptcp_pm_add_addr entries (with
an armed timer) and mptcp_pm_addr_entry entries, and they would be
leaked. The command only needs CAP_NET_ADMIN in the user namespace that
owns the netns.

The later patches in this series only touch the selftests, so nothing
else in the series seems to address this. Should status be widened to
hold the extra bit?

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] run_sf_retry() in
  tools/testing/selftests/net/mptcp/mptcp_join.sh runs the transfer in…
- [Medium] run_sf_retry() calls run_tests() as a background job
  (`test_linkfail=32768 run_tests ... &`, lines 2204-2205), so it runs…
- [Low] Both new subtests check only totals: MPJoinSynRx/SynAckRx/AckRx
  == 2 and MPTcpExtSubflowRetry >= 1.

--

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

This adds two mptcp_join.sh subtests for the new subflow_retry_timeout
sysctl. A transfer runs in the background while netem drops every packet
the client sends on one link. Once a subflow retry is counted, the link is
restored and the join counters are checked, for both an MP_JOIN subflow
and the initial subflow.

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

[ ... ]

> +run_sf_retry()
> +{
> +	local link="${1}"
> +	local retry_nr
> +	local tests_pid
> +
> +	init_shapers
> +	ip netns exec $ns2 sysctl -q net.mptcp.subflow_retry_timeout=1
> +	ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
> +	pm_nl_set_limits $ns1 0 2
> +	pm_nl_set_limits $ns2 0 2
> +	pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
> +	test_linkfail=32768 \
> +		run_tests $ns1 $ns2 10.0.1.1 &

[Severity: Medium]
Can this leak the two 32 MB input files?

Here run_tests() runs in a background subshell. If cinfail and sinfail
are still empty, the mktemp assignments it makes exist only in that
subshell:

run_tests() {
    ...
		if [ -z "$cinfail" ]; then
			cinfail=$(mktemp)
		fi
		make_file "$cinfail" "client" $size
    ...
		if [ -z "$sinfail" ]; then
			sinfail=$(mktemp)
		fi
		make_file "$sinfail" "server" $size
    ...
}

The parent's cleanup() EXIT trap then runs:

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

At that point cinfail and sinfail are still empty in the parent.

In a full run, the earlier foreground link failure tests set both
variables in the parent first. If only the new subtests are selected, for
example with ./mptcp_join.sh "subflow retry after link failure", reset()
skips those earlier tests.

In that case, does each run_sf_retry() call leave about 64 MB behind in
$TMPDIR?

> +	tests_pid=$!
> +
> +	wait_mpj 1
> +	tc -n $ns2 qdisc change dev ns2eth${link} root netem rate 20mbit \
> +		delay 1ms loss 100%

[Severity: Medium]
Is wait_mpj() enough to make sure both subflows are established before
the loss is applied?

wait_mpj() gives up after about one second, and the caller cannot tell
whether it timed out:

wait_mpj()
{
	...
	for _ in $(seq 10); do
		cnt=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinAckRx")
		[ "${cnt}" = "${exp_cnt}" ] && break
		sleep 0.1
	done
}

Before any connection is opened, the background job runs make_file()
twice with test_linkfail=32768, which writes about 64 MB from
/dev/urandom. Then do_transfer() starts nstat, the listener and the
client. With -c, cond_start_capture() also adds a fixed sleep 1 before
the listener starts.

If wait_mpj() times out first, the 100% loss hits the handshake rather
than an established subflow.

For link 3, the MP_JOIN SYN would be dropped and the subflow would stay
in SYN_SENT, where tcp_retries2 does not apply. mptcp_pm_retry_add() in
net/mptcp/pm.c only adds a retry for a subflow that was never established
after a path error:

	if (!established && !mptcp_pm_retry_path_error(err))
		return;

That path error only comes once the SYN retries time out, which is well
past the wait_sf_retry() budget. So MPTcpExtSubflowRetry would stay at 0.
For link 1, the MPC SYN itself would be dropped.

In both cases, wouldn't chk_join_nr 2 2 2 and chk_sf_retry_nr $ns2 1
report false failures, especially on debug kernels or with -c?

The other backgrounded tests in this file wait with wait_event() or
wait_attempt_fail(), not wait_mpj() alone.

> +	wait_sf_retry $ns2
> +	tc -n $ns2 qdisc change dev ns2eth${link} root netem rate 20mbit \
> +		delay 1ms

[ ... ]

> @@ -2585,6 +2657,18 @@ link_failure_tests()

[ ... ]

> +	# same with the initial subflow: re-created with an MPJ using ID 0
> +	if reset "initial subflow retry after link failure" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/subflow_retry_timeout'; then
> +		run_sf_retry 1
> +	fi

[Severity: Low]
This isn't a bug, but nothing in run_sf_retry() checks the "ID 0" part of
this comment. The checks in run_sf_retry() only look at totals:

	join_syn_tx=$((1 + ${retry_nr:-0})) \
		chk_join_nr 2 2 2
	chk_sf_retry_nr $ns2 1

mptcp_pm_retry_add() saves local.id, remote.id and the bound interface so
the subflow can be re-created on the same path.

A kernel change that reconnected on the wrong endpoint, or with the wrong
address ID, would produce the same counters and still pass.

Would it be worth also checking the addresses, interface and address ID
of the re-created subflow?

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