[PATCH mptcp-next v5 0/7] mptcp: pm: in-kernel: retry subflows closed after an error

[PATCH mptcp-next v5 0/7] mptcp: pm: in-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.

Patch 1 fixes a selftest tool issue seen with the new test. Patches 2 to
4 add the new limit, patch 5 the retry, patches 6 and 7 the selftests.

Changes in v5, after Matthieu's review: the retry patch is split, the
limit is also in struct mptcp_info, pm_nl_ctl supports it, a re-created
subflow still connecting is closed with the socket, and the test waits
for SYN retransmissions instead of sleeping. Details in each patch.

v4: https://lore.kernel.org/mptcp/20261016055010.980819-1-anilkaushikwireless@gmail.com/
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 (7):
  selftests: mptcp: connect: retry write() on EAGAIN
  mptcp: pm: in-kernel: pass the maximum value to parse_limit()
  netlink: specs: mptcp: add the subflow-retries attribute
  mptcp: pm: in-kernel: add the subflow-retries limit
  mptcp: pm: in-kernel: retry subflows closed after an error
  selftests: mptcp: pm_nl_ctl: support the subflow-retries limit
  selftests: mptcp: join: validate subflow retry after link failure

 Documentation/netlink/specs/mptcp_pm.yaml     |   9 +
 include/uapi/linux/mptcp.h                    |   2 +-
 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                         | 157 +++++++++++++++++-
 net/mptcp/protocol.c                          |  23 +++
 net/mptcp/protocol.h                          |  19 +++
 net/mptcp/sockopt.c                           |   2 +
 .../selftests/net/mptcp/mptcp_connect.c       |  17 ++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 152 +++++++++++++++++
 .../testing/selftests/net/mptcp/pm_netlink.sh |  42 ++++-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c |  34 +++-
 14 files changed, 469 insertions(+), 23 deletions(-)

-- 
2.25.1

[PATCH mptcp-next v5 1/7] 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:
    v5:
     - no change
    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 v5 2/7] mptcp: pm: in-kernel: pass the maximum value to parse_limit()

[Anil Kaushik]

The subflow-retries limit added in a later patch does not have the same
maximum value as the other limits. Pass it to parse_limit().

No behaviour change.

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

Notes:
    v5:
     - new, split from the retry patch (Matthieu)

 net/mptcp/pm_kernel.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..b662eb914958 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1399,7 +1399,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 +1408,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;
@@ -1424,12 +1424,14 @@ int mptcp_pm_nl_set_limits_doit(struct sk_buff *skb, struct genl_info *info)
 
 	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;
 
-- 
2.25.1

[PATCH mptcp-next v5 3/7] netlink: specs: mptcp: add the subflow-retries attribute

[Anil Kaushik]

It will be used by the in-kernel PM to set the number of SYN
retransmissions of a subflow re-created after an error, with the other
limits: add it to the set-limits and get-limits commands.

The uapi header and the generated files are updated with ynl-regen.

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

Notes:
    v5:
     - new, split from the retry patch (Matthieu)

 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 ++--
 4 files changed, 18 insertions(+), 6 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];
 
-- 
2.25.1

Re: [PATCH mptcp-next v5 3/7] netlink: specs: mptcp: add the subflow-retries attribute

[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] This gap exists only at this one commit; the next patch in the
  series closes it.

--

netlink: specs: mptcp: add the subflow-retries attribute

This adds a subflow-retries u32 attribute to the MPTCP PM netlink spec and
lists it in the set-limits request and in the get-limits request and reply.
The uapi header and the generated policy files are regenerated to match.

> 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, },
>  };

[ ... ]

> @@ -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,
>  	},

[Severity: Low]
With only this patch applied, MPTCP_PM_CMD_SET_LIMITS accepts
MPTCP_PM_ATTR_SUBFLOW_RETRIES. Is the value then silently dropped?

mptcp_pm_nl_set_limits_doit() in net/mptcp/pm_kernel.c still parses only
the two existing limits:

	ret = parse_limit(info, MPTCP_PM_ATTR_RCV_ADD_ADDRS, &rcv_addrs,
			  MPTCP_PM_SUBFLOWS_MAX);
	...
	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOWS, &subflows,
			  MPTCP_PM_SUBFLOWS_MAX);
	...
	WRITE_ONCE(pernet->limit_add_addr_accepted, rcv_addrs);
	WRITE_ONCE(pernet->limit_extra_subflows, subflows);

A request that carries subflow-retries passes validation and returns 0,
but the value is not stored anywhere.

In the same way, mptcp_pm_nl_get_limits_doit() never adds
MPTCP_PM_ATTR_SUBFLOW_RETRIES to its reply, even though the spec lists it
in the get-limits reply.

Only a bisect build at this commit is affected. The next patch in the
series, "mptcp: pm: in-kernel: add the subflow-retries limit", adds the
parse_limit() call, the WRITE_ONCE(pernet->limit_subflow_retries, ...) and
the nla_put_u32() in the reply. At the end of the series the spec and the
implementation match.

Could this spec change be squashed into that patch, so the attribute is
never advertised without a handler?

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

[PATCH mptcp-next v5 4/7] mptcp: pm: in-kernel: add the subflow-retries limit

[Anil Kaushik]

