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

[PATCH mptcp-next v4 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 in-kernel
PM now creates a new one to the same addresses. Its SYN retransmissions
space out the attempts. Their number is set by a new limit,
subflow-retries: 15 by default, about 20 minutes, 0 disables it.

Changes in v4, after Matthieu's review: netlink limit instead of the
sysctl, one new subflow with more SYN retransmissions instead of counted
retries, no MIB counter, and a test closing the connection during a
retry. Details in each patch.

v3: https://lore.kernel.org/mptcp/20261014143349.768138-1-anilkaushikwireless@gmail.com/
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/netlink/specs/mptcp_pm.yaml     |   9 ++
 include/uapi/linux/mptcp_pm.h                 |   1 +
 net/mptcp/mptcp_pm_gen.c                      |  10 +-
 net/mptcp/mptcp_pm_gen.h                      |   4 +-
 net/mptcp/pm.c                                |  20 +++
 net/mptcp/pm_kernel.c                         | 149 +++++++++++++++++-
 net/mptcp/protocol.c                          |   5 +
 net/mptcp/protocol.h                          |  18 +++
 .../selftests/net/mptcp/mptcp_connect.c       |  17 ++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 124 +++++++++++++++
 10 files changed, 344 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH mptcp-next v4 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 a poll timeout is set, still
fail after as many retries, 1ms apart: 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>
---

Notes:
    v4:
     - count the EAGAIN retries, not ms (Sashiko)
     - no limit when there is no poll timeout, -t 0 (Sashiko)
    v3:
     - do not wait forever on EAGAIN (Sashiko)
     - wrap lines over 80 columns
    v2:
     - Fixes tag

 .../testing/selftests/net/mptcp/mptcp_connect.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 178d98d91fea..b9fd0bdab748 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_cnt = 0;
 
 	set_nonblock(peerfd, true);
 
@@ -711,6 +712,21 @@ 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 &&
+					    (poll_timeout < 0 ||
+					     ++eagain_cnt <= poll_timeout)) {
+						usleep(1000);
+						continue;
+					}
+
 					/* expected reset, continue to read */
 					if (cfg_rcv_trunc &&
 					    (errno == ECONNRESET ||
@@ -723,6 +739,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 					return 111;
 				}
 
+				eagain_cnt = 0;
 				winfo->off += bw;
 				winfo->len -= bw;
 				total_wlen += bw;
-- 
2.25.1

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

When TCP closes a subflow created by this side after an error, the PM
now creates a new one with the same addresses. Its SYN retransmissions
space out the attempts. Their number is set by a new limit,
subflow-retries: 15 by default, about 20 minutes, and 0 disables it. If
this new subflow cannot connect, it is not retried again. Once it is
established, it is handled like any other subflow. Re-creating the
initial subflow uses one of the additional subflows allowed by the
limits.

Subflows refused by the peer, reset by the peer with an MP_RST without
the transient flag (RFC 8684 section 3.6), aborted locally, or whose
endpoint has been removed are not retried.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/440
Assisted-by: LLM
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---

Notes:
    v4 (Matthieu):
     - netlink limit, on by default, instead of the sysctl
     - one new subflow per failure, its SYN retries set from the limit: no
       socket reuse, no attempts counter, no 60s check
     - no MIB counter
     - not retried if the endpoint was removed, instead of the pm.c change
     - called through pm.c
     - new bit in the subflow context, no new hole
     - commit message: MP_RST, not any RST (Sashiko)
    v3 (Sashiko):
     - no retry after MP_RST without the T flag, local abort or EBADMSG
     - no retry on a removed endpoint or address
     - use only request_bkup for the backup flag
    v2:
     - use the subflow sockets, no list or timer
     - code moved to pm_kernel.c

 Documentation/netlink/specs/mptcp_pm.yaml |   9 ++
 include/uapi/linux/mptcp_pm.h             |   1 +
 net/mptcp/mptcp_pm_gen.c                  |  10 +-
 net/mptcp/mptcp_pm_gen.h                  |   4 +-
 net/mptcp/pm.c                            |  20 +++
 net/mptcp/pm_kernel.c                     | 149 +++++++++++++++++++++-
 net/mptcp/protocol.c                      |   5 +
 net/mptcp/protocol.h                      |  18 +++
 8 files changed, 203 insertions(+), 13 deletions(-)

diff --git a/Documentation/netlink/specs/mptcp_pm.yaml b/Documentation/netlink/specs/mptcp_pm.yaml
index 39f3facc38e5..1b481908980c 100644
--- a/Documentation/netlink/specs/mptcp_pm.yaml
+++ b/Documentation/netlink/specs/mptcp_pm.yaml
@@ -196,6 +196,12 @@ attribute-sets:
         name: addr-remote
         type: nest
         nested-attributes: address
+      -
+        name: subflow-retries
+        type: u32
+        doc: |
+          SYN retransmissions of a subflow re-created after an error, 0 to
+          not re-create it.
   -
     name: event-attr
     enum-name: mptcp-event-attr
@@ -327,6 +333,7 @@ operations:
           attributes:
             - rcv-add-addrs
             - subflows
+            - subflow-retries
     -
       name: get-limits
       doc: Get protocol limits
@@ -337,10 +344,12 @@ operations:
           attributes:
             - rcv-add-addrs
             - subflows
+            - subflow-retries
         reply:
           attributes:
             - rcv-add-addrs
             - subflows
+            - subflow-retries
     -
       name: set-flags
       doc: Change endpoint flags
diff --git a/include/uapi/linux/mptcp_pm.h b/include/uapi/linux/mptcp_pm.h
index fe9863d75350..43e9643deda9 100644
--- a/include/uapi/linux/mptcp_pm.h
+++ b/include/uapi/linux/mptcp_pm.h
@@ -102,6 +102,7 @@ enum {
 	MPTCP_PM_ATTR_TOKEN,
 	MPTCP_PM_ATTR_LOC_ID,
 	MPTCP_PM_ATTR_ADDR_REMOTE,
+	MPTCP_PM_ATTR_SUBFLOW_RETRIES,
 
 	__MPTCP_ATTR_AFTER_LAST
 };
diff --git a/net/mptcp/mptcp_pm_gen.c b/net/mptcp/mptcp_pm_gen.c
index c180930a8e0e..127a1fadaac4 100644
--- a/net/mptcp/mptcp_pm_gen.c
+++ b/net/mptcp/mptcp_pm_gen.c
@@ -44,15 +44,17 @@ const struct nla_policy mptcp_pm_flush_addrs_nl_policy[MPTCP_PM_ENDPOINT_ADDR +
 };
 
 /* MPTCP_PM_CMD_SET_LIMITS - do */
-const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = {
+const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOW_RETRIES + 1] = {
 	[MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, },
 	[MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, },
+	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = { .type = NLA_U32, },
 };
 
 /* MPTCP_PM_CMD_GET_LIMITS - do */
-const struct nla_policy mptcp_pm_get_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = {
+const struct nla_policy mptcp_pm_get_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOW_RETRIES + 1] = {
 	[MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, },
 	[MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, },
+	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = { .type = NLA_U32, },
 };
 
 /* MPTCP_PM_CMD_SET_FLAGS - do */
@@ -127,7 +129,7 @@ const struct genl_ops mptcp_pm_nl_ops[11] = {
 		.validate	= GENL_DONT_VALIDATE_STRICT,
 		.doit		= mptcp_pm_nl_set_limits_doit,
 		.policy		= mptcp_pm_set_limits_nl_policy,
-		.maxattr	= MPTCP_PM_ATTR_SUBFLOWS,
+		.maxattr	= MPTCP_PM_ATTR_SUBFLOW_RETRIES,
 		.flags		= GENL_UNS_ADMIN_PERM,
 	},
 	{
@@ -135,7 +137,7 @@ const struct genl_ops mptcp_pm_nl_ops[11] = {
 		.validate	= GENL_DONT_VALIDATE_STRICT,
 		.doit		= mptcp_pm_nl_get_limits_doit,
 		.policy		= mptcp_pm_get_limits_nl_policy,
-		.maxattr	= MPTCP_PM_ATTR_SUBFLOWS,
+		.maxattr	= MPTCP_PM_ATTR_SUBFLOW_RETRIES,
 	},
 	{
 		.cmd		= MPTCP_PM_CMD_SET_FLAGS,
diff --git a/net/mptcp/mptcp_pm_gen.h b/net/mptcp/mptcp_pm_gen.h
index b9280bcb43f5..5b4dee8a0a34 100644
--- a/net/mptcp/mptcp_pm_gen.h
+++ b/net/mptcp/mptcp_pm_gen.h
@@ -23,9 +23,9 @@ extern const struct nla_policy mptcp_pm_get_addr_nl_policy[MPTCP_PM_ATTR_TOKEN +
 
 extern const struct nla_policy mptcp_pm_flush_addrs_nl_policy[MPTCP_PM_ENDPOINT_ADDR + 1];
 
-extern const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1];
+extern const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOW_RETRIES + 1];
 
-extern const struct nla_policy mptcp_pm_get_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1];
+extern const struct nla_policy mptcp_pm_get_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOW_RETRIES + 1];
 
 extern const struct nla_policy mptcp_pm_set_flags_nl_policy[MPTCP_PM_ATTR_ADDR_REMOTE + 1];
 
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d7c5b50b34cc..1bea4012d7f1 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -694,6 +694,26 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 	spin_unlock_bh(&pm->lock);
 }
 
