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

[PATCH mptcp-next v8 0/6] 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 gives up on an established subflow we created, after a
retransmission or keepalive timeout, the in-kernel PM now creates a new
one to the same addresses. Its SYN retries space out the attempts.
Their number is set by a new limit, subflow-retries, like TCP_SYNCNT:
the maximum, 127, by default, about 4 hours, and 0 disables it.

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

Changes in v8, after the Sashiko reviews of v7 and Matthieu's comments:
the default number of SYN retries is the maximum, 127; the re-created
subflows still connecting are closed before the write shutdown
disconnects them; the doc is more precise; and a separate test closes
the connection while a subflow is being re-created.

Changes in v7, after the netdev Sashiko reviews of v5 and v6, and
Abdul's tests: only a path error, a retransmission or keepalive
timeout, makes the PM re-create a subflow, not a reset, so a peer
cannot make it loop; the flags are bools under the msk socket lock;
mptcp.rst documents tcp_retries2 and the keepalive needed by a subflow
that only receives data; and small fixes in the tests.

Changes in v6, after the Sashiko reviews (netdev Sashiko on v4, which
also applied to v5, and sashiko.dev on v5): only subflows that were
established are re-created, so the PM still moves on to the next
endpoints as before; removed remote addresses and the local ID 0 are
handled too; the policy checks the maximum value, so the parse_limit()
patch is gone; and the test uses the net/lib.sh helpers. Details in
each patch.

v7: https://lore.kernel.org/mptcp/20261020031711.1896660-1-anilkaushikwireless@gmail.com/
v6: https://lore.kernel.org/mptcp/20261018041748.1655711-1-anilkaushikwireless@gmail.com/
v5: https://lore.kernel.org/mptcp/20261016160926.1155262-1-anilkaushikwireless@gmail.com/
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 (6):
  selftests: mptcp: connect: retry write() on EAGAIN
  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     |  13 +
 Documentation/networking/mptcp.rst            |  14 +
 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                                |  39 +++
 net/mptcp/pm_kernel.c                         | 141 +++++++++-
 net/mptcp/protocol.c                          |  36 +++
 net/mptcp/protocol.h                          |  20 ++
 net/mptcp/sockopt.c                           |   2 +
 .../selftests/net/mptcp/mptcp_connect.c       |  18 ++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 263 +++++++++++++++++-
 .../testing/selftests/net/mptcp/pm_netlink.sh |  47 +++-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c |  34 ++-
 15 files changed, 619 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH mptcp-next v8 1/6] 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:
    v6:
     - the comment says the limit only applies with a poll timeout (netdev
       Sashiko)
    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

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 178d98d91fea..49017d00ab69 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,22 @@ 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 if a poll timeout
+					 * is set: 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 +740,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 v8 2/6] 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:
    v8:
     - the default is the maximum, MAX_TCP_SYNCNT (127) (Matthieu)
    v7:
     - the doc gives the default value (netdev Sashiko)
    v6 (netdev Sashiko):
     - the policy checks the maximum value, 127 (MAX_TCP_SYNCNT)
     - the doc says the value works like TCP_SYNCNT
    v5:
     - new, split from the retry patch (Matthieu)

 Documentation/netlink/specs/mptcp_pm.yaml | 13 +++++++++++++
 include/uapi/linux/mptcp_pm.h             |  1 +
 net/mptcp/mptcp_pm_gen.c                  | 10 ++++++----
 net/mptcp/mptcp_pm_gen.h                  |  4 ++--
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/Documentation/netlink/specs/mptcp_pm.yaml b/Documentation/netlink/specs/mptcp_pm.yaml
index 39f3facc38e5..5374f06b04dc 100644
--- a/Documentation/netlink/specs/mptcp_pm.yaml
+++ b/Documentation/netlink/specs/mptcp_pm.yaml
@@ -196,6 +196,16 @@ attribute-sets:
         name: addr-remote
         type: nest
         nested-attributes: address
+      -
+        name: subflow-retries
+        type: u32
+        checks:
+          max: 127
+        doc: |
+          SYN retries of a subflow re-created after an error, like the
+          TCP_SYNCNT socket option, so with tcp_syn_linear_timeouts on top.
+          The default is the maximum, MAX_TCP_SYNCNT (127), about 4 hours.
+          0 to not re-create it.
   -
     name: event-attr
     enum-name: mptcp-event-attr
@@ -327,6 +337,7 @@ operations:
           attributes:
             - rcv-add-addrs
             - subflows
+            - subflow-retries
     -
       name: get-limits
       doc: Get protocol limits
@@ -337,10 +348,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..e2d4af1bc1d5 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] = NLA_POLICY_MAX(NLA_U32, 127),
 };
 
 /* 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] = NLA_POLICY_MAX(NLA_U32, 127),
 };
 
 /* 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

[PATCH mptcp-next v8 3/6] 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 retries of a subflow re-created after an error, like
the TCP_SYNCNT socket option. The default is the maximum, MAX_TCP_SYNCNT
(127), about 4 hours, and 0 to not re-create it. The netlink policy
checks this 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:
    v8:
     - the default is the maximum, 127, about 4 hours (Matthieu)
    v6:
     - the netlink policy checks the maximum, no more parse_limit() change
       (netdev Sashiko)
     - the limit works like TCP_SYNCNT, say so (netdev Sashiko)
    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      | 23 ++++++++++++++++++++++-
 net/mptcp/protocol.h       |  1 +
 net/mptcp/sockopt.c        |  2 ++
 4 files changed, 26 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 d3014bf57bf3..67bf421d117b 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,8 @@ 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;
+	struct nlattr *attr;
 	int ret;
 
 	spin_lock_bh(&pernet->lock);
@@ -1433,8 +1443,13 @@ int mptcp_pm_nl_set_limits_doit(struct sk_buff *skb, struct genl_info *info)
 	if (ret)
 		goto unlock;
 
+	/* the maximum, MAX_TCP_SYNCNT, is checked by the policy */
+	attr = info->attrs[MPTCP_PM_ATTR_SUBFLOW_RETRIES];
+	retries = nla_get_u32_default(attr, pernet->limit_subflow_retries);
+
 	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 +1479,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);
 
@@ -1618,6 +1637,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;
+	/* the maximum, MAX_TCP_SYNCNT: about 4 hours with the default RTOs */
+	pernet->limit_subflow_retries = 127;
 	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

[PATCH mptcp-next v8 4/6] 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 gives up on an established subflow created by this side, after
a retransmission or keepalive timeout, the PM now creates a new one with
the same addresses. Its SYN
retries, set like TCP_SYNCNT 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.