A new limit for the in-kernel PM, set and read with the other limits:
the number of SYN retransmissions of a subflow re-created after an
error, 15 by default, about 20 minutes, and 0 to not re-create it.
MAX_TCP_SYNCNT is the maximum value.

It is also exposed in struct mptcp_info, in place of the reserved field.

The next patch uses it.

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

Notes:
    v5:
     - new, split from the retry patch (Matthieu)
     - the limit is also in struct mptcp_info (Matthieu)

 include/uapi/linux/mptcp.h |  2 +-
 net/mptcp/pm_kernel.c      | 24 +++++++++++++++++++++++-
 net/mptcp/protocol.h       |  1 +
 net/mptcp/sockopt.c        |  2 ++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 72a5d030154e..d7e3f359773d 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -72,7 +72,7 @@ struct mptcp_info {
 	__u8	mptcpi_subflows_total;
 	__u8	mptcpi_endp_laminar_max;
 	__u8	mptcpi_endp_fullmesh_max;
-	__u8	reserved;
+	__u8	mptcpi_limit_subflow_retries;
 	__u32	mptcpi_last_data_sent;
 	__u32	mptcpi_last_data_recv;
 	__u32	mptcpi_last_ack_recv;
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index b662eb914958..f4e23deda140 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);
 };
@@ -96,6 +97,14 @@ u8 mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk)
 }
 EXPORT_SYMBOL_GPL(mptcp_pm_get_limit_extra_subflows);
 
+u8 mptcp_pm_get_limit_subflow_retries(const struct mptcp_sock *msk)
+{
+	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+
+	return READ_ONCE(pernet->limit_subflow_retries);
+}
+EXPORT_SYMBOL_GPL(mptcp_pm_get_limit_subflow_retries);
+
 static bool has_subflow_daddr(const struct mptcp_sock *msk,
 			      const struct mptcp_addr_info *daddr)
 {
@@ -1419,7 +1428,7 @@ 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);
@@ -1435,8 +1444,15 @@ int mptcp_pm_nl_set_limits_doit(struct sk_buff *skb, struct genl_info *info)
 	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);
@@ -1466,6 +1482,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);
 
@@ -1620,6 +1640,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.h b/net/mptcp/protocol.h
index ed2f363bb915..6d571328aca9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1264,6 +1264,7 @@ u8 mptcp_pm_get_endp_laminar_max(const struct mptcp_sock *msk);
 u8 mptcp_pm_get_endp_fullmesh_max(const struct mptcp_sock *msk);
 u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk);
 u8 mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk);
+u8 mptcp_pm_get_limit_subflow_retries(const struct mptcp_sock *msk);
 
 /* called under PM lock */
 static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 922f6ae5c80c..d3c4abf59ec5 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1075,6 +1075,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 			mptcp_pm_get_endp_laminar_max(msk);
 		info->mptcpi_endp_fullmesh_max =
 			mptcp_pm_get_endp_fullmesh_max(msk);
+		info->mptcpi_limit_subflow_retries =
+			mptcp_pm_get_limit_subflow_retries(msk);
 	}
 
 	if (__mptcp_check_fallback(msk))
-- 
2.25.1

Re: [PATCH mptcp-next v5 4/7] mptcp: pm: in-kernel: add the subflow-retries limit

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] The new MPTCP_PM_ATTR_SUBFLOW_RETRIES netlink attribute is
  validated only by open-coded C, and its range is not declared in the…

--

mptcp: pm: in-kernel: add the subflow-retries limit

This adds a new in-kernel PM limit, subflow-retries, which is set and read
with the other limits through MPTCP_PM_CMD_SET_LIMITS and
MPTCP_PM_CMD_GET_LIMITS. It is the number of SYN retransmissions for a
subflow re-created after an error. The default is 15 and the maximum is
MAX_TCP_SYNCNT. The value is also exposed in struct mptcp_info, replacing
the reserved field.

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index b662eb914958c..f4e23deda1403 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c

[ ... ]

> @@ -1435,8 +1444,15 @@ int mptcp_pm_nl_set_limits_doit(struct sk_buff *skb, struct genl_info *info)
>  	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;

[Severity: Low]
Could this range be declared in the netlink policy, instead of being
checked only here?

In mptcp_pm_nl_set_limits_doit(), this parse_limit() call is the only
place that enforces the 0..127 (MAX_TCP_SYNCNT) range. The spec entry
added earlier in the series, in Documentation/netlink/specs/mptcp_pm.yaml,
is a plain u32 with no max check:

        name: subflow-retries
        type: u32
        doc: |
          SYN retransmissions of a subflow re-created after an error, 0 to
          not re-create it.

So the generated mptcp_pm_set_limits_nl_policy in net/mptcp/mptcp_pm_gen.c
becomes:

	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = { .type = NLA_U32, },

Policy introspection (genl ctrl getpolicy) and spec-driven clients such
as YNL would then report the full u32 range as valid. The kernel still
returns -EINVAL for any value above 127.

Would adding "checks: max: 127" to the spec entry help here? It would
generate NLA_POLICY_MAX(NLA_U32, 127), so the spec, the policy dump and
the kernel would all agree. The netlink guidelines prefer a declared
NLA_POLICY_* over open-coded validation.

