[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

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

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