A subflow reset by the peer, or by anything on the path, is not
re-created: the path works, and re-creating it right away could loop.
A subflow that never got established is not re-created either: the PM
keeps trying the next endpoints, as before. Subflows aborted locally,
also by MPTCP, or using an address that has been removed are not
retried.

TCP only closes a subflow after tcp_retries2 retransmissions, about 15
minutes by default. A subflow that only receives data never gets an
error: the application has to enable TCP keepalive on the MPTCP socket,
which MPTCP applies to all the subflows. Both are documented in
mptcp.rst.

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

Notes:
    v8:
     - mptcp.rst gives the default, 127, about 4 hours
     - close the re-created subflows still connecting before
       __mptcp_wr_shutdown() disconnects them, and also the already
       disconnected ones, e.g. after shutdown(SHUT_WR) (sashiko.dev)
     - mptcp.rst: the initial subflow on the client side, the subflows
       limit, and only one attempt (netdev Sashiko)
    v7:
     - only after a path error, a retransmission or keepalive timeout: a
       reset from the peer or the path could make it loop (netdev
       Sashiko)
     - retry and no_retry are bools under the msk socket lock, in the
       padding after close_event_done, not bits next to the reset ones
       (netdev Sashiko)
     - mark all the subflows using a removed address, whatever their
       state (netdev Sashiko)
     - doc: tcp_retries2, and keepalive for a subflow that only receives
       data (Abdul Wasey's tests)
     - not retried after a reset by MPTCP itself, even with an old sk_err
       left by IP_RECVERR (netdev Sashiko)
     - close the re-created subflows still connecting without
       MPTCP_CF_PUSH: they never carried data (netdev Sashiko)
    v6 (netdev Sashiko):
     - only re-create subflows that were established: the PM still tries the
       next endpoints when one never gets established
     - only once TCP closed the subflow, to read its reset flags safely
     - removed addresses, also the remote ones and the local ID 0: the dead
       subflows are marked in mptcp_pm_rm_addr_or_subflow(), instead of
       looking up the endpoint
    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

 Documentation/networking/mptcp.rst |  14 ++++
 net/mptcp/pm.c                     |  39 ++++++++++
 net/mptcp/pm_kernel.c              | 118 +++++++++++++++++++++++++++++
 net/mptcp/protocol.c               |  36 +++++++++
 net/mptcp/protocol.h               |  19 +++++
 5 files changed, 226 insertions(+)

diff --git a/Documentation/networking/mptcp.rst b/Documentation/networking/mptcp.rst
index b6753ffb9c9a..62f2e6dd8584 100644
--- a/Documentation/networking/mptcp.rst
+++ b/Documentation/networking/mptcp.rst
@@ -73,6 +73,20 @@ To be able to use multiple IP addresses on a host to create multiple *subflows*
 addresses can be used. This can be configured with ``ip mptcp endpoint`` for
 example.
 
+When TCP gives up on an established subflow created by this host, after a
+retransmission or keepalive timeout, the in-kernel path manager creates it
+again, with the ``subflow-retries`` limit as its number of SYN retries, like
+``TCP_SYNCNT``: 127 by default, about 4 hours. This is done for the additional
+subflows, and for the initial one on the client side: the new subflow then takes
+one of the additional subflows allowed by the ``subflows`` limit. If it cannot
+connect, it is not created again. A subflow reset by the peer, or by anything on
+the path, is not created again either. TCP only gives up on a subflow after
+``tcp_retries2`` retransmissions, about 15 minutes by default: shorter outages
+are handled by TCP itself. A subflow that only receives data does not notice
+that its path stopped working: the application should then enable TCP keepalive
+on the MPTCP socket (``SO_KEEPALIVE``, ``TCP_KEEPIDLE``, ``TCP_KEEPINTVL`` and
+``TCP_KEEPCNT``), which MPTCP applies to all its subflows.
+
 Packet Scheduler
 ----------------
 
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d7c5b50b34cc..2bb08be95cd1 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)
 {
@@ -762,6 +782,24 @@ static void mptcp_pm_add_addr_send_ack(struct mptcp_sock *msk)
 	mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
 }
 
+/* A subflow using a removed address must not be re-created, whatever its
+ * state, e.g. closed by TCP but not collected yet, see
+ * mptcp_pm_kernel_retry_check().
+ */
+static void mptcp_pm_rm_no_retry(struct mptcp_subflow_context *subflow,
+				 enum linux_mptcp_mib_field rm_type, u8 rm_id)
+{
+	u8 id;
+
+	if (rm_type == MPTCP_MIB_RMADDR)
+		id = READ_ONCE(subflow->remote_id);
+	else
+		id = subflow_get_local_id(subflow);
+
+	if (id == rm_id)
+		subflow->no_retry = true;
+}
+
 static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 					const struct mptcp_rm_list *rm_list,
 					enum linux_mptcp_mib_field rm_type)
@@ -794,6 +832,7 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 			int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
 			u8 id = subflow_get_local_id(subflow);
 
+			mptcp_pm_rm_no_retry(subflow, rm_type, rm_id);
 			if ((1 << inet_sk_state_load(ssk)) &
 			    (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
 				continue;
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 67bf421d117b..23e8af94b1a4 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1610,6 +1610,124 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
 	return true;
 }
 
+/* TCP gave up on the path: a retransmission or keepalive timeout, possibly
+ * reported with an ICMP error received from the path.
+ */
+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 sock *ssk = mptcp_subflow_tcp_sock(subflow);
+	struct sock *sk = (struct sock *)msk;
+	int err;
+
+	/* Only once TCP closed it: the acquire in inet_sk_state_load() makes
+	 * sure sk_err and the resetting flag checked below are the ones of
+	 * this close.
+	 */
+	if (inet_sk_state_load(ssk) != TCP_CLOSE)
+		return false;
+
+	err = READ_ONCE(ssk->sk_err);
+	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;
+
+	/* Only a path that worked is tried again. If a subflow never got
+	 * established, the PM moves on to the next endpoints instead, and a
+	 * re-created subflow that cannot connect is not retried again: its SYN
+	 * retransmissions already spaced out the tries.
+	 */
+	if (!READ_ONCE(subflow->fully_established))
+		return false;
+
+	/* its address has been removed, see mptcp_pm_rm_addr_or_subflow() */
+	if (subflow->no_retry)
+		return false;
+
+	/* Only when the path stopped working. A reset, from the peer or from
+	 * anything on the path, means the path works: re-creating the subflow
+	 * right away could loop. Not after a reset by MPTCP itself either, with
+	 * mptcp_subflow_reset(), whatever sk_err says.
+	 */
+	if (!mptcp_pm_retry_path_error(err) || subflow->resetting)
+		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;
+
+	mptcp_remote_address((struct sock_common *)ssk, &retry->remote);
+	retry->remote.id = READ_ONCE(subflow->remote_id);
+	if (!retry->remote.id && READ_ONCE(msk->pm.remote_deny_join_id0))
+		return false;
+
+	mptcp_local_address((struct sock_common *)ssk, &retry->local.addr);
+	retry->local.addr.port = 0;
+	retry->local.addr.id = subflow_get_local_id(subflow);
+	retry->local.flags = subflow->request_bkup ?
+			     MPTCP_PM_ADDR_FLAG_BACKUP : 0;
+	retry->local.ifindex = ssk->sk_bound_dev_if;
+
+	return true;
+}
+
+/* Called with the msk socket lock held, 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 = true;
+	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..272bf2beac53 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,34 @@ 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);
+		int state;
+
+		if (!subflow->retry || READ_ONCE(subflow->fully_established))
+			continue;
+
+		/* still connecting, or already disconnected by
+		 * mptcp_subflow_shutdown(), e.g. after shutdown(SHUT_WR)
+		 */
+		state = inet_sk_state_load(ssk);
+		if (state != TCP_SYN_SENT && state != TCP_CLOSE)
+			continue;
+
+		/* it never carried data: nothing to push or re-inject */
+		mptcp_pm_subflow_check_next(msk, subflow);
+		__mptcp_close_ssk(sk, ssk, subflow, 0);
+	}
+}
+
 bool __mptcp_close(struct sock *sk, long timeout)
 {
 	struct mptcp_subflow_context *subflow;
@@ -3509,6 +3542,9 @@ bool __mptcp_close(struct sock *sk, long timeout)
 		goto cleanup;
 	}
 