The existing subflows and rcv-add-addrs limits have the same gap. This
attribute is new, though, and its maximum is different from theirs
(MAX_TCP_SYNCNT vs MPTCP_PM_SUBFLOWS_MAX).

The spec doc also doesn't give the maximum or the default of 15. Could
those be added too?

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

[PATCH mptcp-next v5 5/7] mptcp: pm: in-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,
set by the subflow-retries limit, space out the attempts. If this new
subflow cannot connect, it is not retried again, and it is closed if
the MPTCP socket is closed in the meantime. 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:
    v5 (Matthieu):
     - split in 4 patches
     - reverse xmas tree in mptcp_pm_kernel_retry_check()
     - "in-kernel" in the subject, like the other in-kernel PM patches
     - close the re-created subflows still connecting when the socket is
       closed (Matthieu: closing the socket during a retry)
    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

 net/mptcp/pm.c        |  20 +++++++
 net/mptcp/pm_kernel.c | 119 ++++++++++++++++++++++++++++++++++++++++++
 net/mptcp/protocol.c  |  23 ++++++++
 net/mptcp/protocol.h  |  18 +++++++
 4 files changed, 180 insertions(+)

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 f4e23deda140..f49224377ade 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1613,6 +1613,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);
+	bool established = READ_ONCE(subflow->fully_established);
+	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+	struct sock *sk = (struct sock *)msk;
+	int err = READ_ONCE(ssk->sk_err);
+	bool endp_found;
+	u8 id;
+
+	retry->syn_retries = mptcp_pm_get_limit_subflow_retries(msk);
+	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)
 {
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 60cbd91be632..354b80156bce 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);
 	}
 
 }
@@ -3494,6 +3499,22 @@ static void mptcp_check_listen_stop(struct sock *sk)
 	release_sock(ssk);
 }
 
+/* Re-created subflows still connecting are useless once the msk is closed,
+ * and their SYN retransmissions could keep them around for a long time.
+ */
+static void mptcp_close_retry_subflows(struct sock *sk)
+{
+	struct mptcp_subflow_context *subflow, *tmp;
+	struct mptcp_sock *msk = mptcp_sk(sk);
+
+	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+		if (subflow->retry && inet_sk_state_load(ssk) == TCP_SYN_SENT)
+			mptcp_close_ssk(sk, ssk, subflow);
+	}
+}
+
 bool __mptcp_close(struct sock *sk, long timeout)
 {
 	struct mptcp_subflow_context *subflow;
@@ -3520,6 +3541,8 @@ bool __mptcp_close(struct sock *sk, long timeout)
 		__mptcp_wr_shutdown(sk);
 	}
 
+	mptcp_close_retry_subflows(sk);
+
 	sk_stream_wait_close(sk, timeout);
 
 cleanup:
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6d571328aca9..b82de84d7543 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

Re: [PATCH mptcp-next v5 5/7] mptcp: pm: in-kernel: retry subflows closed after an error

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In mptcp_pm_kernel_retry_check() (net/mptcp/pm_kernel.c), the
  endpoint-removed check ("/* not if its endpoint has been removed */")…
- [Medium] mptcp_pm_kernel_retry_check() treats READ_ONCE(ssk->sk_err) at
  close time as the reason the subflow closed, but that value can be…
- [Medium] A subflow can be re-created towards a remote address the peer
  has just withdrawn with RM_ADDR.
- [Low] mptcp_close_retry_subflows() (net/mptcp/protocol.c), called from
  __mptcp_close(), tears down re-created subflows still in SYN_SENT…

--

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

With this patch, the in-kernel PM re-creates a subflow initiated by this
side, using the same addresses, when TCP closes it after an error. The SYN
retransmissions of the new subflow are set by the subflow-retries limit,
and re-created subflows still in SYN_SENT are closed when the MPTCP socket
is closed. Subflows reset by the peer without the transient flag, aborted
locally, or whose endpoint was removed are not supposed to be retried.

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

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index f4e23deda1403..f49224377aded 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1613,6 +1613,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);
> +	bool established = READ_ONCE(subflow->fully_established);
> +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +	struct sock *sk = (struct sock *)msk;
> +	int err = READ_ONCE(ssk->sk_err);
> +	bool endp_found;
> +	u8 id;
> +
> +	retry->syn_retries = mptcp_pm_get_limit_subflow_retries(msk);
> +	if (!retry->syn_retries || !err || sk->sk_state != TCP_ESTABLISHED ||
> +	    __mptcp_check_fallback(msk))
> +		return false;

[Severity: Medium]
Can ssk->sk_err here be left over from an earlier error that did not close
the subflow, rather than being the reason it closed?

MPTCP syncs IP_RECVERR / IPV6_RECVERR to the subflows. When it is set,
tcp_v4_err() on an established subflow can set sk_err (for example to
EHOSTUNREACH) and the subflow stays up. For an established, non-fallback
msk, __mptcp_subflow_error_report() returns without consuming the error:

net/mptcp/protocol.c:__mptcp_subflow_error_report() {
    ...
	if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
		goto out;

	err = sock_error(ssk);
    ...
}

If the subflow is later closed without a new error, the !err filter does
not catch it and the subflow is retried. Two such closes:

- a graceful FIN from the peer, handled by the TCP_CLOSE_WAIT branch of
  __mptcp_close_subflow()