+/* Called with the msk socket lock held, before closing a subflow: true if
+ * it has to be re-created once closed, see mptcp_pm_subflow_retry().
+ */
+bool mptcp_pm_subflow_retry_check(struct mptcp_sock *msk,
+				  const struct mptcp_subflow_context *subflow,
+				  struct mptcp_pm_retry *retry)
+{
+	if (!mptcp_pm_is_kernel(msk))
+		return false;
+
+	return mptcp_pm_kernel_retry_check(msk, subflow, retry);
+}
+
+void mptcp_pm_subflow_retry(struct mptcp_sock *msk,
+			    const struct mptcp_pm_retry *retry)
+{
+	if (mptcp_pm_is_kernel(msk))
+		mptcp_pm_kernel_retry(msk, retry);
+}
+
 void mptcp_pm_add_addr_received(const struct sock *ssk,
 				const struct mptcp_addr_info *addr)
 {
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..6c081a9c664c 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -25,6 +25,7 @@ struct pm_nl_pernet {
 	u8			endp_fullmesh_max;
 	u8			limit_add_addr_accepted;
 	u8			limit_extra_subflows;
+	u8			limit_subflow_retries;
 	u8			next_id;
 	DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 };
@@ -1399,7 +1400,8 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 	return msg->len;
 }
 
-static int parse_limit(struct genl_info *info, int id, unsigned int *limit)
+static int parse_limit(struct genl_info *info, int id, unsigned int *limit,
+		       unsigned int max)
 {
 	struct nlattr *attr = info->attrs[id];
 
@@ -1407,10 +1409,9 @@ static int parse_limit(struct genl_info *info, int id, unsigned int *limit)
 		return 0;
 
 	*limit = nla_get_u32(attr);
-	if (*limit > MPTCP_PM_SUBFLOWS_MAX) {
+	if (*limit > max) {
 		NL_SET_ERR_MSG_ATTR_FMT(info->extack, attr,
-					"limit greater than maximum (%u)",
-					MPTCP_PM_SUBFLOWS_MAX);
+					"limit greater than maximum (%u)", max);
 		return -EINVAL;
 	}
 	return 0;
@@ -1419,22 +1420,31 @@ static int parse_limit(struct genl_info *info, int id, unsigned int *limit)
 int mptcp_pm_nl_set_limits_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
-	unsigned int rcv_addrs, subflows;
+	unsigned int rcv_addrs, subflows, retries;
 	int ret;
 
 	spin_lock_bh(&pernet->lock);
 	rcv_addrs = pernet->limit_add_addr_accepted;
-	ret = parse_limit(info, MPTCP_PM_ATTR_RCV_ADD_ADDRS, &rcv_addrs);
+	ret = parse_limit(info, MPTCP_PM_ATTR_RCV_ADD_ADDRS, &rcv_addrs,
+			  MPTCP_PM_SUBFLOWS_MAX);
 	if (ret)
 		goto unlock;
 
 	subflows = pernet->limit_extra_subflows;
-	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOWS, &subflows);
+	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOWS, &subflows,
+			  MPTCP_PM_SUBFLOWS_MAX);
+	if (ret)
+		goto unlock;
+
+	retries = pernet->limit_subflow_retries;
+	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOW_RETRIES, &retries,
+			  MAX_TCP_SYNCNT);
 	if (ret)
 		goto unlock;
 
 	WRITE_ONCE(pernet->limit_add_addr_accepted, rcv_addrs);
 	WRITE_ONCE(pernet->limit_extra_subflows, subflows);
+	WRITE_ONCE(pernet->limit_subflow_retries, retries);
 
 unlock:
 	spin_unlock_bh(&pernet->lock);
@@ -1464,6 +1474,10 @@ int mptcp_pm_nl_get_limits_doit(struct sk_buff *skb, struct genl_info *info)
 			READ_ONCE(pernet->limit_extra_subflows)))
 		goto fail;
 
+	if (nla_put_u32(msg, MPTCP_PM_ATTR_SUBFLOW_RETRIES,
+			READ_ONCE(pernet->limit_subflow_retries)))
+		goto fail;
+
 	genlmsg_end(msg, reply);
 	return genlmsg_reply(msg, info);
 
@@ -1591,6 +1605,125 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
 	return true;
 }
 