+	/* before __mptcp_wr_shutdown() disconnects them */
+	mptcp_close_retry_subflows(sk);
+
 	if (mptcp_data_avail(msk) || timeout < 0 ||
 	    (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime)) {
 		/* If the msk has read data, or the caller explicitly ask it,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6d571328aca9..7e33ead76ee6 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -608,6 +608,8 @@ struct mptcp_subflow_context {
 	bool	close_event_done;   /* has done the post-closed part,
 				     * protected by msk socket lock
 				     */
+	bool	retry;		    /* re-created after an error, msk lock */
+	bool	no_retry;	    /* uses a removed address, msk lock */
 	u32	lent_mem_frag;
 	u32	remote_nonce;
 	u32	local_nonce;
@@ -1137,6 +1139,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 +1272,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 v8 5/6] 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:
    v8:
     - the default value is now 127
    v7:
     - also check that the maximum value, 127, is accepted (netdev
       Sashiko)
    v5:
     - new (Matthieu)

 .../testing/selftests/net/mptcp/pm_netlink.sh | 47 ++++++++++++++++++-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 34 +++++++++++---
 2 files changed, 72 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..99072ad8052d 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 127
+	elif mptcp_lib_expect_all_features; then
+		# missing feature: the defaults limits check will fail
+		echo 127
+	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,27 @@ 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} \
+		127
+	check "get_limits" "$(format_limits ${max_limits} ${max_limits} 127)" \
+		"set max 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

[PATCH mptcp-next v8 6/6] 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.

A second test closes the connection while a subflow is being re-created,
and checks the netlink events. Only the client sends data, the server
keeps its side open, and only the client is closed: it has nothing left
to read, so the connection is not closed with a fastclose, which would
reset all the subflows anyway. The check is skipped if a fastclose
happens. The big files used for the long connection are emptied at the
end.

The test is skipped if pm_nl_ctl does not show the new limit, or if the
filter rules cannot be set.

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

Notes:
    v8:
     - comment in run_tests() matching the new condition (netdev Sashiko)
     - a separate test closes the connection while a subflow is
       re-created: the server sends nothing but keeps its side open, and
       only the client is closed, so there is no fastclose, which would
       reset all the subflows anyway; the check is skipped if one happens
       (netdev Sashiko)
    v7 (netdev Sashiko):
     - the test_linkfail=1/2 modes regenerate the shared files when they
       are empty, e.g. when running link_failure_tests() twice
     - read the MP_JOIN ACK counter before letting the packets through,
       and do not compare the ports, they can be reused
    v6 (Sashiko):
     - use the slowwait helpers from net/lib.sh
     - longer first wait, while the 128 MB files are created
     - skip if the filter rules cannot be set, check each drop rule
       (netdev Sashiko)
     - empty the big files at the end, cinsent too, keep the variables
       (netdev Sashiko)
    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 | 263 +++++++++++++++++-
 1 file changed, 255 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaeda4f..27cd37347d5d 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -52,6 +52,7 @@ nr_blank=6
 # These var are used only in some tests, make sure they are not already set
 unset FAILING_LINKS
 unset test_linkfail
+unset server_in
 unset addr_nr_ns1
 unset addr_nr_ns2
 unset cestab_ns1
@@ -1038,7 +1039,9 @@ do_transfer()
 	fi
 
 	extra_srv_args="$extra_args $extra_srv_args"
-	if [ "$test_linkfail" -gt 1 ];then
+	if [ -n "${server_in}" ]; then
+		listener_in="${server_in}"
+	elif [ "$test_linkfail" -gt 1 ]; then
 		listener_in="${sinfail}"
 	fi
 	ip netns exec ${listener_ns} \
@@ -1135,9 +1138,9 @@ run_tests()
 			cinfail=$(mktemp)
 		fi
 		make_file "$cinfail" "client" $size
-	# create the input file for the failure test when
-	# the first failure test run
-	elif [ "$test_linkfail" -ne 0 ] && [ -z "$cinfail" ]; then
+	# create the input file for the failure test, when
+	# it is missing or has been emptied
+	elif [ "$test_linkfail" -ne 0 ] && [ ! -s "$cinfail" ]; then
 		# the client file must be considerably larger
 		# of the maximum expected cwin value, or the
 		# link utilization will be not predicable
@@ -1146,23 +1149,27 @@ run_tests()
 		size=$((size*8192))
 		size=$((size + ( RANDOM % 8192) ))
 
-		cinfail=$(mktemp)
+		if [ -z "$cinfail" ]; then
+			cinfail=$(mktemp)
+		fi
 		make_file "$cinfail" "client" $size
 	fi
 
-	if [ "$test_linkfail" -gt 2 ]; then
+	if [ "$test_linkfail" -gt 2 ] && [ -z "${server_in}" ]; then
 		size=$test_linkfail
 
 		if [ -z "$sinfail" ]; then
 			sinfail=$(mktemp)
 		fi
 		make_file "$sinfail" "server" $size
-	elif [ "$test_linkfail" -eq 2 ] && [ -z "$sinfail" ]; then
+	elif [ "$test_linkfail" -eq 2 ] && [ ! -s "$sinfail" ]; then
 		size=$((RANDOM%16))
 		size=$((size+1))
 		size=$((size*2048))
 