- a local mptcp_subflow_reset(), e.g. from mptcp_mp_fail_no_response(),
  which ends with the following and never touches sk_err:

	mptcp_send_active_reset_reason(ssk);
	tcp_done(ssk);

Would these subflows then be re-created? That would go against the commit
message, which says subflows "aborted locally" are not retried.

> +
> +	/* mptcp_close_ssk() will not handle it again, e.g. the initial
> +	 * subflow, kept in the list once closed
> +	 */

[ ... ]

> +	/* 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: Medium]
Is the endpoint-removed check skipped for the initial subflow?

The initial subflow always has local id 0, and so does any re-created copy
of it, since it is created with local.addr.id = 0. So the
__lookup_addr_by_id() lookup never runs for it.

The endpoint for that address is tracked in msk->mpc_endpoint_id, which is
reset when the endpoint is removed and is never read here:

net/mptcp/pm_kernel.c:mptcp_nl_remove_subflow_and_signal_addr() {
    ...
		if (msk->mpc_endpoint_id == entry->addr.id)
			msk->mpc_endpoint_id = 0;
    ...
}

When the endpoint is deleted, mptcp_pm_rm_addr_or_subflow() skips
subflows that are already in TCP_CLOSE:

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

Take a client whose initial subflow failed with ETIMEDOUT and is waiting
for MPTCP_WORK_CLOSE_SUBFLOW. That subflow is left to
__mptcp_close_subflow(). With a non-zero id, the lookup would refuse the
retry. With id 0, this function returns true.

Could this re-create a subflow on the address of an endpoint the admin
just removed? The commit message says subflows "whose endpoint has been
removed are not retried". Also, deleting the endpoint later could not
remove the new subflow, because the endpoint is already gone.

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

[Severity: Medium]
What happens if the peer has just withdrawn this remote address with
RM_ADDR?

In mptcp_worker(), the PM work runs before the subflow cleanup:

	mptcp_pm_worker(msk);
	...
	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
		__mptcp_close_subflow(sk);

mptcp_pm_rm_addr_or_subflow() skips TCP_CLOSE subflows before it matches
the removed remote id (the same TCPF_CLOSE check quoted above). A subflow
that already closed with a retryable error is therefore neither closed
nor marked by the RM_ADDR handling.

The only remote check here is remote_deny_join_id0, and it only applies
to remote id 0. Could mptcp_pm_kernel_retry() then open a new MP_JOIN to
an address the peer has withdrawn?

One possible trigger: the peer removes an endpoint, closes the matching
subflow (e.g. with an RST when data is unread), and sends RM_ADDR on
another subflow. Both events arrive before the worker runs.

> +
> +	mptcp_local_address((struct sock_common *)ssk, &retry->local.addr);

[ ... ]

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

[ ... ]

> @@ -3494,6 +3499,22 @@ static void mptcp_check_listen_stop(struct sock *sk)
>  	release_sock(ssk);
>  }
>  
> +/* Re-created subflows still connecting are useless once the msk is closed,
> + * and their SYN retransmissions could keep them around for a long time.
> + */
> +static void mptcp_close_retry_subflows(struct sock *sk)
> +{
> +	struct mptcp_subflow_context *subflow, *tmp;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +
> +	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		if (subflow->retry && inet_sk_state_load(ssk) == TCP_SYN_SENT)
> +			mptcp_close_ssk(sk, ssk, subflow);

[Severity: Low]
mptcp_close_ssk() always calls __mptcp_close_ssk() with MPTCP_CF_PUSH. Is
that intended for subflows still in SYN_SENT while the msk is closing?

In __mptcp_close_ssk(), this re-injects the whole rtx queue and later
calls __mptcp_push_pending(sk, 0), even though a SYN_SENT subflow never
carried data:

	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);

In the fastclose branch of __mptcp_close(), mptcp_do_fastclose() has
already sent the RSTs. The other subflows are still TCP_ESTABLISHED
locally, though, so data segments could go out on them just before
__mptcp_destroy_sock().

The timeout-driven fastclose path in mptcp_worker() avoids this by
passing 0:

		mptcp_do_fastclose(sk);
		mptcp_for_each_subflow_safe(msk, subflow, tmp)
			__mptcp_close_ssk(sk, subflow->tcp_sock, subflow, 0);

On a graceful close, the unacked rtx queue would also be re-injected each
time a pending retry subflow is torn down.

Would it be better to close these subflows without MPTCP_CF_PUSH?

> +	}
> +}

[ ... ]

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

[PATCH mptcp-next v5 6/7] selftests: mptcp: pm_nl_ctl: support the subflow-retries limit

[Anil Kaushik]

'pm_nl_ctl limits' can now set the subflow-retries limit with a third
value, and shows it when the kernel supports it.

pm_netlink.sh checks its default value, that a value above the maximum
is refused, and that it can be set. This is only done with pm_nl_ctl
for the moment: iproute2 does not support this limit yet.

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

Notes:
    v5:
     - new (Matthieu)

 .../testing/selftests/net/mptcp/pm_netlink.sh | 42 ++++++++++++++++++-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 34 +++++++++++----
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 21bfe1311f11..ddf2899e4830 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -46,15 +46,20 @@ trap cleanup EXIT
 
 mptcp_lib_ns_init ns1
 