+static bool mptcp_pm_retry_path_error(int err)
+{
+	switch (err) {
+	case ETIMEDOUT:
+	case EHOSTUNREACH:
+	case ENETUNREACH:
+	case EHOSTDOWN:
+	case ENETDOWN:
+		return true;
+	}
+
+	return false;
+}
+
+/* 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 pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+	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);
+	bool endp_found;
+	u8 id;
+
+	retry->syn_retries = READ_ONCE(pernet->limit_subflow_retries);
+	if (!retry->syn_retries || !err || sk->sk_state != TCP_ESTABLISHED ||
+	    __mptcp_check_fallback(msk))
+		return false;
+
+	/* mptcp_close_ssk() will not handle it again, e.g. the initial
+	 * subflow, kept in the list once closed
+	 */
+	if (subflow->close_event_done || READ_ONCE(subflow->local_id) < 0)
+		return false;
+
+	/* A single attempt: its SYN retransmissions space out the tries */
+	if (subflow->retry && !established)
+		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;
+
+	/* not if its endpoint has been removed */
+	id = subflow_get_local_id(subflow);
+	if (id) {
+		rcu_read_lock();
+		endp_found = !!__lookup_addr_by_id(pernet, id);
+		rcu_read_unlock();
+		if (!endp_found)
+			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 = id;
+	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, 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;
+	struct sock *ssk;
+	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 ||
+	    __mptcp_subflow_connect(sk, &retry->local, &retry->remote))
+		return;
+
+	/* the new subflow is the last one */
+	subflow = list_last_entry(&msk->conn_list, struct mptcp_subflow_context,
+				  node);
+	ssk = mptcp_subflow_tcp_sock(subflow);
+
+	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
+	WRITE_ONCE(inet_csk(ssk)->icsk_syn_retries, retry->syn_retries);
+	subflow->retry = 1;
+	release_sock(ssk);
+}
+
 /* Called under PM lock */
 void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
 {
@@ -1618,6 +1751,8 @@ static int __net_init pm_nl_init_net(struct net *net)
 
 	/* Cit. 2 subflows ought to be enough for anybody. */
 	pernet->limit_extra_subflows = 2;
+	/* about 20 minutes with the default RTO values */
+	pernet->limit_subflow_retries = 15;
 	pernet->next_id = 1;
 	spin_lock_init(&pernet->lock);
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 60cbd91be632..307a818d4783 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,10 @@ static void __mptcp_close_subflow(struct sock *sk)
 		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
 			continue;
 
+		do_retry = mptcp_pm_subflow_retry_check(msk, subflow, &retry);
 		mptcp_close_ssk(sk, ssk, subflow);
+		if (do_retry)
+			mptcp_pm_subflow_retry(msk, &retry);
 	}
 
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ed2f363bb915..ab916c75e406 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -621,6 +621,7 @@ struct mptcp_subflow_context {
 	u8	reset_seen:1;
 	u8	reset_transient:1;
 	u8	reset_reason:4;
+	u8	retry:1;	    /* re-created after an error */
 	u8	stale_count;	    /* Protected by the msk socket lock */
 	u8	stale;		    /* Protected by the msk socket lock,
 				     * if set the subflow is unable to snd/rcv
@@ -1137,6 +1138,18 @@ void mptcp_pm_subflow_established(struct mptcp_sock *msk);
 bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk);
 void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 				 const struct mptcp_subflow_context *subflow);
+
+struct mptcp_pm_retry {
+	struct mptcp_pm_local	local;
+	struct mptcp_addr_info	remote;
+	u8			syn_retries;
+};
+
+bool mptcp_pm_subflow_retry_check(struct mptcp_sock *msk,
+				  const struct mptcp_subflow_context *subflow,
+				  struct mptcp_pm_retry *retry);
+void mptcp_pm_subflow_retry(struct mptcp_sock *msk,
+			    const struct mptcp_pm_retry *retry);
 void mptcp_pm_add_addr_received(const struct sock *ssk,
 				const struct mptcp_addr_info *addr);
 void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
@@ -1258,6 +1271,11 @@ 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);
+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);
 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);
-- 
2.25.1

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

[Anil Kaushik]

Test the subflow-retries limit, with its default value, and 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 re-created. Let them through again
later than tcp_syn_retries allows, and check the subflow comes back from
the same address. Then do the same with the initial subflow.

Finally, close the connection while a subflow is being re-created, and
check the netlink events. The big files used for the long connection
are removed at the end.

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

Notes:
    v4:
     - default limit, no sysctl or MIB counter
     - more SYN retransmissions than tcp_syn_retries
     - close the connection while a subflow is re-created
     - check the netlink events
     - check the waits and the new subflow port, remove the big files at
       the end (Sashiko)
    v3:
     - check the subflow address, create the files before the background job
    v2:
     - one test, with netfilter

 .../testing/selftests/net/mptcp/mptcp_join.sh | 124 ++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaeda4f..bbeff6739c5a 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2151,6 +2151,69 @@ wait_attempt_fail()
 	return 1
 }
 
+# $1: ns ; $2: counter ; $3: expected minimum value
+wait_counter()
+{
+	local cnt
+
+	for _ in $(seq $((timeout_poll * 10))); do
+		cnt=$(mptcp_lib_get_counter "${1}" "${2}")
+		[ "${cnt:-0}" -ge "${3}" ] && return 0
+		sleep 0.1
+	done
+
+	return 1
+}
+
+# $1: client address ; prints the local address and port of its subflow
+sf_from()
+{
+	ip netns exec $ns2 ss -tnH state established src "${1}" |
+		awk '{ print $3; exit }'
+}
+
+# $1: client address to block
+#
+# The link stays up, but the client packets from this address are dropped:
+# the subflow using it is closed after a few retransmissions, and re-created.
+# The packets go through again later than tcp_syn_retries allows: the new
+# subflow still connects thanks to the subflow-retries limit.
+sf_retry_after_block()
+{
+	local syn_tx ack_rx before after
+
+	syn_tx=$(mptcp_lib_get_counter $ns2 "MPTcpExtMPJoinSynTx")
+	ack_rx=$(mptcp_lib_get_counter $ns1 "MPTcpExtMPJoinAckRx")
+	before=$(sf_from "${1}")
+
+	print_check "subflow from ${1}"
+	if [ -z "${before}" ]; then
+		fail_test "no established subflow from ${1} before the block"
+		return
+	fi
+
+	ip netns exec $ns2 nft add rule ip filter OUTPUT ip saddr "${1}" drop
+	if ! wait_counter $ns2 "MPTcpExtMPJoinSynTx" $((${syn_tx:-0} + 1)); then
+		ip netns exec $ns2 nft flush chain ip filter OUTPUT
+		fail_test "subflow from ${1} not re-created"
+		return
+	fi
+	sleep 5
+	ip netns exec $ns2 nft flush chain ip filter OUTPUT
+
+	if ! wait_counter $ns1 "MPTcpExtMPJoinAckRx" $((${ack_rx:-0} + 1)); then
+		fail_test "re-created subflow from ${1} not established"
+		return
+	fi
+
+	after=$(sf_from "${1}")
+	if [ -z "${after}" ] || [ "${after}" = "${before}" ]; then
+		fail_test "no new subflow from ${1}: ${before} -> ${after}"
+	else
+		print_ok
+	fi
+}
+
 set_userspace_pm()
 {
 	local ns=$1
@@ -2585,6 +2648,67 @@ 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. Then close the connection
+	# while a subflow is being re-created.
+	if reset_with_events "subflow retry after link failure" &&
+	   continue_if mptcp_lib_kallsyms_has "mptcp_pm_kernel_retry$"; then
+		init_nftables
+		init_shapers
+		ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
+		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
+		ip netns exec $ns2 sysctl -q net.mptcp.close_timeout=5
+		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_counter $ns1 "MPTcpExtMPJoinAckRx" 1
+		sf_retry_after_block 10.0.3.2
+		sf_retry_after_block 10.0.1.2
+
+		ip netns exec $ns2 nft add rule ip filter OUTPUT \
+			ip saddr 10.0.3.2 drop
+		wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4
+		local retrying=$?
+		mptcp_lib_kill_group_wait $tests_pid
+
+		# stopped at the latest by the close timeout
+		print_check "no subflow connecting after close"
+		for _ in $(seq $((timeout_poll * 10))); do
+			ip netns exec $ns2 ss -tnH state syn-sent | grep -q . ||
+				break
+			sleep 0.1
+		done
+		if [ "${retrying}" -ne 0 ]; then
+			fail_test "no subflow re-created before the close"
+		elif ip netns exec $ns2 ss -tnH state syn-sent | grep -q .; then
+			fail_test "subflow still connecting after close"
+		else
+			print_ok
+		fi
+		ip netns exec $ns2 nft flush chain ip filter OUTPUT
+		kill_events_pids
+
+		join_syn_tx=4 chk_join_nr 3 3 3
+		chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_ESTABLISHED 3
+		chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED 3 error 3
+
+		# big files: do not keep them, the next tests create their own
+		rm -f "$cinfail" "$sinfail"
+		cinfail=""
+		sinfail=""
+	fi
 }
 
 add_addr_timeout_tests()
-- 
2.25.1

Re: [PATCH mptcp-next v4 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: OOB stack read via unsigned len underflow
--

commit 2c05e206fd91fc4ebb90da18cf7072e457266159
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..b9fd0bdab7486 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
[ ... ]
> @@ -711,6 +712,21 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
>  
>  				bw = do_rnd_write(peerfd, winfo->buf + winfo->off, winfo->len);
[Severity: Medium]
Does this sequence allow an out-of-bounds stack read if the preceding read
call encounters an error?

In copyfd_io_poll(), winfo->len is assigned the result of read(). If read()
fails and returns -1, the signed value is assigned to the unsigned int
winfo->len, wrapping to its maximum value.

The condition checking if winfo->len > 0 then evaluates to true and bypasses
the error handling. This massive length is passed here to do_rnd_write(),
which computes a random write length up to 65535:

do_rnd_write() {
    ...
    do_w = rand() & 0xffff;
    if (do_w == 0 || do_w > len)
        do_w = len;

    bw = write(fd, buf, do_w);
}

Since the winfo->buf size is only 8192 bytes, could write() read uninitialized
data beyond the stack buffer boundary?

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

Re: [PATCH mptcp-next v4 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): Unstable: 1 failed test(s): selftest_mptcp_join ⚠️ 
- 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/38859846683

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


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: [PATCH mptcp-next v4 2/3] mptcp: pm: kernel: retry subflows closed after an error

[Matthieu Baerts]

Hi Anil,

On 16/10/2026 07:50, 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.
> 
> When TCP closes a subflow created by this side after an error, the PM
> now creates a new one with the same addresses. Its SYN retransmissions
> space out the attempts. Their number is set by a new limit,
> subflow-retries: 15 by default, about 20 minutes, and 0 disables it. If
> this new subflow cannot connect, it is not retried again. Once it is
> established, it is handled like any other subflow. Re-creating the
> initial subflow uses one of the additional subflows allowed by the
> limits.
> 
> Subflows refused by the peer, reset by the peer with an MP_RST without
> the transient flag (RFC 8684 section 3.6), aborted locally, or whose
> endpoint has been removed are not retried.
Thank you for the new version. I had a very quick look:

- The patch is quite big, can you split it please? e.g.

  - the modification of parse_limit() before adding the new limit
  - the modification of the netlink specs and the generated files
  - the new limit: parse, store, expose
  - the retry feature

- You can expose the new limit in "struct mptcp_info", instead of
  "reserved".

- in mptcp_pm_kernel_retry_check(), move "struct sock *ssk =
  mptcp_subflow_tcp_sock(subflow);" one line down for the reverse xmas
  tree
  https://docs.kernel.org/process/maintainer-netdev.html

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

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

[Matthieu Baerts]

On 16/10/2026 07:50, Anil Kaushik wrote:
> Test the subflow-retries limit, with its default value, and 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 re-created. Let them through again
> later than tcp_syn_retries allows, and check the subflow comes back from
> the same address. Then do the same with the initial subflow.
> 
> Finally, close the connection while a subflow is being re-created, and
> check the netlink events. The big files used for the long connection
> are removed at the end.
> 
> Assisted-by: LLM
> Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
> ---
> 
> Notes:
>     v4:
>      - default limit, no sysctl or MIB counter
>      - more SYN retransmissions than tcp_syn_retries
>      - close the connection while a subflow is re-created
>      - check the netlink events
>      - check the waits and the new subflow port, remove the big files at
>        the end (Sashiko)
>     v3:
>      - check the subflow address, create the files before the background job
>     v2:
>      - one test, with netfilter
> 
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f4d8afaeda4f..bbeff6739c5a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2151,6 +2151,69 @@ wait_attempt_fail()
>  	return 1
>  }
>  
> +# $1: ns ; $2: counter ; $3: expected minimum value
> +wait_counter()
> +{
> +	local cnt
> +
> +	for _ in $(seq $((timeout_poll * 10))); do
> +		cnt=$(mptcp_lib_get_counter "${1}" "${2}")
> +		[ "${cnt:-0}" -ge "${3}" ] && return 0
> +		sleep 0.1
> +	done
> +
> +	return 1
> +}
> +
> +# $1: client address ; prints the local address and port of its subflow
> +sf_from()
> +{
> +	ip netns exec $ns2 ss -tnH state established src "${1}" |
> +		awk '{ print $3; exit }'
> +}
> +
> +# $1: client address to block
> +#
> +# The link stays up, but the client packets from this address are dropped:
> +# the subflow using it is closed after a few retransmissions, and re-created.
> +# The packets go through again later than tcp_syn_retries allows: the new
> +# subflow still connects thanks to the subflow-retries limit.
> +sf_retry_after_block()
> +{
> +	local syn_tx ack_rx before after
> +
> +	syn_tx=$(mptcp_lib_get_counter $ns2 "MPTcpExtMPJoinSynTx")
> +	ack_rx=$(mptcp_lib_get_counter $ns1 "MPTcpExtMPJoinAckRx")
> +	before=$(sf_from "${1}")
> +
> +	print_check "subflow from ${1}"
> +	if [ -z "${before}" ]; then
> +		fail_test "no established subflow from ${1} before the block"
> +		return
> +	fi
> +
> +	ip netns exec $ns2 nft add rule ip filter OUTPUT ip saddr "${1}" drop
> +	if ! wait_counter $ns2 "MPTcpExtMPJoinSynTx" $((${syn_tx:-0} + 1)); then
> +		ip netns exec $ns2 nft flush chain ip filter OUTPUT
> +		fail_test "subflow from ${1} not re-created"
> +		return
> +	fi
> +	sleep 5

Do you need a long sleep here?

We usually try to have "quick" tests. Here it looks like we do nothing
for 5 seconds, and possibly the sleep is not even needed.

> +	ip netns exec $ns2 nft flush chain ip filter OUTPUT
> +
> +	if ! wait_counter $ns1 "MPTcpExtMPJoinAckRx" $((${ack_rx:-0} + 1)); then
> +		fail_test "re-created subflow from ${1} not established"
> +		return
> +	fi
> +
> +	after=$(sf_from "${1}")
> +	if [ -z "${after}" ] || [ "${after}" = "${before}" ]; then
> +		fail_test "no new subflow from ${1}: ${before} -> ${after}"
> +	else
> +		print_ok
> +	fi
> +}
> +
>  set_userspace_pm()
>  {
>  	local ns=$1
> @@ -2585,6 +2648,67 @@ 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. Then close the connection
> +	# while a subflow is being re-created.
> +	if reset_with_events "subflow retry after link failure" &&
> +	   continue_if mptcp_lib_kallsyms_has "mptcp_pm_kernel_retry$"; then

Probably best to check if the new limit exists: modifying pm_nl_ctl (in
a dedicated commit) and see if it is there. Maybe pm_netlink.sh will
need to be modified as well.

(And iproute2 too if you don't mind, please :) )

> +		init_nftables
> +		init_shapers
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
> +		ip netns exec $ns2 sysctl -q net.mptcp.close_timeout=5
> +		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_counter $ns1 "MPTcpExtMPJoinAckRx" 1
> +		sf_retry_after_block 10.0.3.2
> +		sf_retry_after_block 10.0.1.2
> +
> +		ip netns exec $ns2 nft add rule ip filter OUTPUT \
> +			ip saddr 10.0.3.2 drop
> +		wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4
> +		local retrying=$?
> +		mptcp_lib_kill_group_wait $tests_pid
> +
> +		# stopped at the latest by the close timeout
> +		print_check "no subflow connecting after close"
> +		for _ in $(seq $((timeout_poll * 10))); do
> +			ip netns exec $ns2 ss -tnH state syn-sent | grep -q . ||
> +				break
> +			sleep 0.1
> +		done
> +		if [ "${retrying}" -ne 0 ]; then
> +			fail_test "no subflow re-created before the close"
> +		elif ip netns exec $ns2 ss -tnH state syn-sent | grep -q .; then
> +			fail_test "subflow still connecting after close"
> +		else
> +			print_ok
> +		fi

Best to move this block from print_check in a new helper: we usually
avoid adding "subchecks" in tests directly.

> +		ip netns exec $ns2 nft flush chain ip filter OUTPUT
> +		kill_events_pids
> +
> +		join_syn_tx=4 chk_join_nr 3 3 3
> +		chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_ESTABLISHED 3
> +		chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED 3 error 3
> +
> +		# big files: do not keep them, the next tests create their own
> +		rm -f "$cinfail" "$sinfail"
> +		cinfail=""
> +		sinfail=""
> +	fi
>  }
>  
>  add_addr_timeout_tests()

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

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

[Anil Kaushik]

Hi Matt,

 Thanks for the review!

On Fri, Oct 16, 2026 at 2:31 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>
> On 16/10/2026 07:50, Anil Kaushik wrote:
> > Test the subflow-retries limit, with its default value, and 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 re-created. Let them through again
> > later than tcp_syn_retries allows, and check the subflow comes back from
> > the same address. Then do the same with the initial subflow.
> >
> > Finally, close the connection while a subflow is being re-created, and
> > check the netlink events. The big files used for the long connection
> > are removed at the end.
> >
> > Assisted-by: LLM
> > Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
> > ---
> >
> > Notes:
> >     v4:
> >      - default limit, no sysctl or MIB counter
> >      - more SYN retransmissions than tcp_syn_retries
> >      - close the connection while a subflow is re-created
> >      - check the netlink events
> >      - check the waits and the new subflow port, remove the big files at
> >        the end (Sashiko)
> >     v3:
> >      - check the subflow address, create the files before the background job
> >     v2:
> >      - one test, with netfilter
> >
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 124 ++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> >
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index f4d8afaeda4f..bbeff6739c5a 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -2151,6 +2151,69 @@ wait_attempt_fail()
> >       return 1
> >  }
> >
> > +# $1: ns ; $2: counter ; $3: expected minimum value
> > +wait_counter()
> > +{
> > +     local cnt
> > +
> > +     for _ in $(seq $((timeout_poll * 10))); do
> > +             cnt=$(mptcp_lib_get_counter "${1}" "${2}")
> > +             [ "${cnt:-0}" -ge "${3}" ] && return 0
> > +             sleep 0.1
> > +     done
> > +
> > +     return 1
> > +}
> > +
> > +# $1: client address ; prints the local address and port of its subflow
> > +sf_from()
> > +{
> > +     ip netns exec $ns2 ss -tnH state established src "${1}" |
> > +             awk '{ print $3; exit }'
> > +}
> > +
> > +# $1: client address to block
> > +#
> > +# The link stays up, but the client packets from this address are dropped:
> > +# the subflow using it is closed after a few retransmissions, and re-created.
> > +# The packets go through again later than tcp_syn_retries allows: the new
> > +# subflow still connects thanks to the subflow-retries limit.
> > +sf_retry_after_block()
> > +{
> > +     local syn_tx ack_rx before after
> > +
> > +     syn_tx=$(mptcp_lib_get_counter $ns2 "MPTcpExtMPJoinSynTx")
> > +     ack_rx=$(mptcp_lib_get_counter $ns1 "MPTcpExtMPJoinAckRx")
> > +     before=$(sf_from "${1}")
> > +
> > +     print_check "subflow from ${1}"
> > +     if [ -z "${before}" ]; then
> > +             fail_test "no established subflow from ${1} before the block"
> > +             return
> > +     fi
> > +
> > +     ip netns exec $ns2 nft add rule ip filter OUTPUT ip saddr "${1}" drop
> > +     if ! wait_counter $ns2 "MPTcpExtMPJoinSynTx" $((${syn_tx:-0} + 1)); then
> > +             ip netns exec $ns2 nft flush chain ip filter OUTPUT
> > +             fail_test "subflow from ${1} not re-created"
> > +             return
> > +     fi
> > +     sleep 5
>
> Do you need a long sleep here?
It was there to show the new subflow outlives tcp_syn_retries. In v5,
the test waits for the SYN retransmissions instead, with
tcp_syn_linear_timeouts=0, so no sleep.

>
> We usually try to have "quick" tests. Here it looks like we do nothing
> for 5 seconds, and possibly the sleep is not even needed.
>
> > +     ip netns exec $ns2 nft flush chain ip filter OUTPUT
> > +
> > +     if ! wait_counter $ns1 "MPTcpExtMPJoinAckRx" $((${ack_rx:-0} + 1)); then
> > +             fail_test "re-created subflow from ${1} not established"
> > +             return
> > +     fi
> > +
> > +     after=$(sf_from "${1}")
> > +     if [ -z "${after}" ] || [ "${after}" = "${before}" ]; then
> > +             fail_test "no new subflow from ${1}: ${before} -> ${after}"
> > +     else
> > +             print_ok
> > +     fi
> > +}
> > +
> >  set_userspace_pm()
> >  {
> >       local ns=$1
> > @@ -2585,6 +2648,67 @@ 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. Then close the connection
> > +     # while a subflow is being re-created.
> > +     if reset_with_events "subflow retry after link failure" &&
> > +        continue_if mptcp_lib_kallsyms_has "mptcp_pm_kernel_retry$"; then
>
> Probably best to check if the new limit exists: modifying pm_nl_ctl (in
> a dedicated commit) and see if it is there. Maybe pm_netlink.sh will
> need to be modified as well.
>
> (And iproute2 too if you don't mind, please :) )
Sure, I will send it after this series.
>
> > +             init_nftables
> > +             init_shapers
> > +             ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
> > +             ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
> > +             ip netns exec $ns2 sysctl -q net.mptcp.close_timeout=5
> > +             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_counter $ns1 "MPTcpExtMPJoinAckRx" 1
> > +             sf_retry_after_block 10.0.3.2
> > +             sf_retry_after_block 10.0.1.2
> > +
> > +             ip netns exec $ns2 nft add rule ip filter OUTPUT \
> > +                     ip saddr 10.0.3.2 drop
> > +             wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4
> > +             local retrying=$?
> > +             mptcp_lib_kill_group_wait $tests_pid
> > +
> > +             # stopped at the latest by the close timeout
> > +             print_check "no subflow connecting after close"
> > +             for _ in $(seq $((timeout_poll * 10))); do
> > +                     ip netns exec $ns2 ss -tnH state syn-sent | grep -q . ||
> > +                             break
> > +                     sleep 0.1
> > +             done
> > +             if [ "${retrying}" -ne 0 ]; then
> > +                     fail_test "no subflow re-created before the close"
> > +             elif ip netns exec $ns2 ss -tnH state syn-sent | grep -q .; then
> > +                     fail_test "subflow still connecting after close"
> > +             else
> > +                     print_ok
> > +             fi
>
> Best to move this block from print_check in a new helper: we usually
> avoid adding "subchecks" in tests directly.
>
> > +             ip netns exec $ns2 nft flush chain ip filter OUTPUT
> > +             kill_events_pids
> > +
> > +             join_syn_tx=4 chk_join_nr 3 3 3
> > +             chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_ESTABLISHED 3
> > +             chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED 3 error 3
> > +
> > +             # big files: do not keep them, the next tests create their own
> > +             rm -f "$cinfail" "$sinfail"
> > +             cinfail=""
> > +             sinfail=""
> > +     fi
> >  }
> >
> >  add_addr_timeout_tests()
>
> Cheers,
> Matt
> --
> Sponsored by the NGI0 Core fund.
>

Regards
Anil

Re: [PATCH mptcp-next v4 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] In copyfd_io_poll()
  (tools/testing/selftests/net/mptcp/mptcp_connect.c), the new comment…

--

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

This patch changes copyfd_io_poll() in mptcp_connect so that a write()
failing with EAGAIN no longer ends the transfer. The tool sleeps 1ms and
goes back to poll(). When a poll timeout is set, the number of retries is
capped by the poll_timeout value.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..b9fd0bdab7486 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c

[ ... ]

> @@ -711,6 +712,21 @@ 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 &&
> +					    (poll_timeout < 0 ||
> +					     ++eagain_cnt <= poll_timeout)) {
> +						usleep(1000);
> +						continue;
> +					}

[Severity: Low]
This isn't a bug, but is the "but not forever" part of the comment
accurate in every mode?

main() turns -t 0, or any negative -t value, into poll_timeout = -1:

	case 't':
		poll_timeout = atoi(optarg) * 1000;
		if (poll_timeout <= 0)
			poll_timeout = -1;

When poll_timeout is -1, the condition above never looks at eagain_cnt.
If write() keeps returning EAGAIN while poll() keeps reporting POLLOUT,
copyfd_io_poll() loops forever with a 1ms sleep on each pass. That is
the case the comment says it guards against.

The commit message ("If a poll timeout is set, still fail after as many
retries") and the v4 note ("no limit when there is no poll timeout,
-t 0") suggest this is intended. The in-tree callers all pass
-t ${timeout_poll}, so the selftests never take this branch.

Could the comment say that the limit only applies when a poll timeout
is set?

> +
>  					/* expected reset, continue to read */
>  					if (cfg_rcv_trunc &&
>  					    (errno == ECONNRESET ||

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

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] In __mptcp_close_subflow(), mptcp_close_ssk() first calls
  mptcp_pm_subflow_check_next().
- [Low] The new uAPI attribute is documented as "SYN retransmissions of a
  subflow re-created after an error", and the commit message says "Their…
- [Low] The in-tree pm_nl_ctl tool and pm_netlink.sh were not updated for
  MPTCP_PM_ATTR_SUBFLOW_RETRIES.
- [Low] Subsystem guide violation (netlink.md, 'Validation policy':
  "Declaring the right attribute validation policy is strongly preferred…
- [Low] mptcp_pm_rm_addr_or_subflow() skips subflows already in
  TCP_CLOSE: `if ((1 << inet_sk_state_load(ssk)) & (... | TCPF_CLOSE))…
- [Low] __mptcp_close_subflow() also passes TCP_CLOSE_WAIT subflows (msk
  ESTABLISHED, no fallback) to mptcp_pm_kernel_retry_check().

--

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

With the in-kernel PM, this re-creates a locally initiated subflow with
the same addresses when TCP closes it after an error. A new netlink
limit, subflow-retries (default 15, 0 disables it), sets the SYN retries
of the re-created subflow. Subflows refused by the peer, reset by an
MP_RST without the T flag, aborted locally, or whose endpoint was removed
are not retried.

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

> diff --git a/Documentation/netlink/specs/mptcp_pm.yaml b/Documentation/netlink/specs/mptcp_pm.yaml
> index 39f3facc38e55..1b481908980cc 100644
> --- a/Documentation/netlink/specs/mptcp_pm.yaml
> +++ b/Documentation/netlink/specs/mptcp_pm.yaml
> @@ -196,6 +196,12 @@ attribute-sets:
>          name: addr-remote
>          type: nest
>          nested-attributes: address
> +      -
> +        name: subflow-retries
> +        type: u32
> +        doc: |
> +          SYN retransmissions of a subflow re-created after an error, 0 to
> +          not re-create it.

[Severity: Low]
Is this description exact? The commit message also says "Their number
is set by a new limit".

mptcp_pm_kernel_retry() writes the value unchanged into icsk_syn_retries.
For SYN_SENT, tcp_write_timeout() then adds the linear timeouts:

net/ipv4/tcp_timer.c:tcp_write_timeout() {
    ...
		max_retransmits = retry_until;
		if (sk->sk_state == TCP_SYN_SENT)
			max_retransmits += READ_ONCE(net->ipv4.sysctl_tcp_syn_linear_timeouts);
    ...
}

There is also a time-based limit in retransmits_timed_out(). With the
default tcp_syn_linear_timeouts of 4, a value of 15 can allow up to 19
SYN retransmissions.

The value works like TCP_SYNCNT, not like an exact retransmission count.
This attribute becomes uAPI once released. Could the doc describe it
that way?

[ ... ]

> diff --git a/net/mptcp/mptcp_pm_gen.c b/net/mptcp/mptcp_pm_gen.c
> index c180930a8e0e4..127a1fadaac40 100644
> --- a/net/mptcp/mptcp_pm_gen.c
> +++ b/net/mptcp/mptcp_pm_gen.c
> @@ -44,15 +44,17 @@ const struct nla_policy mptcp_pm_flush_addrs_nl_policy[MPTCP_PM_ENDPOINT_ADDR +
>  };
>  
>  /* MPTCP_PM_CMD_SET_LIMITS - do */
> -const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = {
> +const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOW_RETRIES + 1] = {
>  	[MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, },
>  	[MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, },
> +	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = { .type = NLA_U32, },
>  };

[Severity: Low]
Could this bound go in the policy instead of being checked by hand? The
accepted range 0..127 is only enforced in mptcp_pm_nl_set_limits_doit():

	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOW_RETRIES, &retries,
			  MAX_TCP_SYNCNT);

The netlink guidelines prefer a declared validation policy over
open-coded checks. Adding "checks: max: 127" to subflow-retries in
mptcp_pm.yaml would generate NLA_POLICY_MAX(NLA_U32, 127). The bound
would then also show up in policy introspection.

[ ... ]

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index d3014bf57bf3c..6c081a9c664c0 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c

[ ... ]

> @@ -1464,6 +1474,10 @@ int mptcp_pm_nl_get_limits_doit(struct sk_buff *skb, struct genl_info *info)
>  			READ_ONCE(pernet->limit_extra_subflows)))
>  		goto fail;
>  
> +	if (nla_put_u32(msg, MPTCP_PM_ATTR_SUBFLOW_RETRIES,
> +			READ_ONCE(pernet->limit_subflow_retries)))
> +		goto fail;
> +

[Severity: Low]
Should pm_nl_ctl and pm_netlink.sh be updated for the new attribute?
print_limits() in tools/testing/selftests/net/mptcp/pm_nl_ctl.c still
drops it:

			if (type != MPTCP_PM_ATTR_RCV_ADD_ADDRS &&
			    type != MPTCP_PM_ATTR_SUBFLOWS)
				goto next;

get_set_limits() also only sends the two older limits.

Because of this, the "defaults limits" check in pm_netlink.sh still
passes even if the new default were wrong or missing from the reply:

	check "get_limits" "$(format_limits 0 2)" "defaults limits"

The set path is never tested, including 0 to disable the feature and
the 127 upper bound. The mptcp_join.sh test in the series only covers
the default value implicitly.

[ ... ]

> @@ -1591,6 +1605,125 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)

[ ... ]

> +/* 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 pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
> +	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);

[ ... ]

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

[Severity: Low]
Can these reads see a new sk_err with stale reset bits?

__mptcp_close_subflow() also passes TCP_CLOSE_WAIT subflows to this
check when the msk is ESTABLISHED and there is no fallback. Those
subflows are still live, and only the msk socket lock is held. An RST
carrying an MP_RST can be processed at the same time in softirq:

  mptcp_worker                       softirq, RST + MP_RST (T=0)
  __mptcp_close_subflow()
    ssk_state == TCP_CLOSE_WAIT
                                     tcp_reset()
                                       mptcp_incoming_options()
                                         subflow->reset_seen = 1;
                                         subflow->reset_transient = 0;
                                       tcp_done_with_error()
                                         sk_err = EPIPE
    mptcp_pm_kernel_retry_check()
      err = READ_ONCE(ssk->sk_err);   /* EPIPE */
      subflow->reset_seen             /* may still read 0 */

There is no barrier between the plain reset bit stores and the sk_err
store. The reader also has no acquire or smp_rmb() between its sk_err
read and the bitfield reads.

For a fully established subflow the check would then return true. The
kernel would re-create a subflow that the peer reset with an MP_RST
without the T flag. The do_retry result is also cached and not checked
again after mptcp_close_ssk().

In the TCP_CLOSE branch the reads look ordered, because the
inet_sk_state_load() acquire pairs with the release in tcp_set_state().
Should the retry only be considered for subflows in TCP_CLOSE, or should
these reads be ordered some other way?

[ ... ]

> +	/* not if its endpoint has been removed */
> +	id = subflow_get_local_id(subflow);
> +	if (id) {
> +		rcu_read_lock();
> +		endp_found = !!__lookup_addr_by_id(pernet, id);
> +		rcu_read_unlock();
> +		if (!endp_found)
> +			return false;
> +	}

[Severity: Low]
Does this cover a remote address withdrawn by RM_ADDR, or a removed
local id 0?

mptcp_pm_rm_addr_or_subflow() skips subflows that are already closed:

			if ((1 << inet_sk_state_load(ssk)) &
			    (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
				continue;

Such a subflow keeps close_event_done == false and is collected later by
__mptcp_close_subflow(). That can be delayed while its sk_receive_queue
is not empty. mptcp_worker() also runs mptcp_pm_worker() before
__mptcp_close_subflow().

So a locally initiated subflow can fail with ETIMEDOUT, and an RM_ADDR
for its remote id can be processed before the close is collected. In
that case this function still returns true, since nothing checks
whether the remote address was withdrawn. The new subflow can then hold
an extra_subflows slot for the whole SYN budget of about 20 minutes.

Local id 0 behaves the same way. mptcp_nl_remove_id_zero_address() calls
mptcp_pm_rm_subflow(), which also skips TCP_CLOSE subflows, and then
__mark_subflow_endp_available(msk, 0). Here the lookup is skipped when
id == 0, so the initial subflow can be re-created from an id 0 address
that was explicitly removed.

The commit message says subflows "whose endpoint has been removed are
not retried". Should these two cases be handled as well?

[ ... ]

> +/* 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;
> +	struct sock *ssk;
> +	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);

[Severity: Medium]
Does this take back the slot that mptcp_close_ssk() just freed for the
next endpoint?

In __mptcp_close_subflow(), the close happens first:

__mptcp_close_subflow()
  mptcp_close_ssk()
    mptcp_pm_subflow_check_next()
      __mptcp_pm_close_subflow(msk)      /* extra_subflows-- */
      mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED)
  mptcp_pm_subflow_retry()
    mptcp_pm_kernel_retry()              /* extra_subflows++ */

The check_next step is there so that a "subflow aborted before reaching
the fully_established status" lets the PM try the next one. A
never-established subflow that failed with ETIMEDOUT passes the retry
check. The same failing local/remote pair then gets the slot again.

When the PM worker later runs mptcp_pm_create_subflow_or_signal_addr(),
the limit check fails again, so the next endpoint is not tried:

	while (msk->pm.local_addr_used < endp_subflow_max &&
	       msk->pm.extra_subflows < limit_extra_subflows) {

For example, take limit subflows=1 with two subflow endpoints, where the
first path silently drops MP_JOIN SYNs. Before this patch, the PM moved
to the second endpoint after about 2 minutes of SYN timeouts. With the
default set in pm_nl_init_net():

	pernet->limit_subflow_retries = 15;

it first spends about 20 more minutes on the same failing endpoint.

Is this change in endpoint selection intended? The commit message does
not mention it, and the feature is on by default.

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

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] The post-close check in link_failure_tests() sets
  `net.mptcp.close_timeout=5` and says 'stopped at the latest by the…
- [Low] The new test never checks whether its netfilter rules were
  installed.
- [Low] link_failure_tests() now ends with `cinfail=""` and `sinfail=""`.
- [Low] The commit message says 'The big files used for the long
  connection are removed at the end'.
- [Low] The first sync point with the background job ignores its result:
  `wait_counter $ns1 "MPTcpExtMPJoinAckRx" 1` at line 2676.

--

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

This adds a test to mptcp_join.sh. While a long transfer runs in the
background, nftables drops the client packets from one subflow address,
and the test checks that the subflow is re-created from the same address
once the packets go through again. It does the same for the initial
subflow, then closes the connection while a subflow is being re-created
and checks the netlink events.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f4d8afaeda4f3..bbeff6739c5ac 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2151,6 +2151,69 @@ wait_attempt_fail()

[ ... ]

> +sf_retry_after_block()
> +{
> +	local syn_tx ack_rx before after
> +
> +	syn_tx=$(mptcp_lib_get_counter $ns2 "MPTcpExtMPJoinSynTx")
> +	ack_rx=$(mptcp_lib_get_counter $ns1 "MPTcpExtMPJoinAckRx")
> +	before=$(sf_from "${1}")
> +
> +	print_check "subflow from ${1}"
> +	if [ -z "${before}" ]; then
> +		fail_test "no established subflow from ${1} before the block"
> +		return
> +	fi
> +
> +	ip netns exec $ns2 nft add rule ip filter OUTPUT ip saddr "${1}" drop
> +	if ! wait_counter $ns2 "MPTcpExtMPJoinSynTx" $((${syn_tx:-0} + 1)); then
> +		ip netns exec $ns2 nft flush chain ip filter OUTPUT
> +		fail_test "subflow from ${1} not re-created"
> +		return
> +	fi

[Severity: Low]
What happens if this nft rule can't be installed? The test ignores the
result of init_nftables, of this nft add rule, and of the drop rule added
in the third phase in link_failure_tests().

If the insertion fails, the subflow keeps working and wait_counter times
out. The test then reports "subflow from X not re-created" (or "no
subflow re-created before the close") when it should have been skipped.

The other nft add rule users in mptcp_join.sh check the result and skip.
For example, reset_with_tcp_filter():

	if ! ip netns exec "$ns" nft add rule ip filter "${chain}" \
			ip saddr "${src}" meta l4proto tcp "${target,,}"; then
		mark_as_skipped "unable to set the filter rules"

Could this test use the same check-and-skip pattern? In practice this
only shows up on kernels without IPv4 nftables support, since the
selftest config gets it via CONFIG_NF_TABLES_INET.

[ ... ]

> @@ -2585,6 +2648,67 @@ 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. Then close the connection
> +	# while a subflow is being re-created.
> +	if reset_with_events "subflow retry after link failure" &&
> +	   continue_if mptcp_lib_kallsyms_has "mptcp_pm_kernel_retry$"; then
> +		init_nftables
> +		init_shapers
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_retries2=3
> +		ip netns exec $ns2 sysctl -q net.ipv4.tcp_syn_retries=1
> +		ip netns exec $ns2 sysctl -q net.mptcp.close_timeout=5
> +		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_counter $ns1 "MPTcpExtMPJoinAckRx" 1

[Severity: Low]
Should the return value of this wait_counter be checked?

mktemp only creates empty files in the foreground. The background
run_tests() still fills both 128 MiB files from /dev/urandom with
bs=1024 before mptcp_connect starts:

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

On slow or debug kernels (KASAN, lockdep), could that plus the
connection setup take longer than the ~30 second wait_counter budget?

If so, sf_retry_after_block 10.0.3.2 runs before any join is
established. It fails with the misleading "no established subflow from
10.0.3.2 before the block", and the later checks fail too.

> +		sf_retry_after_block 10.0.3.2
> +		sf_retry_after_block 10.0.1.2
> +
> +		ip netns exec $ns2 nft add rule ip filter OUTPUT \
> +			ip saddr 10.0.3.2 drop
> +		wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4
> +		local retrying=$?
> +		mptcp_lib_kill_group_wait $tests_pid
> +
> +		# stopped at the latest by the close timeout
> +		print_check "no subflow connecting after close"
> +		for _ in $(seq $((timeout_poll * 10))); do
> +			ip netns exec $ns2 ss -tnH state syn-sent | grep -q . ||
> +				break
> +			sleep 0.1
> +		done

[Severity: Medium]
Does the net.mptcp.close_timeout=5 setting above apply at this point?

After mptcp_lib_kill_group_wait, the msk is orphaned. __mptcp_close()
calls sock_orphan(sk), which sets SOCK_DEAD, before it calls
mptcp_start_tout_timer(sk). For a dead msk, mptcp_close_timeout()
ignores the sysctl:

net/mptcp/ctrl.c:mptcp_close_timeout() {
	if (sock_flag(sk, SOCK_DEAD))
		return TCP_TIMEWAIT_LEN;
	return mptcp_get_pernet(sock_net(sk))->close_timeout;
}

So the guaranteed bound is 60 seconds, but this loop polls for only
timeout_poll * 10 * 0.1 seconds (about 30 seconds).

mptcp_pm_kernel_retry() sets icsk_syn_retries to 15 on the SYN-SENT
subflow. The subflow only goes away sooner in one of two ways:

  - a fastclose, when one side has unread data at close
  - the client DATA_FIN, sent through
    mptcp_check_send_data_fin()->mptcp_subflow_shutdown()->tcp_disconnect()

The DATA_FIN goes out only after the whole pending send queue has been
pushed.

If both sides close gracefully and the client send queue doesn't drain
in time, could this report "subflow still connecting after close" on a
correct kernel? In that case the comment "stopped at the latest by the
close timeout" would not hold either.

[ ... ]

> +		# big files: do not keep them, the next tests create their own
> +		rm -f "$cinfail" "$sinfail"

[Severity: Low]
The commit message says:

    The big files used for the long connection are removed at the end.

Doesn't $cinsent also hold a big file at this point? With
test_linkfail=131072, do_transfer() copies the client stream into it:

	tee "$cinsent" < "$cinfail" | \

That can be tens of MB, up to 128 MB. Only a later do_transfer() with
test_linkfail != 0 rewrites $cinsent; in a full run that is
fastclose_tests(), many subtests later. Otherwise cleanup() removes it
at exit.

On a tmpfs /tmp, that memory stays in use through all the subtests in
between. Should $cinsent be removed or truncated here as well?

> +		cinfail=""
> +		sinfail=""

[Severity: Low]
Can clearing these variables leave temp files behind in later subtests?

Before this patch, link_failure_tests() left cinfail and sinfail
pointing at files owned by the parent shell.

Later subtests in userspace_tests() and endpoint_tests() call
run_tests() in a background subshell, for example:

	{ timeout_test=120 test_linkfail=128 speed=5 \

Inside the subshell, run_tests() sees empty variables and creates new
files:

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

These assignments never reach the parent shell, so cleanup() at exit
does not remove those files:

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

In a full run, fastclose_tests() runs in the foreground in between and
sets both variables again. With "mptcp_join.sh -l -u" or
"mptcp_join.sh -l -I", though, each background subtest would leave two
temp files in /tmp.

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