-		sinfail=$(mktemp)
+		if [ -z "$sinfail" ]; then
+			sinfail=$(mktemp)
+		fi
 		make_file "$sinfail" "server" $size
 	fi
 
@@ -2151,6 +2158,183 @@ wait_attempt_fail()
 	return 1
 }
 
+# $1: timeout in seconds ; $2: ns ; $3: counter ; $4: increment, 1 by default
+wait_counter_inc()
+{
+	slowwait_for_counter "${1}" "${4:-1}" \
+		mptcp_lib_get_counter "${2}" "${3}" >/dev/null
+}
+
+# $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"
+}
+
+# the client packets are dropped with rules in this chain
+init_nft_filter()
+{
+	init_nftables
+	if ! ip netns exec $ns2 nft list chain ip filter OUTPUT \
+	     &>/dev/null; then
+		mark_as_skipped "unable to set the filter rules"
+		return 1
+	fi
+}
+
+# $1: client address
+nft_drop_from()
+{
+	ip netns exec $ns2 nft add rule ip filter OUTPUT ip saddr "${1}" drop
+}
+
+# $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 before ack_rx
+
+	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
+
+	if ! nft_drop_from "${1}"; then
+		fail_test "unable to drop the packets from ${1}"
+		return
+	fi
+	if ! wait_counter_inc "${timeout_poll}" $ns2 "MPTcpExtMPJoinSynTx"; 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_inc "${timeout_poll}" \
+		$ns2 "TcpExtTCPSynRetrans" 2; then
+		ip netns exec $ns2 nft flush chain ip filter OUTPUT
+		fail_test "SYN from ${1} not retransmitted enough"
+		return
+	fi
+	# read before the flush: the next SYN can be sent right after it
+	ack_rx=$(mptcp_lib_get_counter $ns1 "MPTcpExtMPJoinAckRx")
+	ip netns exec $ns2 nft flush chain ip filter OUTPUT
+
+	if ! slowwait "${timeout_poll}" until_counter_is ">= $((ack_rx + 1))" \
+	     mptcp_lib_get_counter $ns1 "MPTcpExtMPJoinAckRx" >/dev/null; then
+		fail_test "re-created subflow from ${1} not established"
+		return
+	fi
+
+	if [ -z "$(sf_from "${1}")" ]; then
+		fail_test "no established subflow from ${1}"
+	else
+		print_ok
+	fi
+}
+
+# $1: ns ; prints the number of MP_FASTCLOSE sent and received
+fastclose_nr()
+{
+	local tx rx
+
+	tx=$(mptcp_lib_get_counter "${1}" "MPTcpExtMPFastcloseTx")
+	rx=$(mptcp_lib_get_counter "${1}" "MPTcpExtMPFastcloseRx")
+	echo $((${tx:-0} + ${rx:-0}))
+}
+
+no_sf_connecting()
+{
+	! ip netns exec $ns2 ss -tnH state syn-sent | grep -q .
+}
+
+# The client has nothing left to read: it closes the connection without a
+# fastclose, which would reset all the subflows anyway.
+kill_client()
+{
+	local pid
+
+	for pid in $(ip netns pids $ns2); do
+		if [ "$(cat "/proc/${pid}/comm" 2>/dev/null)" = mptcp_connect ]
+		then
+			kill "${pid}"
+		fi
+	done
+}
+
+# $1: result of the wait for the subflow being re-created before the close
+# $2: number of MP_FASTCLOSE before the close
+chk_no_sf_connecting()
+{
+	local closed
+
+	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
+	slowwait "${timeout_poll}" no_sf_connecting
+	closed=$?
+	if [ "$(fastclose_nr $ns2)" -gt "${2}" ]; then
+		print_skip "closed with a fastclose, resetting all the subflows"
+	elif [ "${closed}" -eq 0 ]; then
+		print_ok
+	else
+		fail_test "subflow still connecting after close"
+	fi
+}
+
+# Slow links with short queues: the transfer lasts the whole test on any
+# machine, and the blocked subflow always has data in flight.
+sf_retry_init()
+{
+	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
+}
+
+sf_retry_cleanup()
+{
+	ip netns exec $ns2 nft flush chain ip filter OUTPUT
+	# big files: empty them, the next tests write their own content
+	: > "$cinfail"
+	: > "$sinfail"
+	: > "$cinsent"
+}
+
 set_userspace_pm()
 {
 	local ns=$1
@@ -2585,6 +2769,69 @@ 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
+	if reset_with_events "subflow retry after link failure" &&
+	   continue_if has_subflow_retries && init_nft_filter; then
+		sf_retry_init
+		# 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=$!
+
+		# the 128 MB files are created first, that can take a while
+		wait_counter_inc $((timeout_poll * 4)) \
+			$ns1 "MPTcpExtMPJoinAckRx"
+		sf_retry_after_block 10.0.3.2
+		sf_retry_after_block 10.0.1.2
+		mptcp_lib_kill_group_wait $tests_pid
+		kill_events_pids
+
+		join_syn_tx=3 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 2 error 2
+		sf_retry_cleanup
+	fi
+
+	# close the connection while a subflow is being re-created
+	if reset_with_events "subflow retry, close while connecting" &&
+	   continue_if has_subflow_retries && init_nft_filter; then
+		sf_retry_init
+		# The server sends nothing, but does not close its side: the
+		# client has nothing to read, and the connection stays
+		# established.
+		local srv_in srv_in_pid
+		srv_in=$(mktemp -u)
+		mkfifo "${srv_in}"
+		sleep 600 > "${srv_in}" &
+		srv_in_pid=$!
+		{ timeout_test=600 test_linkfail=131072 server_in="${srv_in}" \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
+		local tests_pid=$!
+
+		wait_counter_inc $((timeout_poll * 4)) \
+			$ns1 "MPTcpExtMPJoinAckRx"
+		local retrying=1
+		if nft_drop_from 10.0.3.2; then
+			wait_counter_inc "${timeout_poll}" \
+				$ns2 "MPTcpExtMPJoinSynTx"
+			retrying=$?
+		fi
+		local fastclose
+		fastclose=$(fastclose_nr $ns2)
+		kill_client
+		chk_no_sf_connecting ${retrying} ${fastclose}
+		mptcp_lib_kill_group_wait $tests_pid
+		mptcp_lib_kill_wait ${srv_in_pid}
+		rm -f "${srv_in}"
+		kill_events_pids
+
+		join_syn_tx=2 chk_join_nr 1 1 1
+		chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_ESTABLISHED 1
+		chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1 error 1
+		sf_retry_cleanup
+	fi
 }
 
 add_addr_timeout_tests()
-- 
2.25.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: mptcp: blocking FIFO stdin stalls server in copyfd_io_poll()
--

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

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

This commit adds two selftests in mptcp_join.sh to validate subflow retries
after packet drops using netfilter rules. The first test verifies that
additional and initial subflows reconnect after silent packet drops, and the
second test verifies closing the client connection while a subflow is being
re-created.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c3ccce96b45b1..58a48f50ba152 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
[ ... ]
> @@ -1038,7 +1039,9 @@ do_transfer()
>  	fi
>  
>  	extra_srv_args="$extra_args $extra_srv_args"
> -	if [ "$test_linkfail" -gt 1 ];then
> +	if [ -n "${server_in}" ]; then
> +		listener_in="${server_in}"
> +	elif [ "$test_linkfail" -gt 1 ]; then
>  		listener_in="${sinfail}"
>  	fi
>  	ip netns exec ${listener_ns} \
[ ... ]
> @@ -2585,6 +2769,69 @@ link_failure_tests()
[ ... ]
> +	# close the connection while a subflow is being re-created
> +	if reset_with_events "subflow retry, close while connecting" &&
> +	   continue_if has_subflow_retries && init_nft_filter; then
> +		sf_retry_init
> +		# The server sends nothing, but does not close its side: the
> +		# client has nothing to read, and the connection stays
> +		# established.
> +		local srv_in srv_in_pid
> +		srv_in=$(mktemp -u)
> +		mkfifo "${srv_in}"
> +		sleep 600 > "${srv_in}" &
> +		srv_in_pid=$!
> +		{ timeout_test=600 test_linkfail=131072 server_in="${srv_in}" \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null

[Severity: Medium]
Does redirecting the server's stdin from the srv_in FIFO cause the server
mptcp_connect process to block in read() and stop draining incoming data from
the client?

In link_failure_tests() -> run_tests() -> do_transfer(), the server is started
in poll mode (-j / CFG_MODE_POLL) with stdin redirected from listener_in
(srv_in), while sleep 600 holds the write end of the FIFO open without writing
any bytes.

In tools/testing/selftests/net/mptcp/mptcp_connect.c:copyfd_io_poll(), only
peerfd is set to non-blocking mode and passed to poll(), while infd (stdin)
remains blocking:

	set_nonblock(peerfd, true);

	for (;;) {
		...
		switch (poll(&fds, 1, poll_timeout)) {
		...
		}

		if (fds.revents & POLLIN) {
			...
		}

		if (fds.revents & POLLOUT) {
			if (winfo->len == 0) {
				winfo->off = 0;
				winfo->len = read(infd, winfo->buf, sizeof(winfo->buf));
			}

Because peerfd has POLLOUT set on the first poll() iteration after accept(),
the server calls read(infd, ...) on the FIFO and blocks for 600 seconds. As a
result, the server never returns to poll() and never reads from peerfd again.

Once the client fills the initial ~64 KB receive window, the server advertises
a zero receive window and stops acknowledging new data. If wait_counter_inc
for MPTcpExtMPJoinAckRx and nft_drop_from 10.0.3.2 run after that initial
window has been sent and acknowledged, subflow 10.0.3.2 has no unacknowledged
data in flight (and zero-window probes are only sent on the initial subflow
10.0.1.2), so subflow 10.0.3.2 never times out or gets re-created, causing
wait_counter_inc on MPTcpExtMPJoinSynTx to time out.

[ ... ]

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

Re: [PATCH mptcp-next v8 4/6] mptcp: pm: in-kernel: retry subflows closed after an error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: race in mptcp_close_retry_subflows() due to unlocked ssk state check
--

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

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

This commit teaches the in-kernel path manager to re-create established
client-initiated or joined subflows that were closed by TCP after a path
timeout error. It also closes any re-created subflows that are still
connecting when the parent MPTCP socket is closed.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..f941533a1eb48 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -3501,6 +3506,34 @@ 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);
> +		int state;
> +
> +		if (!subflow->retry || READ_ONCE(subflow->fully_established))
> +			continue;
> +
> +		/* still connecting, or already disconnected by
> +		 * mptcp_subflow_shutdown(), e.g. after shutdown(SHUT_WR)
> +		 */
> +		state = inet_sk_state_load(ssk);
> +		if (state != TCP_SYN_SENT && state != TCP_CLOSE)
> +			continue;
> +
> +		/* it never carried data: nothing to push or re-inject */
> +		mptcp_pm_subflow_check_next(msk, subflow);
> +		__mptcp_close_ssk(sk, ssk, subflow, 0);
> +	}
> +}

[Severity: High]
Can mptcp_close_retry_subflows() in net/mptcp/protocol.c race with softirq
packet processing on ssk because subflow->fully_established and
inet_sk_state_load(ssk) are checked before ssk is locked in
__mptcp_close_ssk()?

When __mptcp_close() calls mptcp_close_retry_subflows(sk), the msk socket
lock is held, sk->sk_state is still TCP_ESTABLISHED, and msk->allow_subflows
is true, while ssk is not locked until __mptcp_close_ssk() runs:

net/mptcp/protocol.c:__mptcp_close_ssk() {
    lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
    subflow->closing = 1;

    if (flags & MPTCP_CF_PUSH)
        mptcp_cleanup_ssk_backlog(sk, ssk);
    ...
}

Between the lockless checks in mptcp_close_retry_subflows() and
lock_sock_nested(ssk) in __mptcp_close_ssk(), TCP softirq on another CPU can
process incoming packets under bh_lock_sock_nested(ssk):

1. If ssk is observed in TCP_SYN_SENT, an incoming MP_JOIN handshake can
transition ssk to TCP_ESTABLISHED via:

  tcp_rcv_synsent_state_process()
    -> subflow_finish_connect()
      -> mptcp_finish_join()

which succeeds without acquiring the msk socket lock because subflow->node
is on msk->conn_list and msk->allow_subflows is true. A subsequent packet
carrying the 4th ACK and DSS data can then set subflow->fully_established =
true in check_fully_established() and queue skbs onto msk->backlog_list via:

  subflow_data_ready()
    -> mptcp_data_ready()
      -> __mptcp_move_skbs_from_subflow(msk, ssk, false)
        -> __mptcp_add_backlog()

which sets skb->destructor = NULL and skb->sk = ssk while leaving
skb->truesize charged to ssk->sk_rmem_alloc.

2. Similarly, if ssk already completed the 3-way handshake
(TCP_ESTABLISHED, subflow->fully_established == false) when
READ_ONCE(subflow->fully_established) is checked, softirq can process the
4th ACK with DSS data (queuing skbs onto msk->backlog_list) and then a
subsequent FIN or RST transitioning ssk to TCP_CLOSE before
inet_sk_state_load(ssk) is read.

When __mptcp_close_ssk(sk, ssk, subflow, 0) then runs without MPTCP_CF_PUSH,
it skips mptcp_cleanup_ssk_backlog(sk, ssk) and destroys ssk while skbs on
msk->backlog_list still hold skb->sk = ssk:

net/mptcp/protocol.h:mptcp_borrow_fwdmem() {
    struct sock *ssk = skb->sk;
    ...
    if (!ssk)
        return;

    atomic_sub(skb->truesize, &ssk->sk_rmem_alloc);
    skb->sk = NULL;
}

Wouldn't this trigger the sk->sk_rmem_alloc warning in inet_sock_destruct()
and a use-after-free atomic_sub() on ssk->sk_rmem_alloc in
mptcp_borrow_fwdmem() when msk->backlog_list is later drained or purged?

不断地

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

Re: [PATCH mptcp-next v8 2/6] 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] Concern as raised: the commit "netlink: specs: mptcp: add the
  subflow-retries attribute" adds MPTCP_PM_ATTR_SUBFLOW_RETRIES to the…