+# $3: subflow retries, only shown by pm_nl_ctl, if supported
 format_limits() {
 	local accept="${1}"
 	local subflows="${2}"
+	local retries="${3}"
 
 	if mptcp_lib_is_ip_mptcp; then
 		# with a space at the end
 		printf "add_addr_accepted %d subflows %d \n" "${accept}" "${subflows}"
 	else
 		printf "accept %d\nsubflows %d\n" "${accept}" "${subflows}"
+		if [ -n "${retries}" ]; then
+			printf "subflow_retries %d\n" "${retries}"
+		fi
 	fi
 }
 
@@ -75,6 +80,22 @@ get_limits_nb() {
 	fi
 }
 
+# The subflow retries limit is only set and shown with pm_nl_ctl for now.
+# Prints its default value if it is supported.
+get_default_retries() {
+	if mptcp_lib_is_ip_mptcp; then
+		return
+	fi
+
+	if ip netns exec "${ns1}" ./pm_nl_ctl limits |
+	   grep -q "^subflow_retries"; then
+		echo 15
+	elif mptcp_lib_expect_all_features; then
+		# missing feature: the defaults limits check will fail
+		echo 15
+	fi
+}
+
 format_endpoints() {
 	mptcp_lib_pm_nl_format_endpoints "${@}"
 }
@@ -154,8 +175,10 @@ mptcp_lib_subtests_last_ts_reset
 check "show_endpoints" "" "defaults addr list"
 
 default_limits="$(get_limits)"
+default_retries="$(get_default_retries)"
 if mptcp_lib_expect_all_features; then
-	check "get_limits" "$(format_limits 0 2)" "defaults limits"
+	check "get_limits" "$(format_limits 0 2 "${default_retries}")" \
+		"defaults limits"
 fi
 
 add_endpoint 10.0.1.1
@@ -224,7 +247,22 @@ check "get_limits" "${default_limits}" "subflows above hard limit"
 
 set_limits ${max_limits} ${max_limits}
 flush_endpoint  ## to make sure it doesn't affect the limits
-check "get_limits" "$(format_limits ${max_limits} ${max_limits})" "set limits"
+max_limits_out="$(format_limits ${max_limits} ${max_limits} \
+		 "${default_retries}")"
+check "get_limits" "${max_limits_out}" "set limits"
+
+if [ -n "${default_retries}" ]; then
+	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} \
+		128 2>/dev/null
+	check "get_limits" "${max_limits_out}" \
+		"subflow retries above hard limit"
+
+	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} 0
+	check "get_limits" "$(format_limits ${max_limits} ${max_limits} 0)" \
+		"set subflow retries"
+	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} \
+		"${default_retries}"
+fi
 
 flush_endpoint
 add_endpoint 10.0.1.1
diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 78180da1efcc..41467b997018 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -39,7 +39,7 @@ static void syntax(char *argv[])
 	fprintf(stderr, "\tset [<ip>] [id <nr>] flags [no]backup|[no]fullmesh [port <nr>] [token <token>] [rip <ip>] [rport <port>]\n");
 	fprintf(stderr, "\tflush\n");
 	fprintf(stderr, "\tdump\n");
-	fprintf(stderr, "\tlimits [<rcv addr max> <subflow max>]\n");
+	fprintf(stderr, "\tlimits [<rcv addr max> <subflow max> [<retry>]]\n");
 	fprintf(stderr, "\tevents\n");
 	fprintf(stderr, "\tlisten <local-ip> <local-port>\n");
 	exit(0);