--

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

This commit adds the MPTCP_PM_ATTR_SUBFLOW_RETRIES attribute to the
mptcp_pm netlink spec and the uapi header. It also adds the attribute to
the set-limits and get-limits policies, with a maximum of 127. The
in-kernel PM is meant to use it for the SYN retries of subflows that are
re-created after an error.

> diff --git a/net/mptcp/mptcp_pm_gen.c b/net/mptcp/mptcp_pm_gen.c
> index c180930a8e0e4..e2d4af1bc1d5d 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] = NLA_POLICY_MAX(NLA_U32, 127),
>  };

[Severity: Low]
At this commit, nothing in net/mptcp/pm_kernel.c handles the new
attribute yet:

- mptcp_pm_nl_set_limits_doit() never reads
  MPTCP_PM_ATTR_SUBFLOW_RETRIES.
- mptcp_pm_nl_get_limits_doit() does not put it in the reply.

The spec still lists the attribute in the get-limits reply and gives
127 as the default.

Is that acceptable for a kernel built from this commit alone, for
example when a bisect stops here?

This is mostly a note, because the rest of the series appears to cover
it. The next commit, "mptcp: pm: in-kernel: add the subflow-retries
limit", adds the parsing, the storage, the reply and the default of 127.
After that, "mptcp: pm: in-kernel: retry subflows closed after an error"
uses the value.