@@ -1217,6 +1217,7 @@ int flush_addrs(int fd, int pm_family, int argc, char *argv[])
 static void print_limits(struct nlmsghdr *nh, int pm_family, int total_len)
 {
 	struct rtattr *attrs;
+	const char *name;
 	uint32_t max;
 
 	for (; NLMSG_OK(nh, total_len); nh = NLMSG_NEXT(nh, total_len)) {
@@ -1235,13 +1236,22 @@ static void print_limits(struct nlmsghdr *nh, int pm_family, int total_len)
 		while (RTA_OK(attrs, len)) {
 			int type = attrs->rta_type;
 
-			if (type != MPTCP_PM_ATTR_RCV_ADD_ADDRS &&
-			    type != MPTCP_PM_ATTR_SUBFLOWS)
+			switch (type) {
+			case MPTCP_PM_ATTR_RCV_ADD_ADDRS:
+				name = "accept";
+				break;
+			case MPTCP_PM_ATTR_SUBFLOWS:
+				name = "subflows";
+				break;
+			case MPTCP_PM_ATTR_SUBFLOW_RETRIES:
+				name = "subflow_retries";
+				break;
+			default:
 				goto next;
+			}
 
 			memcpy(&max, RTA_DATA(attrs), 4);
-			printf("%s %u\n", type == MPTCP_PM_ATTR_SUBFLOWS ?
-					  "subflows" : "accept", max);
+			printf("%s %u\n", name, max);
 
 next:
 			attrs = RTA_NEXT(attrs, len);
@@ -1254,15 +1264,17 @@ int get_set_limits(int fd, int pm_family, int argc, char *argv[])
 	char data[NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
 		  NLMSG_ALIGN(sizeof(struct genlmsghdr)) +
 		  1024];
-	uint32_t rcv_addr = 0, subflows = 0;
+	uint32_t rcv_addr = 0, subflows = 0, retries = 0;
 	int cmd, len = sizeof(data);
 	struct nlmsghdr *nh;
 	int off = 0;
 
 	/* limit */
-	if (argc == 4) {
+	if (argc == 4 || argc == 5) {
 		rcv_addr = atoi(argv[2]);
 		subflows = atoi(argv[3]);
+		if (argc == 5)
+			retries = atoi(argv[4]);
 		cmd = MPTCP_PM_CMD_SET_LIMITS;
 	} else {
 		cmd = MPTCP_PM_CMD_GET_LIMITS;
@@ -1287,6 +1299,14 @@ int get_set_limits(int fd, int pm_family, int argc, char *argv[])
 		memcpy(RTA_DATA(rta), &subflows, 4);
 		off += NLMSG_ALIGN(rta->rta_len);
 
+		if (argc == 5) {
+			rta = (void *)(data + off);
+			rta->rta_type = MPTCP_PM_ATTR_SUBFLOW_RETRIES;
+			rta->rta_len = RTA_LENGTH(4);
+			memcpy(RTA_DATA(rta), &retries, 4);
+			off += NLMSG_ALIGN(rta->rta_len);
+		}
+
 		/* do not expect a reply */
 		len = 0;
 	}
-- 
2.25.1

Re: [PATCH mptcp-next v5 6/7] selftests: mptcp: pm_nl_ctl: support the subflow-retries limit

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] The new subflow-retries subtests in
  tools/testing/selftests/net/mptcp/pm_netlink.sh test only one side of…

--

selftests: mptcp: pm_nl_ctl: support the subflow-retries limit

This commit lets 'pm_nl_ctl limits' take an optional third value that
sets the subflow-retries limit. It also prints that limit from the
GET_LIMITS reply when the kernel reports it. pm_netlink.sh gets new
subtests for the default value, for an out-of-range value being refused,
and for setting a new value.

> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> index 21bfe1311f118..ddf2899e4830b 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh

[ ... ]

> @@ -224,7 +247,22 @@ check "get_limits" "${default_limits}" "subflows above hard limit"
>
>  set_limits ${max_limits} ${max_limits}
>  flush_endpoint  ## to make sure it doesn't affect the limits
> -check "get_limits" "$(format_limits ${max_limits} ${max_limits})" "set limits"
> +max_limits_out="$(format_limits ${max_limits} ${max_limits} \
> +		 "${default_retries}")"
> +check "get_limits" "${max_limits_out}" "set limits"
> +
> +if [ -n "${default_retries}" ]; then
> +	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} \
> +		128 2>/dev/null
> +	check "get_limits" "${max_limits_out}" \
> +		"subflow retries above hard limit"
> +
> +	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} 0
> +	check "get_limits" "$(format_limits ${max_limits} ${max_limits} 0)" \
> +		"set subflow retries"

[Severity: Low]
This isn't a bug, but should these subtests also check that the largest
valid value is accepted?

The values sent here are 128 (refused), 0, and then the default 15 when
the limit is restored. On the kernel side, mptcp_pm_nl_set_limits_doit()
in net/mptcp/pm_kernel.c does:

	retries = pernet->limit_subflow_retries;
	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOW_RETRIES, &retries,
			  MAX_TCP_SYNCNT);

parse_limit() only refuses the value when *limit > max, so 127 should be
accepted. Nothing in the test sends 127.

If the check broke in a way that still refuses 128 and accepts 0, these
subtests would still pass. Two examples are an off-by-one (>= instead
of >), which would refuse 127, and a smaller cap such as 15.

The earlier tests for the rcv-add-addrs and subflows limits cover both
sides of the boundary: max_limits + 1 is refused, and max_limits is
accepted in "set limits".

Could a step be added that sets 127 and checks it with format_limits
before the default is restored?

A smaller point: the out-of-range request sends rcv_addr and subflows
equal to the values already in place (max_limits). That means the test
can't show that the whole request is refused together. The current
kernel stores all three values only after every parse_limit() call
succeeds, so this is a gap in the test, not a bug.

> +	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} \
> +		"${default_retries}"
> +fi
>
>  flush_endpoint
>  add_endpoint 10.0.1.1

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

[PATCH mptcp-next v5 7/7] 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
once the SYN of the new subflow has been retransmitted more times 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.

The test is skipped if pm_nl_ctl does not show the new limit.

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

Notes:
    v5 (Matthieu):
     - skipped if pm_nl_ctl does not show the new limit
     - wait for the SYN retransmissions instead of a sleep, with
       tcp_syn_linear_timeouts=0
     - helper for the check after the close
     - slower links with short queues, so the transfer lasts the whole
       test on any machine, longer timeout (killed at the end)
    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 | 152 ++++++++++++++++++
 1 file changed, 152 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaeda4f..521f95d3027f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2151,6 +2151,102 @@ 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 }'