SET_LIMITS and GET_LIMITS also use GENL_DONT_VALIDATE_STRICT, so
genl_family_rcv_msg_attrs_parse() parses them with NL_VALIDATE_LIBERAL.
That means the attribute was already ignored, not rejected, before this
patch, so userspace sees no change in behavior at this commit.

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

Re: [PATCH mptcp-next v8 4/6] mptcp: pm: in-kernel: retry subflows closed after an error

[netdev-bot+sashiko]

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

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

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

- [High] TOCTOU race in mptcp_close_retry_subflows()
  (net/mptcp/protocol.c:3512) that can lead to a use-after-free.
- [Medium] This patch changes how a dead MPTCP client connection is
  detected, the change is on by default, and neither the commit message…
- [Medium] mptcp_pm_retry_path_error() (net/mptcp/pm_kernel.c:1616)
  treats only ETIMEDOUT, EHOSTUNREACH, ENETUNREACH, EHOSTDOWN and…

--

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

This patch makes the in-kernel PM re-create an established subflow that
this side initiated, once TCP gives up on it after a retransmission or
keepalive timeout. The new subflow takes its SYN retry count from the
subflow-retries limit. Re-created subflows that are still connecting are
closed when the MPTCP socket is closed.

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 67bf421d117b5..23e8af94b1a45 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1610,6 +1610,124 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
>  	return true;
>  }
>  
> +/* TCP gave up on the path: a retransmission or keepalive timeout, possibly
> + * reported with an ICMP error received from the path.
> + */
> +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;
> +}

[ ... ]

> +	/* Only when the path stopped working. A reset, from the peer or from
> +	 * anything on the path, means the path works: re-creating the subflow
> +	 * right away could loop. Not after a reset by MPTCP itself either, with
> +	 * mptcp_subflow_reset(), whatever sk_err says.
> +	 */
> +	if (!mptcp_pm_retry_path_error(err) || subflow->resetting)
> +		return false;

[Severity: Medium]
Does this allow-list miss timeouts that had other ICMP errors before them?

When TCP gives up after a retransmission or keepalive timeout,
tcp_write_err() sets sk_err to the last soft error, not to ETIMEDOUT:

net/ipv4/tcp_timer.c:tcp_write_err() {
	tcp_done_with_error(sk, READ_ONCE(sk->sk_err_soft) ? : ETIMEDOUT);
	...
}

On an established socket without IP_RECVERR, tcp_v4_err() stores non-PMTU
ICMP errors in sk_err_soft and keeps retransmitting:

net/ipv4/tcp_ipv4.c:tcp_v4_err() {
	...
	} else	{ /* Only an error on timeout */
		WRITE_ONCE(sk->sk_err_soft, err);
	}
	...
}

That covers ECONNREFUSED (port unreachable, e.g. iptables -j REJECT),
ENOPROTOOPT, ENONET and EPROTO. tcp_v4_err() also stores EMSGSIZE. On
IPv6, icmpv6_err_convert() maps admin-prohibited, policy-fail and
reject-route to EACCES, and parameter problem to EPROTO.

In all of these cases the subflow is in TCP_CLOSE because TCP timed out.
Still, mptcp_pm_kernel_retry_check() returns false here, so the subflow is
never re-created.

IPv4 and IPv6 also seem to behave differently here. On IPv4,
administratively prohibited or filtered maps to ENETUNREACH or
EHOSTUNREACH and is retried. The IPv6 equivalent maps to EACCES and is
not.

Can the errno alone tell a TCP timeout apart from the last soft error
seen? The comment above mptcp_pm_retry_path_error() and the new mptcp.rst
text say any retransmission or keepalive timeout leads to a retry, and
they only exclude resets.

[ ... ]