+}
+
+has_subflow_retries()
+{
+	ip netns exec $ns2 ./pm_nl_ctl limits | grep -q "^subflow_retries"
+}
+
+# $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 once the SYN has been retransmitted more times
+# than tcp_syn_retries allows: the new subflow still connects thanks to the
+# subflow-retries limit.
+sf_retry_after_block()
+{
+	local syn_tx syn_rtx ack_rx before after
+
+	syn_tx=$(mptcp_lib_get_counter $ns2 "MPTcpExtMPJoinSynTx")
+	syn_rtx=$(mptcp_lib_get_counter $ns2 "TcpExtTCPSynRetrans")
+	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
+	# tcp_syn_retries=1: one more retransmission than allowed
+	if ! wait_counter $ns2 "TcpExtTCPSynRetrans" $((syn_rtx + 2)); then
+		ip netns exec $ns2 nft flush chain ip filter OUTPUT
+		fail_test "SYN from ${1} not retransmitted enough"
+		return
+	fi
+	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
+}
+
+# $1: result of the wait for the subflow being re-created before the close
+chk_no_sf_connecting()
+{
+	print_check "no subflow connecting after close"
+	if [ "${1}" -ne 0 ]; then
+		fail_test "no subflow re-created before the close"
+		return
+	fi
+
+	# closed with the MPTCP socket
+	for _ in $(seq $((timeout_poll * 10))); do
+		if ! ip netns exec $ns2 ss -tnH state syn-sent | grep -q .; then
+			print_ok
+			return
+		fi
+		sleep 0.1
+	done
+
+	fail_test "subflow still connecting after close"
+}
+
 set_userspace_pm()
 {
 	local ns=$1
@@ -2585,6 +2681,62 @@ 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 has_subflow_retries; then
+		init_nftables
+		# slow links, short queues: the transfer lasts the whole test on
+		# any machine, and the blocked subflow always has data in flight
+		local i
+		for i in 1 3; do
+			tc -n $ns1 qdisc add dev ns1eth$i root netem \
+				rate 4mbit limit 30 delay 1ms
+			tc -n $ns2 qdisc add dev ns2eth$i root netem \
+				rate 4mbit limit 30 delay 1ms
+		done
+		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.ipv4.tcp_syn_linear_timeouts=0
+		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
+		# killed at the end: the timeout only stops a stuck test
+		{ timeout_test=600 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
+		chk_no_sf_connecting ${retrying}
+		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 v5 7/7] selftests: mptcp: join: validate subflow retry after link failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/mptcp: sf_from() extracts Send-Q instead of local address and port
- [Medium] selftests/mptcp: hand-rolled busy-wait polling loops instead of shared helpers
- [Medium] selftests: mptcp: background file generation race causes flaky timeout
--

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

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

Test the subflow-retries limit, with its default value, and a long
connection in the background.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f4d8afaeda4f3..521f95d3027fa 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2151,6 +2151,102 @@ 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
> +}

[Severity: Medium]
Does this hand-roll a busy-wait polling loop instead of using the shared
helpers?

The networking selftests library (tools/testing/selftests/net/lib.sh) already
provides busywait_for_counter specifically to handle this pattern robustly.

> +
> +# $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 }'
> +}

[Severity: Medium]
Does this extract the Send-Q column instead of the Local Address:Port column?

The command ss -tnH state established outputs columns: State, Recv-Q, Send-Q,
Local Address:Port, Peer Address:Port. So the 3rd column is Send-Q, rather than
Local Address:Port.

Because the previously blocked subflow has queued, dropped packets, its Send-Q
is non-zero. The newly created subflow has an empty Send-Q (0). The test logic
later evaluates [ "${after}" = "${before}" ] as [ "0" = "<non-zero>" ],
which evaluates to false, causing the test to pass by coincidence without ever
verifying that the new subflow originated from a different port.

[ ... ]

> +
> +# $1: result of the wait for the subflow being re-created before the close
> +chk_no_sf_connecting()
> +{
> +	print_check "no subflow connecting after close"
> +	if [ "${1}" -ne 0 ]; then
> +		fail_test "no subflow re-created before the close"
> +		return
> +	fi
> +
> +	# closed with the MPTCP socket
> +	for _ in $(seq $((timeout_poll * 10))); do
> +		if ! ip netns exec $ns2 ss -tnH state syn-sent | grep -q .; then
> +			print_ok
> +			return
> +		fi
> +		sleep 0.1
> +	done
> +
> +	fail_test "subflow still connecting after close"
> +}

[Severity: Medium]
Does this hand-roll a busy-wait polling loop instead of using the shared
helpers?

The networking selftests library (tools/testing/selftests/net/lib.sh) already
provides busywait to handle this pattern robustly.

[ ... ]

> @@ -2585,6 +2681,62 @@ 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 has_subflow_retries; then
> +		init_nftables
> +		# slow links, short queues: the transfer lasts the whole test on
> +		# any machine, and the blocked subflow always has data in flight
> +		local i
> +		for i in 1 3; do
> +			tc -n $ns1 qdisc add dev ns1eth$i root netem \
> +				rate 4mbit limit 30 delay 1ms
> +			tc -n $ns2 qdisc add dev ns2eth$i root netem \
> +				rate 4mbit limit 30 delay 1ms
> +		done
> +		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.ipv4.tcp_syn_linear_timeouts=0
> +		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
> +		# killed at the end: the timeout only stops a stuck test
> +		{ timeout_test=600 test_linkfail=131072 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_counter $ns1 "MPTcpExtMPJoinAckRx" 1

[Severity: Medium]
Could this foreground polling loop time out before the background file
generation completes?

The test case sets test_linkfail=131072 and launches run_tests in the
background. Inside run_tests, it sequentially generates 128MB of random data
per file via dd, blocking the network transfer from starting until both files
are fully generated.

Meanwhile, the foreground script immediately calls wait_counter, which polls
for the MPTcpExtMPJoinAckRx counter to increment with a 30-second timeout.
If the dd commands take longer than 30 seconds on a slow test runner, the
foreground script will time out and fail the test before the actual connection
has even started.

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Setup for the new "subflow retry after link failure" test in
  link_failure_tests() depends on timing, and its first gate is never…
- [Low] chk_no_sf_connecting() cannot reliably show that the new
  mptcp_close_retry_subflows() helper works.

--

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

This adds a test to mptcp_join.sh for the subflow-retries limit. While a
long transfer runs in the background, nftables drops the client packets
sent from a subflow address. Once the re-created subflow's SYN has been
retransmitted more times than tcp_syn_retries allows, the packets are let
through and the test checks that the subflow comes back. This is done for
the additional subflow and then the initial one. Finally the connection is
closed while a subflow is being re-created, and the netlink events are
checked.

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

[ ... ]

> @@ -2585,6 +2681,62 @@ 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 has_subflow_retries; then
> +		init_nftables

[ ... ]

> +		# created here, not in the background job, for cleanup()
> +		if [ -z "$cinfail" ]; then
> +			cinfail=$(mktemp)
> +		fi
> +		if [ -z "$sinfail" ]; then
> +			sinfail=$(mktemp)
> +		fi
> +		# killed at the end: the timeout only stops a stuck test
> +		{ timeout_test=600 test_linkfail=131072 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_counter $ns1 "MPTcpExtMPJoinAckRx" 1

[Severity: Medium]

Can the setup for this test take longer than this first wait_counter()
allows?

With test_linkfail=131072, run_tests() always regenerates both files,
because test_linkfail > 2. Only after that does it call do_transfer():

tools/testing/selftests/net/mptcp/mptcp_join.sh:run_tests() {
    ...
	if [ "$test_linkfail" -gt 2 ]; then
		size=$test_linkfail
		...
		make_file "$cinfail" "client" $size
    ...
	if [ "$test_linkfail" -gt 2 ]; then
		size=$test_linkfail
		...
		make_file "$sinfail" "server" $size
    ...
	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP ${connect_addr}
}

That reads 2 x 128 MiB from /dev/urandom in 1 KiB blocks
(mptcp_lib_make_file() uses dd with bs=1024). All of this runs in the
background subshell before the listener and connector start. The mktemp
calls above only create the file names.

The return value of this wait_counter() call is ignored. File generation
plus connection setup might not fit in the timeout_poll window, for
example on a KASAN or lockdep debug kernel, or under emulation. In that
case sf_retry_after_block 10.0.3.2 fails with "no established subflow
from 10.0.3.2 before the block".

The later phases would then fail in turn. The absolute
wait_counter $ns2 "MPTcpExtMPJoinSynTx" 4 waits out its full timeout,
chk_no_sf_connecting() reports "no subflow re-created before the close",
and chk_join_nr and chk_evt_nr fail.

The commit message says the transfer lasts the whole test "on any
machine", but that covers the transfer, not this setup time.

Would it help to check the result of this first wait, or to generate the
files in the foreground before starting the background job?

> +		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
> +		chk_no_sf_connecting ${retrying}

[Severity: Low]

Does chk_no_sf_connecting() actually exercise
mptcp_close_retry_subflows()?

mptcp_lib_kill_group_wait() closes the connection by killing both
mptcp_connect processes in the middle of a bidirectional transfer. If the
client msk still has unread data at that point, __mptcp_close() takes the
fastclose path:

net/mptcp/protocol.c:__mptcp_close() {
    ...
	if (mptcp_data_avail(msk) || timeout < 0 ||
	    (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime)) {
		...
		mptcp_do_fastclose(sk);
		timeout = 0;
	} else if (mptcp_close_state(sk)) {
		__mptcp_wr_shutdown(sk);
	}

	mptcp_close_retry_subflows(sk);
    ...
}

mptcp_do_fastclose() moves the msk to TCP_CLOSE. __mptcp_destroy_sock()
then closes every subflow, including the SYN_SENT retry subflow, whatever
the new helper does.

The same happens if the server is killed first with unread data. It sends
MP_FASTCLOSE, and on the client mptcp_check_fastclose() closes every
subflow not already in TCP_CLOSE, including SYN_SENT ones:

	if (tcp_sk->sk_state != TCP_CLOSE) {
		mptcp_send_active_reset_reason(tcp_sk);
		tcp_set_state(tcp_sk, TCP_CLOSE);
	}

The helper only seems to matter on the graceful close path. Which path a
run takes depends on timing that the test doesn't control.

Could the "no subflow connecting after close" check pass even if
mptcp_close_retry_subflows() were broken or removed? Would a drained,
graceful close make this check deterministic?

[ ... ]

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

Re: [PATCH mptcp-next v5 0/7] mptcp: pm: in-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: Critical: No conclusion ❓
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/39578295986

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


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)