> +/* 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)
> +{

[ ... ]

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

[Severity: Medium]
Does this change how long a dead client connection with a single path
stays up?

Before this patch, when the last subflow of an established msk that had
not fallen back timed out, __mptcp_close_ssk() started the close timeout:

net/mptcp/protocol.c:__mptcp_close_ssk() {
	...
	if (list_is_singular(&msk->conn_list) && msk->first &&
	    inet_sk_state_load(msk->first) == TCP_CLOSE) {
		if (sk->sk_state != TCP_ESTABLISHED ||
		    msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
			...
		} else {
			mptcp_start_tout_timer(sk);
		}
	}
}

On such an msk, __mptcp_subflow_error_report() does not pass subflow
errors up. So this timer was how the application found out about the
failure: after close_timeout (60s by default), mptcp_worker() fastcloses
the msk and wakes the application.

With this patch, mptcp_pm_kernel_retry_check() accepts the client's
initial subflow, and the call chain becomes:

__mptcp_close_subflow()
  mptcp_close_ssk()
    __mptcp_close_ssk()
      mptcp_start_tout_timer()
  mptcp_pm_subflow_retry()
    mptcp_pm_kernel_retry()
      __mptcp_subflow_connect()
        mptcp_stop_tout_timer()

The defaults are subflows 2 and subflow-retries 127. With those, the new
subflow then stays in SYN_SENT for about 4 hours. During that time the msk
stays TCP_ESTABLISHED and the application gets no error.

mptcp.rst now recommends keepalive for detecting dead paths. With this
change, a keepalive timeout would reach the application after about 4
hours instead of about a minute.

Is this change in failure detection intended? Neither the commit message
nor mptcp.rst mentions it, or that setting subflow-retries to 0 turns the
retry off.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..f941533a1eb48 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -3501,6 +3506,34 @@ 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);
> +		int state;
> +
> +		if (!subflow->retry || READ_ONCE(subflow->fully_established))
> +			continue;
> +
> +		/* still connecting, or already disconnected by
> +		 * mptcp_subflow_shutdown(), e.g. after shutdown(SHUT_WR)
> +		 */
> +		state = inet_sk_state_load(ssk);
> +		if (state != TCP_SYN_SENT && state != TCP_CLOSE)
> +			continue;
> +
> +		/* it never carried data: nothing to push or re-inject */
> +		mptcp_pm_subflow_check_next(msk, subflow);
> +		__mptcp_close_ssk(sk, ssk, subflow, 0);

[Severity: High]
Can the subflow finish its handshake and receive data between these
lockless checks and the ssk lock taken inside __mptcp_close_ssk()?

Only the msk socket lock is held here. fully_established and the ssk
state are read without the ssk lock. subflow->closing is only set later,
inside __mptcp_close_ssk().

Holding the msk lock does not keep softirq away from the ssk. In that
window:

softirq
subflow_finish_connect()
  mptcp_finish_join()       <- msk still TCP_ESTABLISHED, since
                               mptcp_close_state() runs later
...
subflow_data_ready()
  mptcp_data_ready()        <- closing == 0, msk owned by user
    __mptcp_move_skbs_from_subflow(msk, ssk, false)
      __mptcp_add_backlog() <- skb on msk->backlog_list, skb->sk == ssk

Because flags is 0, __mptcp_close_ssk() then skips the cleanup that would
clear skb->sk:

	if (flags & MPTCP_CF_PUSH)
		mptcp_cleanup_ssk_backlog(sk, ssk);

It then disposes of the ssk. Once the FIN is acked, the orphaned ssk
reaches tcp_time_wait() or tcp_done() and is freed.

Later, release_sock(sk) at the end of mptcp_close() runs
mptcp_release_cb() and spools the backlog, or mptcp_backlog_purge() runs.
Either way, mptcp_borrow_fwdmem() does:

	atomic_sub(skb->truesize, &ssk->sk_rmem_alloc);

on the freed, and possibly reused, ssk. Is that a use-after-free? The
comment in mptcp_borrow_fwdmem() assumes that "mptcp_close_ssk() already
released the ssk rcv memory", which does not hold on this path.

The same window also lets a subflow that already emitted
MPTCP_EVENT_SUB_ESTABLISHED be closed without MPTCP_EVENT_SUB_CLOSED.

Would passing MPTCP_CF_PUSH, re-checking the state and fully_established
under the ssk lock, or going through mptcp_close_ssk() avoid this?

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

Re: [PATCH mptcp-next v8 5/6] 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 'subflow retries above hard limit' subtest in
  tools/testing/selftests/net/mptcp/pm_netlink.sh cannot show what the…

--

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

With this patch, 'pm_nl_ctl limits' can set the subflow-retries limit
through an optional third argument, and it prints the limit when the
kernel reports it. pm_netlink.sh is extended to check the default value,
that a value above the maximum is refused, and that the value can be set.

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

[ ... ]

> @@ -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 127
> +	elif mptcp_lib_expect_all_features; then
> +		# missing feature: the defaults limits check will fail
> +		echo 127
> +	fi
> +}

[ ... ]

> @@ -224,7 +247,27 @@ 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"

[Severity: Low]
Can this subtest tell a refused value apart from a clamped one?

The commit message says pm_netlink.sh checks "that a value above the
maximum is refused". Since v8, the per-netns default is the same value
as the policy maximum.

In net/mptcp/pm_kernel.c:pm_nl_init_net():

	pernet->limit_subflow_retries = 127;

In net/mptcp/mptcp_pm_gen.c, mptcp_pm_set_limits_nl_policy has:

	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = NLA_POLICY_MAX(NLA_U32, 127),

get_default_retries() also hardcodes 127, so max_limits_out already
contains "subflow_retries 127" when the request with 128 is sent. The
exit status and stderr of pm_nl_ctl are thrown away.

If the kernel refuses 128, the readback shows 127. If 128 were accepted
and clamped to 127, the readback would also show 127. Both outcomes pass
this check, so it only fails if 128 is stored as-is.

The earlier "rcv addrs above hard limit" and "subflows above hard limit"
checks work because the stored value differs from what the bad request
would set.

Could the 128 attempt be moved after the "set subflow retries" step, so
the check can verify the value stays at 0? Checking that pm_nl_ctl exits
non-zero for the 128 request would also work.

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

[ ... ]

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

Re: [PATCH mptcp-next v8 6/6] 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 'subflow retry, close while connecting' test in
  link_failure_tests() feeds the server a FIFO (`server_in`) that nobody…
- [Low] In the 'subflow retry, close while connecting' test,
  link_failure_tests() ignores the return value of the first…
- [Low] The second new test leaves the FIFO and its writer process behind
  if the script exits abnormally.
- [Low] The relative-counter waits take their baseline only after the
  asynchronous event has been triggered.
- [Low] The new 'subflow retry, close while connecting' test builds its
  FIFO path with `srv_in=$(mktemp -u)`, which picks a name but creates…

--

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

This patch adds two mptcp_join.sh tests for the subflow-retries limit. The
first test uses nftables to drop the client packets from one subflow
address until the subflow is closed and re-created, for the additional
subflow and then for the initial one. The second test closes the
connection while a subflow is being re-created and checks the netlink
events, with the server input taken from a FIFO that is never written.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c3ccce96b45b1..58a48f50ba152 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh

[ ... ]

> @@ -2151,6 +2158,183 @@ wait_attempt_fail()

[ ... ]

> +sf_retry_after_block()
> +{
> +	local before ack_rx

[ ... ]

> +	if ! nft_drop_from "${1}"; then
> +		fail_test "unable to drop the packets from ${1}"
> +		return
> +	fi
> +	if ! wait_counter_inc "${timeout_poll}" $ns2 "MPTcpExtMPJoinSynTx"; then
> +		ip netns exec $ns2 nft flush chain ip filter OUTPUT
> +		fail_test "subflow from ${1} not re-created"
> +		return
> +	fi

[Severity: Low]

Is the MPJoinSynTx baseline read too late here? wait_counter_inc() calls
slowwait_for_counter() in net/lib.sh, and that function reads the baseline
when it is called:

tools/testing/selftests/net/lib.sh:slowwait_for_counter() {
    ...
	local base=$("$@")
	slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
}

By then the drop rule is already installed. With tcp_retries2=3 the
subflow is closed after about 3 s. If the shell is descheduled long enough
for the subflow to time out and be re-created before the baseline is read,
the wait expects a second MP_JOIN SYN that never comes. The test then
reports "subflow from X not re-created".

The "close while connecting" test has the same ordering for the drop rule
and its MPJoinSynTx wait. So do both initial MPJoinAckRx waits, which start
after the background run_tests is launched.

Would it be more robust to read the absolute counter before installing the
rule, or before launching the background job, and poll for that target?
The TCPSynRetrans wait does not look affected, because a late baseline
only means waiting for more retransmissions.

[ ... ]

> @@ -2585,6 +2769,69 @@ link_failure_tests()

[ ... ]

> +	# close the connection while a subflow is being re-created
> +	if reset_with_events "subflow retry, close while connecting" &&
> +	   continue_if has_subflow_retries && init_nft_filter; then
> +		sf_retry_init
> +		# The server sends nothing, but does not close its side: the
> +		# client has nothing to read, and the connection stays
> +		# established.
> +		local srv_in srv_in_pid
> +		srv_in=$(mktemp -u)
> +		mkfifo "${srv_in}"

[Severity: Low]

Can another local user race with this? mktemp -u only picks a name in /tmp
and creates nothing. The result of mkfifo is not checked.

Suppose a symlink or a regular file is created at that path before mkfifo
runs. mkfifo fails with EEXIST and the script carries on. The root
redirection below then opens the path with O_WRONLY|O_CREAT|O_TRUNC, which
follows symlinks unless fs.protected_symlinks or fs.protected_regular is
set. The mptcp_connect server, run as root, also reads that path as stdin
through do_transfer().

The rest of mptcp_join.sh uses plain mktemp. Would it be safer to create a
directory with mktemp -d, make the FIFO inside it, and stop the test if
mkfifo fails?

> +		sleep 600 > "${srv_in}" &
> +		srv_in_pid=$!
> +		{ timeout_test=600 test_linkfail=131072 server_in="${srv_in}" \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!

[Severity: Medium]

Does the server ever read from the MPTCP socket in this test? When
server_in is set, do_transfer() starts the listener with the FIFO as
stdin. copyfd_io_poll() only makes peerfd non-blocking, and a freshly
accepted socket reports POLLOUT, so the server ends up here:

tools/testing/selftests/net/mptcp/mptcp_connect.c:copyfd_io_poll() {
    ...
		if (fds.revents & POLLOUT) {
			if (winfo->len == 0) {
				winfo->off = 0;
				winfo->len = read(infd, winfo->buf, sizeof(winfo->buf));
    ...
}

It then blocks on the silent FIFO until it is killed.

MPTCP only grows the receive buffer from the read paths, through
mptcp_rcv_space_adjust(). So the server window would close after about one
initial rcvbuf of data, which takes a fraction of a second at 2x4mbit.

By the time MPJoinAckRx has been polled and nft_drop_from 10.0.3.2 has
run, is any data still in flight on the 10.0.3.2 subflow? That seems to go
against the comment above sf_retry_init():

    # machine, and the blocked subflow always has data in flight.

If that subflow is idle, it only times out (tcp_retries2=3) when the
MPTCP-level zero-window probe from mptcp_sendmsg_frag() and the TCP probe0
timer happen to land on 10.0.3.2 rather than 10.0.1.2. The scheduler makes
that choice. Otherwise retrying stays 1, chk_no_sf_connecting() reports "no
subflow re-created before the close", and join_syn_tx=2 chk_join_nr 1 1 1
fails.

In this state the client can neither read nor write. Could it also leave
copyfd_io_poll() with "poll timed out" after timeout_poll and close the
connection on its own, racing with kill_client()?

The first test does not seem to have this problem, because its listener_in
is the regular file $sinfail.

> +
> +		wait_counter_inc $((timeout_poll * 4)) \
> +			$ns1 "MPTcpExtMPJoinAckRx"
> +		local retrying=1
> +		if nft_drop_from 10.0.3.2; then
> +			wait_counter_inc "${timeout_poll}" \
> +				$ns2 "MPTcpExtMPJoinSynTx"
> +			retrying=$?
> +		fi

[Severity: Low]

Should the result of the first wait_counter_inc() be checked here?
sf_retry_after_block() checks sf_from() before it blocks anything. Here,
nothing confirms that the initial MP_JOIN subflow is established before the
drop rule is installed.

That wait can time out, for example when creating the 128 MB files in
run_tests() takes too long on a slow debug kernel. The rule can then be in
place before the SYN of the original join. MPJoinSynTx goes up because of
that original SYN, and retrying becomes 0 even though nothing was
re-created. mptcp_pm_kernel_retry_check() does not retry a subflow that
was never fully established.

The test still fails later in chk_join_nr, chk_no_sf_connecting or
chk_evt_nr. The messages would point at the retry or close logic, though,
rather than at the precondition that failed.

[ ... ]

> +		mptcp_lib_kill_wait ${srv_in_pid}
> +		rm -f "${srv_in}"

[Severity: Low]

Are the FIFO and its sleep writer left behind if the script exits
abnormally during this test? They are only kept in local variables, and
they are only released on this normal path.

The EXIT trap cleanup() removes cin, cout, sinfail, sin, sout, cinsent,
cinfail, tmpfile, the event files and err. cleanup_partial() only handles
$capout and the namespaces.

The script may be interrupted before the listener has opened the FIFO.
That is likely while run_tests() is still creating the 128 MB cinfail. In
that case the forked shell stays blocked in open(O_WRONLY) and never exits.

sf_retry_init() already creates cinfail and sinfail up front "for
cleanup()". Could the FIFO path and the writer pid also be global, so that
cleanup() handles them as well?

[ ... ]

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