[PATCH net 00/14] mptcp: misc fixes for v7.3-rc1

[PATCH net 00/14] mptcp: misc fixes for v7.3-rc1

[Matthieu Baerts (NGI0)]

Here are various unrelated fixes:

- Patch 1: Do not reschedule the RTX timer for sockets that fell back to
  TCP. A fix for v5.7.

- Patch 2: Avoid copying thmac which will not be used and could be
  uninitialised. A fix for v5.7.

- Patch 3: Re-set the request backup flag when SYN cookies are used. A
  fix for v5.9.

- Patch 4: Drop pending ADD_ADDR when removing ID0, and avoid a WARN. A
  fix for v5.13.

- Patch 5: Handle invalid suboptions where the checksum is requested in
  the MP_CAPABLE 4th ACK with data, but not added in the option. A fix
  for v5.14.

- Patch 6: Fix a use-after-free in the selftests that could lead to
  false positive. A fix for v5.17.

- Patch 7: Limit new addresses with the userspace PM to avoid an address
  ID overflow. A fix for v5.19.

- Patch 8: Reset the ADD_ADDR retransmission counter when the timer is
  reused. A fix for v5.19.

- Patch 9: Remove unneeded and confusing READ_ONCE() annotations. A fix
  for v6.13.

- Patches 10-11: Get nstat counters for the current test, not since the
  creation of the netns. A fix for v6.19.

- Patch 12: Fix an uninit-value in mptcp_write_data_fin for a corner
  case now that only a part of the tcp_out_options struct is reset. A
  fix for v7.1.

- Patches 13-14: Two follow-up patches addressing minor comments
  discovered after the human review. A fix for v7.3-rc0.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Gang Yan (1):
      selftests: mptcp: fix an UAF in mptcp_connect.c

Kalpan Jani (1):
      mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

Matthieu Baerts (NGI0) (7):
      mptcp: subflow: no need to copy thmac during ulp_clone
      mptcp: syncookies: remember the request backup flag
      mptcp: options: handle MPC data + csum reqd + no csum
      mptcp: pm: reset retrans_time when ADD_ADDR entry is reused
      selftests: mptcp: lib: dump nstat for the right test
      selftests: mptcp: lib: get counters for the right test
      mptcp: options: fix uninit-value in mptcp_write_data_fin

Paolo Abeni (4):
      mptcp: do not reschedule the RTX timer for fallback sockets
      mptcp: remove unneeded READ_ONCE() annotation
      mptcp: being below memory limit is a likely() condition
      mptcp: avoid pruning for OoW data

Qing Luo (1):
      mptcp: pm: userspace: fix address ID overflow

 net/mptcp/options.c                               |  8 ++++---
 net/mptcp/pm.c                                    |  2 +-
 net/mptcp/pm_kernel.c                             |  8 +++++++
 net/mptcp/pm_userspace.c                          | 18 ++++++++++----
 net/mptcp/protocol.c                              | 29 +++++++++++++----------
 net/mptcp/protocol.h                              |  1 +
 net/mptcp/subflow.c                               |  1 -
 net/mptcp/syncookies.c                            |  5 +++-
 tools/testing/selftests/net/mptcp/mptcp_connect.c |  4 +++-
 tools/testing/selftests/net/mptcp/mptcp_lib.sh    | 26 +++++++++++---------
 10 files changed, 68 insertions(+), 34 deletions(-)
---
base-commit: 7cbfb180945ce529608e4d4e24a6d483699fab1e
change-id: 20260824-net-mptcp-misc-fixes-7-3-rc1-6a4d6223ec41

Best regards,
--  
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH net 01/14] mptcp: do not reschedule the RTX timer for fallback sockets

[Matthieu Baerts (NGI0)]

From: Paolo Abeni <pabeni@redhat.com>

On fallback socket the retrans timer is a quite convoluted no-op, but
currently nothing prevents the MPTCP core to keep rescheduling it.

Additionally gate RTX timer reset to the msk not being fallen back to
TCP yet. To avoid adding multiple tests in fast-path, use a new flags
bit for such condition.

Fixes: b51f9b80c032 ("mptcp: introduce MPTCP retransmission timer")
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 11 ++++++++---
 net/mptcp/protocol.h |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b474d03620a7..23dde0b7144b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -95,6 +95,7 @@ bool __mptcp_try_fallback(struct mptcp_sock *msk, int fb_mib)
 
 	msk->allow_subflows = false;
 	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
+	set_bit(MPTCP_RTX_DISABLED, &msk->flags);
 	__MPTCP_INC_STATS(net, fb_mib);
 	spin_unlock_bh(&msk->fallback_lock);
 	return true;
@@ -1084,13 +1085,14 @@ static bool mptcp_rtx_timer_pending(struct sock *sk)
 
 static void mptcp_reset_rtx_timer(struct sock *sk)
 {
+	struct mptcp_sock *msk = mptcp_sk(sk);
 	unsigned long tout;
 
-	/* prevent rescheduling on close */
-	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
+	/* Prevent rescheduling on close and in case of fallback. */
+	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
 		return;
 
-	tout = mptcp_sk(sk)->timer_ival;
+	tout = msk->timer_ival;
 	sk_reset_timer(sk, &sk->mptcp_retransmit_timer, jiffies + tout);
 }
 
@@ -3324,6 +3326,9 @@ void mptcp_set_state(struct sock *sk, int state)
 		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
 		 */
 		break;
+	case TCP_CLOSE:
+		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
+		fallthrough;
 	default:
 		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
 			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 06a107d4e839..6c00a80344f9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -116,6 +116,7 @@
 #define MPTCP_WORK_RTX		1
 #define MPTCP_FALLBACK_DONE	2
 #define MPTCP_WORK_CLOSE_SUBFLOW 3
+#define MPTCP_RTX_DISABLED	4
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1

-- 
2.53.0

[PATCH net 02/14] mptcp: subflow: no need to copy thmac during ulp_clone

[Matthieu Baerts (NGI0)]

'thmac' is not used after that point.

Indeed, subflow_ulp_clone() is called when the request on the passive
side is over, so when the truncated HMAC is no longer needed.

Note that in case of SYN cookies, thmac will not be initialised. So
better to remove it to avoid a warning from debug tools like KMSAN for
reading uninitialised data.

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Cc: stable@vger.kernel.org
Reviewed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Florian Westphal <fw@strlen.de>
Note: Peter Krystad's email address is bouncing.
---
 net/mptcp/subflow.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index af81ad5e699d..01db7edce18a 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -2084,7 +2084,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->request_bkup = subflow_req->request_bkup;
 		WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
 		new_ctx->token = subflow_req->token;
-		new_ctx->thmac = subflow_req->thmac;
 
 		/* the subflow req id is valid, fetched via subflow_check_req()
 		 * and subflow_token_join_request()

-- 
2.53.0

[PATCH net 03/14] mptcp: syncookies: remember the request backup flag

[Matthieu Baerts (NGI0)]

Instead of using an uninitialised bit when copying the info in
subflow_ulp_clone().

To fix this, no need to extend the join_entry structure: backup is
coming from struct mptcp_subflow_request_sock, only one bit. Do the same
here by using one bit for both.

Fixes: 9466a1ccebbe ("mptcp: enable JOIN requests even if cookies are in use")
Cc: stable@vger.kernel.org
Reviewed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Florian Westphal <fw@strlen.de>
Note: Peter Krystad's email address is bouncing.
---
 net/mptcp/syncookies.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c
index b5cac5701122..9474706641c1 100644
--- a/net/mptcp/syncookies.c
+++ b/net/mptcp/syncookies.c
@@ -26,7 +26,8 @@ struct join_entry {
 	u32 local_nonce;
 	u8 join_id;
 	u8 local_id;
-	u8 backup;
+	u8 backup:1,
+	   request_bkup:1;
 	u8 valid;
 };
 
@@ -63,6 +64,7 @@ static void mptcp_join_store_state(struct join_entry *entry,
 	entry->remote_nonce = subflow_req->remote_nonce;
 	entry->local_nonce = subflow_req->local_nonce;
 	entry->backup = subflow_req->backup;
+	entry->request_bkup = subflow_req->request_bkup;
 	entry->join_id = subflow_req->remote_id;
 	entry->local_id = subflow_req->local_id;
 	entry->valid = 1;
@@ -117,6 +119,7 @@ bool mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subfl
 	subflow_req->remote_nonce = e->remote_nonce;
 	subflow_req->local_nonce = e->local_nonce;
 	subflow_req->backup = e->backup;
+	subflow_req->request_bkup = e->request_bkup;
 	subflow_req->remote_id = e->join_id;
 	subflow_req->local_id = e->local_id;
 	subflow_req->token = e->token;

-- 
2.53.0

[PATCH net 04/14] mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

[Matthieu Baerts (NGI0)]

From: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

The in-kernel MPTCP path manager can leave a stale ADD_ADDR announcement
entry alive when removing the id 0 endpoint. This happens because the id 0
removal path does not tear down pending announcements, unlike the non-zero
id path.

When the PM later reselects id 0 after adding another signal endpoint, it
finds the stale anno_list entry and hits WARN_ON_ONCE(mptcp_pm_is_kernel())
in mptcp_pm_alloc_anno_list().

Root cause: asymmetry between removal paths.
- Non-zero id path: mptcp_nl_remove_subflow_and_signal_addr() calls
  mptcp_pm_remove_anno_addr() to clean up.
- Id 0 path: mptcp_nl_remove_id_zero_address() skips cleanup entirely.

Fix by making the id 0 path symmetric: call mptcp_pm_announced_remove()
and decrement add_addr_signaled before queuing the RM_ADDR.

Subtle detail: signal endpoints are stored in anno_list with port 0, but
msk_local carries the connection's local port. mptcp_pm_announced_remove()
uses use_port=true for comparison, so clear the port before the lookup.

Fixes: 740d798e8767 ("mptcp: remove id 0 address")
Cc: stable@vger.kernel.org
Reported-by: syzbot+55c2a5c871441261ed14@syzkaller.appspotmail.com
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/620
Suggested-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_kernel.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 424f1a7f9248..1a7750813235 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1137,6 +1137,8 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
 		struct sock *sk = (struct sock *)msk;
 		struct mptcp_addr_info msk_local;
+		struct mptcp_addr_info anno_addr;
+		bool announced;
 
 		if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
 			goto next;
@@ -1146,7 +1148,13 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
 			goto next;
 
 		lock_sock(sk);
+		/* Drop a possibly pending ADD_ADDR for this address. */
+		anno_addr = msk_local;
+		anno_addr.port = 0;
+		announced = mptcp_pm_announced_remove(msk, &anno_addr);
 		spin_lock_bh(&msk->pm.lock);
+		if (announced)
+			msk->pm.add_addr_signaled--;
 		mptcp_pm_remove_addr(msk, &list);
 		mptcp_pm_rm_subflow(msk, &list);
 		__mark_subflow_endp_available(msk, 0);

-- 
2.53.0

[PATCH net 05/14] mptcp: options: handle MPC data + csum reqd + no csum

[Matthieu Baerts (NGI0)]

Before this modification, a remote peer could send an MP_CAPABLE with
data, with the checksum flag set, but without adding the actual 2 bytes
of checksum. As a result, uninitialised bytes could be used for the
'csum' field.

That was not a critical issue, because this 'csum' field is only used to
compare with the expected one, if previously negotiated in the 3WHS.
Worst case, the checksum is likely wrong, a fallback is done without a
reject if the negotiation was done earlier. That's OK.

Yet, better to take the expected path with this case: only look at the
checksum flag for MP_CAPABLEs not carrying a data-len.

Such packet can be seen as a 3rd or 4th ACK. The RFC8684 mentions [1]
that the 3rd packet should have the checksum flag set. When an MPC + ACK
contains data, the checksum flag is redundant with the checksum field.
It is not clear what should be done for the 4th ACK, nor if the flag has
to be set if the checksum field is set.

Therefore, it seems fine to only look at the presence of the checksum
field, not to break the interaction with stacks that were not setting
both.

Fixes: 208e8f66926c ("mptcp: receive checksum for MP_CAPABLE with data")
Cc: stable@vger.kernel.org
Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.1-23 [1]
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803-net-mptcp-misc-fixes-7-2-rc6-v2-0-b8f496d71664%40kernel.org?part=1
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index b8318e030138..92f27b9e087a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -93,7 +93,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		 * In other words, the only way for checksums not to be used
 		 * is if both hosts in their SYNs set A=0."
 		 */
-		if (flags & MPTCP_CAP_CHECKSUM_REQD)
+		if ((flags & MPTCP_CAP_CHECKSUM_REQD) &&
+		    opsize < TCPOLEN_MPTCP_MPC_ACK_DATA)
 			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
 
 		mp_opt->deny_join_id0 = !!(flags & MPTCP_CAP_DENY_JOIN_ID0);

-- 
2.53.0

[PATCH net 06/14] selftests: mptcp: fix an UAF in mptcp_connect.c

[Matthieu Baerts (NGI0)]

From: Gang Yan <yangang@kylinos.cn>

At the end of 'sock_connect_mptcp()', it calls 'freeaddrinfo(addr)',
the 'peer' pointer (which points into 'addr') remains. Later, the main
loop uses this peer pointer for reconnection attempts. If the memory has
been freed and reused, the address data could be overwritten, resulting
in an invalid remote address.

This patch keeps the addrinfo list allocated for the whole process
lifetime so "peer" remains valid across reconnects; the memory will be
released at exit() time.

Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
Cc: stable@vger.kernel.org
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index ea4cb6c1bd5e..178d98d91fea 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -381,6 +381,9 @@ static int sock_connect_mptcp(const char * const remoteaddr,
 
 	hints.ai_family = pf;
 
+	/* Keep the resolved address alive for the whole execution: it is
+	 * used again when reconnecting, and will be released at exit time.
+	 */
 	xgetaddrinfo(remoteaddr, port, &hints, &addr);
 	for (a = addr; a; a = a->ai_next) {
 		sock = socket(a->ai_family, a->ai_socktype, proto);
@@ -421,7 +424,6 @@ static int sock_connect_mptcp(const char * const remoteaddr,
 		sock = -1;
 	}
 
-	freeaddrinfo(addr);
 	if (sock != -1)
 		SOCK_TEST_TCPULP(sock, proto);
 	return sock;

-- 
2.53.0

[PATCH net 07/14] mptcp: pm: userspace: fix address ID overflow

[Matthieu Baerts (NGI0)]

From: Qing Luo <luoqing@kylinos.cn>

When all MPTCP address IDs (1-255) are exhausted in the userspace PM,
find_next_zero_bit() returns MPTCP_PM_MAX_ADDR_ID + 1 (256). This value
overflows when stored in the u8 field e->addr.id, resulting in ID 0
being stored and the entry being incorrectly added to the list.

ID 0 is reserved for the initial connection in MPTCP, so this overflow
can cause address conflicts.

Note: the in-kernel PM already has an 'endpoints == MPTCP_PM_MAX_ADDR_ID'
check in mptcp_pm_nl_append_new_local_addr() that returns -ERANGE before
reaching find_next_zero_bit(), preventing this overflow. So this fix only
addresses the userspace PM path.

Check the find_next_zero_bit() result against MPTCP_PM_MAX_ADDR_ID and
return -ENOSPC if all IDs are truly exhausted. Move the ID allocation
check before the memory allocation so that the error path does not need
to free the allocated entry.

Fixes: 4638de5aefe5 ("mptcp: handle local addrs announced by userspace PMs")
Cc: stable@vger.kernel.org
Assisted-by: LLM:0
Signed-off-by: Qing Luo <luoqing@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Kishen Maloor <kishen.maloor@intel.com>
---
 net/mptcp/pm_userspace.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index b94fbb483bf9..fab16d953dbf 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -69,6 +69,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 	}
 
 	if (!addr_match && !id_match) {
+		unsigned int id;
+
+		if (!entry->addr.id && needs_id) {
+			id = find_next_zero_bit(id_bitmap,
+						MPTCP_PM_MAX_ADDR_ID + 1, 1);
+			if (id > MPTCP_PM_MAX_ADDR_ID) {
+				ret = -ENOSPC;
+				goto append_err;
+			}
+		} else {
+			id = entry->addr.id;
+		}
+
 		/* Memory for the entry is allocated from the
 		 * sock option buffer.
 		 */
@@ -78,10 +91,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 			goto append_err;
 		}
 
-		if (!e->addr.id && needs_id)
-			e->addr.id = find_next_zero_bit(id_bitmap,
-							MPTCP_PM_MAX_ADDR_ID + 1,
-							1);
+		e->addr.id = id;
 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
 		msk->pm.local_addr_used++;
 		ret = e->addr.id;

-- 
2.53.0

[PATCH net 08/14] mptcp: pm: reset retrans_time when ADD_ADDR entry is reused

[Matthieu Baerts (NGI0)]

When an ADD_ADDR entry is reused, the timer is re-armed, because the
goal is to re-announce an ADD_ADDR, and eventually retransmit it if
needed.

In this case, the retransmission counter should be reset as well, so the
re-announced address gets its retransmissions back instead of relying on
what was left before, and possibly not being able to retransmit it.

Fixes: 304ab97f4c7c ("mptcp: allow ADD_ADDR reissuance by userspace PMs")
Cc: stable@vger.kernel.org
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803-net-mptcp-misc-fixes-7-2-rc6-v2-0-b8f496d71664%40kernel.org?part=4
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Kishen Maloor <kishen.maloor@intel.com>
---
 net/mptcp/pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 8b68868255c5..b0b71adefb8f 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -462,10 +462,10 @@ bool mptcp_pm_announced_alloc(struct mptcp_sock *msk,
 
 	add_entry->addr = *addr;
 	add_entry->sock = msk;
-	add_entry->retrans_times = 0;
 
 	timer_setup(&add_entry->timer, mptcp_pm_add_addr_timer, 0);
 reset_timer:
+	add_entry->retrans_times = 0;
 	add_entry->timer_done = false;
 	timeout = mptcp_adjust_add_addr_timeout(msk);
 	if (timeout)

-- 
2.53.0

[PATCH net 09/14] mptcp: remove unneeded READ_ONCE() annotation

[Matthieu Baerts (NGI0)]

From: Paolo Abeni <pabeni@redhat.com>

The subflow->fully_established flag is always written under the subflow
socket lock. Reading such value under the same lock does not require any
ONCE annotation.

Fixes: 581c8cbfa934 ("mptcp: annotate data-races around subflow->fully_established")
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/options.c  | 4 ++--
 net/mptcp/protocol.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 92f27b9e087a..196a46e7467d 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -530,7 +530,7 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
 		return false;
 
 	/* MPC/MPJ needed only on 3rd ack packet, DATA_FIN and TCP shutdown take precedence */
-	if (READ_ONCE(subflow->fully_established) || snd_data_fin_enable ||
+	if (subflow->fully_established || snd_data_fin_enable ||
 	    subflow->snd_isn != TCP_SKB_CB(skb)->seq ||
 	    sk->sk_state != TCP_ESTABLISHED)
 		return false;
@@ -981,7 +981,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 	/* here we can process OoO, in-window pkts, only in-sequence 4th ack
 	 * will make the subflow fully established
 	 */
-	if (likely(READ_ONCE(subflow->fully_established))) {
+	if (likely(subflow->fully_established)) {
 		/* on passive sockets, check for 3rd ack retransmission
 		 * note that msk is always set by subflow_syn_recv_sock()
 		 * for mp_join subflows
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 23dde0b7144b..c7c8bb3b3e00 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3881,7 +3881,7 @@ static void schedule_3rdack_retransmission(struct sock *ssk)
 	struct tcp_sock *tp = tcp_sk(ssk);
 	unsigned long timeout;
 
-	if (READ_ONCE(mptcp_subflow_ctx(ssk)->fully_established))
+	if (mptcp_subflow_ctx(ssk)->fully_established)
 		return;
 
 	/* reschedule with a timeout above RTT, as we must look only for drop */

-- 
2.53.0

[PATCH net 10/14] selftests: mptcp: lib: dump nstat for the right test

[Matthieu Baerts (NGI0)]

In case of errors, mptcp_lib_pr_nstat is called to dump the nstat
counters, but for some tests, it was dumping the counters for all
subtests, not just the current one.

That's an issue for tests that don't recreate the netns for each
subtest, e.g. mptcp_connect.sh. In this case, 'nstat -a' will look at
the absolute counters since the creation of the netns, making
debugging harder.

Instead, it should dump the counters for the current test, by using the
history recorded in /tmp/<ns>.nstat if available, and not using '-a'
which was dumping the absolute values instead of calculating increments.

While at it, rename the previous 'hist' variable to 'cache' as it was
used to look at the cache, not the nstat history.

Fixes: 658e53141780 ("selftests: mptcp: join: dump stats from history")
Cc: stable@vger.kernel.org
Reviewed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 5ef6033775c8..da1da414c30f 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -108,12 +108,14 @@ mptcp_lib_pr_info() {
 
 mptcp_lib_pr_nstat() {
 	local ns="${1}"
-	local hist="/tmp/${ns}.out"
+	local cache="/tmp/${ns}.out"
+	local hist="/tmp/${ns}.nstat"
 
-	if [ -f "${hist}" ]; then
-		awk '$2 != 0 { print "  "$0 }' "${hist}"
+	if [ -f "${cache}" ]; then
+		awk '$2 != 0 { print "  "$0 }' "${cache}"
 	else
-		ip netns exec "${ns}" nstat -as | grep Tcp
+		NSTAT_HISTORY="${hist}" ip netns exec "${ns}" nstat -s |
+			grep Tcp
 	fi
 }
 

-- 
2.53.0

[PATCH net 11/14] selftests: mptcp: lib: get counters for the right test

[Matthieu Baerts (NGI0)]

When the value for a MIB counter is required, mptcp_lib_get_counter is
called. It tries to use the cache, if available. If not it falls back to
calling 'nstat' directly by looking at the absolute counters.

That's an issue for tests that don't recreate the netns for each
subtest. In this case, 'nstat -a' will look at the counters for the
netns.

Instead, it should look at the increment for the current test, by using
the history recorded in /tmp/<ns>.nstat, if available, and not using
'-a' which was dumping the absolute values.

While at it, rename the previous 'hist' variable to 'cache' as it was
used to look at the cache, not the nstat history.

Fixes: 71388a9f331d ("selftests: mptcp: lib: get counters from nstat history")
Cc: stable@vger.kernel.org
Reviewed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index da1da414c30f..b9d14647f401 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -416,19 +416,21 @@ mptcp_lib_nstat_get() {
 }
 
 # $1: ns, $2: MIB counter
-# Get the counter from the history (mptcp_lib_nstat_{init,get}()) if available.
-# If not, get the counter from nstat ignoring any history.
+# Get the counter from the cache (mptcp_lib_nstat_{init,get}()) if available.
+# If not, get the counter from nstat ignoring any cache, but using the history.
 mptcp_lib_get_counter() {
 	local ns="${1}"
 	local counter="${2}"
-	local hist="/tmp/${ns}.out"
+	local cache="/tmp/${ns}.out"
+	local hist="/tmp/${ns}.nstat"
 	local count
 
-	if [[ -s "${hist}" && "${counter}" == *"Tcp"* ]]; then
-		count=$(awk "/^${counter} / {print \$2; exit}" "${hist}")
+	if [[ -s "${cache}" && "${counter}" == *"Tcp"* ]]; then
+		count=$(awk "/^${counter} / {print \$2; exit}" "${cache}")
 	else
-		count=$(ip netns exec "${ns}" nstat -asz "${counter}" |
-			awk 'NR==1 {next} {print $2}')
+		count=$(NSTAT_HISTORY="${hist}" ip netns exec "${ns}" \
+			nstat -sz "${counter}" |
+				awk 'NR==1 {next} {print $2}')
 	fi
 	if [ -z "${count}" ]; then
 		mptcp_lib_fail_if_expected_feature "${counter} counter"

-- 
2.53.0

[PATCH net 12/14] mptcp: options: fix uninit-value in mptcp_write_data_fin

[Matthieu Baerts (NGI0)]

When sending a DATA_FIN without data, and because the DATA_FIN occupies
1 octet of the connection-level sequence space [1], it is then required
to add a DSS mapping with specific values.

If the checksum has been negotiated, it also needs to be computed, and
included in the outgoing packet, and thus the initial csum data needs to
be reset to 0 as well. This is no longer the case since commit
cfcceb7a39fc ("tcp: shrink per-packet memset in __tcp_transmit_skb()"),
because the whole ext_copy structure is no longer zeroed by default.

This seems to be the only case where use_map is changed and set
afterwards, so initialising the csum field only in this case, along with
other fields for this specific case.

Fixes: cfcceb7a39fc ("tcp: shrink per-packet memset in __tcp_transmit_skb()")
Cc: stable@vger.kernel.org
Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.3.3 [1]
Link: https://sashiko.dev/#/patchset/20260812-net-next-mptcp-misc-feat-7-3-v1-0-1905a818f6cb%40kernel.org?part=2
Reviewed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Cc: Kuniyuki Iwashima <kuniyu@google.com>
Cc: Keita Morisaki <kmta1236@gmail.com>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/mptcp/options.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 196a46e7467d..ce0de02f5a3a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -612,6 +612,7 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
 		ext->data_seq = data_fin_tx_seq;
 		ext->subflow_seq = 0;
 		ext->data_len = 1;
+		ext->csum = 0;
 	} else if (ext->data_seq + ext->data_len == data_fin_tx_seq) {
 		/* If there's an existing DSS mapping and it is the
 		 * final mapping, DATA_FIN consumes 1 additional byte of

-- 
2.53.0

[PATCH net 13/14] mptcp: being below memory limit is a likely() condition

[Matthieu Baerts (NGI0)]

From: Paolo Abeni <pabeni@redhat.com>

The current compiler hint annotation is wrong, due to inverted
logic in the previous revision of the relevant code.

Fixes: e468d371180d ("mptcp: implemented OoO queue pruning")
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c7c8bb3b3e00..be59ffa4bd31 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -289,8 +289,8 @@ static void mptcp_prune_ofo_queue(struct sock *sk,
  */
 static bool mptcp_can_ingest(const struct sock *sk)
 {
-	return unlikely(sk_rmem_alloc_get(sk) <= READ_ONCE(sk->sk_rcvbuf)) ||
-			__mptcp_check_fallback(mptcp_sk(sk));
+	return likely(sk_rmem_alloc_get(sk) <= READ_ONCE(sk->sk_rcvbuf)) ||
+	       __mptcp_check_fallback(mptcp_sk(sk));
 }
 
 static bool mptcp_try_rmem_schedule(struct sock *sk, const struct sk_buff *skb)

-- 
2.53.0

[PATCH net 14/14] mptcp: avoid pruning for OoW data

[Matthieu Baerts (NGI0)]

From: Paolo Abeni <pabeni@redhat.com>

Pruning is expansive and destructive, do it only when we expect
to accept the skb triggering the cleanup.

Fixes: e468d371180d ("mptcp: implemented OoO queue pruning")
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index be59ffa4bd31..b26a8d560612 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -313,12 +313,6 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 	u64 seq, end_seq, max_seq;
 	struct sk_buff *skb1;
 
-	if (!mptcp_try_rmem_schedule(sk, skb)) {
-		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
-		mptcp_drop(sk, skb);
-		return;
-	}
-
 	seq = MPTCP_SKB_CB(skb)->map_seq;
 	end_seq = MPTCP_SKB_CB(skb)->end_seq;
 	max_seq = atomic64_read(&msk->rcv_wnd_sent);
@@ -335,6 +329,12 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 		return;
 	}
 
+	if (!mptcp_try_rmem_schedule(sk, skb)) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
+		mptcp_drop(sk, skb);
+		return;
+	}
+
 	p = &msk->out_of_order_queue.rb_node;
 	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUE);
 	if (RB_EMPTY_ROOT(&msk->out_of_order_queue)) {

-- 
2.53.0

Re: [PATCH net 01/14] mptcp: do not reschedule the RTX timer for fallback sockets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] MPTCP_RTX_DISABLED is set on TCP_CLOSE but never cleared if a connection fails synchronously, permanently disabling retransmissions if the socket is retried.
--

--- Patch [1]: [PATCH net 01/14] mptcp: do not reschedule the RTX timer for fallback sockets ---
commit e15c87662e83ee7836d581b8ebc2d5e99a964d98
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: do not reschedule the RTX timer for fallback sockets

This commit stops the MPTCP core from rescheduling the retransmission timer
for fallback sockets. It uses a new flag bit MPTCP_RTX_DISABLED to prevent
rescheduling during fallback and when transitioning to TCP_CLOSE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b474d03620a75..23dde0b7144bb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -3324,6 +3326,9 @@ void mptcp_set_state(struct sock *sk, int state)
>  		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
>  		 */
>  		break;
> +	case TCP_CLOSE:
> +		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
> +		fallthrough;

[Severity: High]
Does this permanently disable retransmissions if a socket is reused after a
failed connect?

If an application calls connect() and it fails synchronously (for example,
connection refused), mptcp_connect() handles the error by calling
mptcp_set_state(sk, TCP_CLOSE). This sets the MPTCP_RTX_DISABLED flag.

If the application then retries connect() on the same socket, mptcp_connect()
transitions the socket to TCP_SYN_SENT but never clears the sticky
MPTCP_RTX_DISABLED flag.

Because the flag remains set, mptcp_reset_rtx_timer() will permanently abort
arming the retransmission timer for this socket, which could cause the
reused connection to hang indefinitely upon packet loss.

>  	default:
>  		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
>  			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-net-mptcp-misc-fixes-7-3-rc1-v1-0-a92309d121ca@kernel.org?part=1

Re: [PATCH net 07/14] mptcp: pm: userspace: fix address ID overflow

[sashiko-bot]

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

Pre-existing issues:
- [High] Spurious failure in mptcp_userspace_pm_append_new_local_addr when allocating a new ID if an address with ID 0 exists.
--

--- Patch [7]: [PATCH net 07/14] mptcp: pm: userspace: fix address ID overflow ---
commit 88d3db755130c825590998e405b9b270ff919205
Author: Qing Luo <luoqing@kylinos.cn>

mptcp: pm: userspace: fix address ID overflow

This commit fixes an overflow issue in the userspace PM when all MPTCP address
IDs (1-255) are exhausted. It adds a check against MPTCP_PM_MAX_ADDR_ID after
calling find_next_zero_bit(), returning -ENOSPC to prevent storing an
incorrect ID 0.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index b94fbb483bf94..fab16d953dbfe 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -69,6 +69,19 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  	}

[Severity: High]
This is a pre-existing issue, but does the loop just above here falsely
trigger a match if both IDs are 0?

In the preceding loop, we have:

	id_match = (e->addr.id == entry->addr.id);
	if (addr_match || id_match)
		break;

If needs_id is true, entry->addr.id is initially 0 as a placeholder. If an
existing address in userspace_pm_local_addr_list was previously added without
an ID attribute, it might have id == 0.

Could this cause id_match to evaluate to true (0 == 0), breaking the loop
prematurely and leading to a spurious -EINVAL failure? This might completely
break dynamic ID allocation and permanently reject new incoming or outgoing
subflow JOINs that require an ID.

>  	if (!addr_match && !id_match) {
> +		unsigned int id;
> +
> +		if (!entry->addr.id && needs_id) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-net-mptcp-misc-fixes-7-3-rc1-v1-0-a92309d121ca@kernel.org?part=7

Re: [PATCH net 04/14] mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

[sashiko-bot]

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

New issues:
- [High] Hardcoded `anno_addr.port = 0` fails to cancel the pending `ADD_ADDR` if the ID 0 endpoint was configured with a specific port.

Pre-existing issues:
- [High] Canceling an `ADD_ADDR` fails to clear `MPTCP_ADD_ADDR_SIGNAL`, silently dropping `RM_ADDR` and erroneously transmitting a canceled `ADD_ADDR`.
- [High] Calling `sk_stop_timer_sync()` while holding `rcu_read_lock()` triggers a sleep-in-atomic bug on PREEMPT_RT kernels.
--

--- Patch [4]: [PATCH net 04/14] mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0 ---
commit 8ec4a41a6bf3f96bdf00a1088430d4fcabb37f97
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

The in-kernel MPTCP path manager can leave a stale ADD_ADDR announcement
entry alive when removing the id 0 endpoint. This happens because the id 0
removal path does not tear down pending announcements, unlike the non-zero
id path.

Cc: stable@vger.kernel.org
Reported-by: syzbot+55c2a5c871441261ed14@syzkaller.appspotmail.com
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/620
Suggested-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 424f1a7f92482..1a77508132354 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1146,7 +1148,13 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
>  			goto next;
>  
>  		lock_sock(sk);
> +		/* Drop a possibly pending ADD_ADDR for this address. */
> +		anno_addr = msk_local;
> +		anno_addr.port = 0;

[Severity: High]
Does hardcoding the port to 0 here prevent mptcp_pm_announced_remove() from
finding the pending announcement if the ID 0 endpoint was configured with a
specific port?

If a signal endpoint matches the initial subflow's IP but has a non-zero
port, the PM maps it to ID 0 and stores it in anno_list with the non-zero
port. Because mptcp_pm_announced_remove() enforces strict port matching,
it seems it would fail to find the announcement, leaving a stale ADD_ADDR
timer that could trigger a WARN_ON_ONCE.

> +		announced = mptcp_pm_announced_remove(msk, &anno_addr);

[Severity: High]
This is a pre-existing issue, but does calling mptcp_pm_announced_remove()
trigger a sleep-in-atomic bug on PREEMPT_RT kernels?

In net/mptcp/pm.c:mptcp_pm_announced_del_timer(), which is called by
mptcp_pm_announced_remove(), there is a call to sk_stop_timer_sync()
inside an RCU read-side critical section:

mptcp_pm_announced_del_timer()
    rcu_read_lock();
    ...
    sk_stop_timer_sync(sk, &entry->timer);

On PREEMPT_RT, timer_delete_sync() can sleep, which is not allowed inside
an RCU critical section.

>  		spin_lock_bh(&msk->pm.lock);
> +		if (announced)
> +			msk->pm.add_addr_signaled--;
>  		mptcp_pm_remove_addr(msk, &list);

[Severity: High]
This is also a pre-existing issue, but does canceling an ADD_ADDR fail to
clear the signal bit, causing the RM_ADDR to be dropped here?

When mptcp_pm_announced_del_timer() deletes the timer and removes the entry,
it does not clear the corresponding BIT(MPTCP_ADD_ADDR_SIGNAL) in
msk->pm.addr_signal. 

Then, when mptcp_pm_remove_addr() is called, it checks if rm_addr is
non-zero (which it is, because the signal bit is still set):

mptcp_pm_remove_addr()
    if (rm_addr) {
        MPTCP_ADD_STATS(...);
        return -EINVAL;
    }

Would this cause the RM_ADDR request to be silently dropped, while
erroneously transmitting the canceled ADD_ADDR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-net-mptcp-misc-fixes-7-3-rc1-v1-0-a92309d121ca@kernel.org?part=4

Re: [PATCH net 01/14] mptcp: do not reschedule the RTX timer for fallback sockets

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
mptcp: do not reschedule the RTX timer for fallback sockets

This replaces the TCP_CLOSE state test in mptcp_reset_rtx_timer() with a
new MPTCP_RTX_DISABLED bit in msk->flags, set both on fallback in
__mptcp_try_fallback() and on the transition to TCP_CLOSE in
mptcp_set_state().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b474d03620a75..23dde0b7144bb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -1084,13 +1085,14 @@ static bool mptcp_rtx_timer_pending(struct sock *sk)
>  
>  static void mptcp_reset_rtx_timer(struct sock *sk)
>  {
> +	struct mptcp_sock *msk = mptcp_sk(sk);
>  	unsigned long tout;
>  
> -	/* prevent rescheduling on close */
> -	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
> +	/* Prevent rescheduling on close and in case of fallback. */
> +	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
>  		return;

[Severity: Medium]
The new test depends on a flag that mptcp_disconnect() clears while the
socket is still in TCP_CLOSE, and after mptcp_stop_rtx_timer() already ran.
The order inside mptcp_disconnect() is:

	mptcp_set_state(sk, TCP_CLOSE);    /* sets MPTCP_RTX_DISABLED */
	...
	mptcp_stop_rtx_timer(sk);          /* non-sync, timer_ival = 0 */
	...
	WRITE_ONCE(msk->flags, 0);         /* clears MPTCP_RTX_DISABLED */
	msk->cb_flags = 0;

Can the rtx timer be armed again after that point?

mptcp_stop_rtx_timer() uses sk_stop_timer() and therefore the non-sync
timer_delete(), so an already-expired mptcp_retransmit_timer() can still be
running on another CPU. It only takes bh_lock_sock(), and its
__set_bit(MPTCP_RETRANSMIT, &msk->cb_flags) can land after the plain
msk->cb_flags = 0 store above.

On the release_sock() that follows, mptcp_release_cb() calls
__mptcp_retrans(sk) with no sk_state check, and that reaches
__mptcp_clean_una():

out:
	if (snd_una == msk->snd_nxt && snd_una == msk->write_seq) {
		if (mptcp_rtx_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
			mptcp_stop_rtx_timer(sk);
	} else {
		mptcp_reset_rtx_timer(sk);
	}

mptcp_disconnect() does not reset snd_una/snd_nxt/write_seq, so for a socket
disconnected with unacked data the else branch runs and arms the timer with
timer_ival == 0 on a TCP_CLOSE socket whose timers were deliberately
stopped. The previous inet_sk_state_load(sk) == TCP_CLOSE test returned
early here.

When that immediately-expiring timer fires, mptcp_retransmit_timer() does
test_and_set_bit(MPTCP_WORK_RTX, &msk->flags) and then calls
mptcp_schedule_work(), which refuses to queue the worker for a closed
socket without clearing the bit:

	if (inet_sk_state_load(sk) == TCP_CLOSE)
		return false;

If the application then reuses the socket with a new connect(), does that
leftover MPTCP_WORK_RTX bit keep mptcp_worker() from being scheduled on
subsequent rtx expiries, until some other mptcp_schedule_work() caller lets
the worker clear it?

>  
> -	tout = mptcp_sk(sk)->timer_ival;
> +	tout = msk->timer_ival;
>  	sk_reset_timer(sk, &sk->mptcp_retransmit_timer, jiffies + tout);
>  }
>  
> @@ -3324,6 +3326,9 @@ void mptcp_set_state(struct sock *sk, int state)
>  		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
>  		 */
>  		break;
> +	case TCP_CLOSE:
> +		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
> +		fallthrough;

[Severity: High]
This latches the bit on every transition to TCP_CLOSE, and there is no
clear_bit(MPTCP_RTX_DISABLED) anywhere in the tree. The only site that
resets the word is mptcp_disconnect():

	WRITE_ONCE(msk->flags, 0);

Can this leave the bit set on a socket that afterwards carries a normal,
non-fallback MPTCP connection?

First path, a failed connect() followed by a retry on the same fd. The
error path of mptcp_connect() does:

	if (unlikely(err)) {
		/* avoid leaving a dangling token in an unconnected socket */
		mptcp_token_destroy(msk);
		mptcp_set_state(sk, TCP_CLOSE);
		return err;
	}

For a negative return from sk->sk_prot->connect(), __inet_stream_connect()
takes:

		err = sk->sk_prot->connect(sk, uaddr, addr_len);
		if (err < 0)
			goto out;

so it never reaches the sock_error: label that calls ->disconnect(), and
mptcp_disconnect() does not run. The socket stays SS_UNCONNECTED with
sk_state TCP_CLOSE, which is exactly what __inet_stream_connect() requires
to accept a second connect(), and __mptcp_nmpc_sk() also permits
TCPF_CLOSE. The later mptcp_set_state(sk, TCP_SYN_SENT) and
mptcp_set_state(sk, TCP_ESTABLISHED) do not clear the bit.

Second path, a failed listen() and inheritance by accepted sockets.
mptcp_listen() unconditionally does:

	err = __inet_listen_sk(ssk, backlog);
	release_sock(ssk);
	mptcp_set_state(sk, inet_sk_state_load(ssk));

which is TCP_CLOSE on failure, so the bit is latched on the listener; a
later successful listen() only sets TCP_LISTEN. mptcp_sk_clone_init() then
creates accepted sockets with sk_clone_lock(), which copies the whole
struct mptcp_sock, msk->flags included, and __mptcp_init_sock() re-inits
the lists, timers and allow_* fields but not msk->flags.

With the bit stuck, mptcp_reset_rtx_timer() is a permanent no-op.
MPTCP_WORK_RTX and MPTCP_RETRANSMIT are set only by
mptcp_retransmit_timer(), so does that mean __mptcp_retrans() can never run
for such a socket, leaving data reinjected into msk->rtx_queue when a
subflow stalls or dies unretransmitted, and DATA_FIN retransmission
disabled, while the write side keeps its sndbuf pinned?

The previous guard read the live socket state, so it stopped applying on the
next state transition. Would clearing the bit in __mptcp_init_sock(), and on
the connect()/listen() retry paths, restore that property?

>  	default:
>  		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
>  			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);

[ ... ]

Re: [PATCH net 02/14] mptcp: subflow: no need to copy thmac during ulp_clone

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
mptcp: subflow: no need to copy thmac during ulp_clone

This drops the copy of subflow_req->thmac into the new subflow context in
subflow_ulp_clone(), since the truncated HMAC is no longer needed once the
passive-side request is over.  The commit message notes that thmac is left
uninitialised on the SYN cookie path, so removing the copy also avoids a
KMSAN uninitialised-read report.

(no Link: tags in the commit header)

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index af81ad5e699d1..01db7edce18a6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -2084,7 +2084,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
>  		new_ctx->request_bkup = subflow_req->request_bkup;
              ^^^^

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the
neighbouring store: does request_bkup have exactly the same
uninitialised-on-SYN-cookies problem that motivated removing the thmac
copy here?

subflow_init_req() only clears a subset of the fields:

net/mptcp/subflow.c:subflow_init_req() {
	subflow_req->mp_capable = 0;
	subflow_req->mp_join = 0;
	subflow_req->csum_reqd = mptcp_is_checksum_enabled(sock_net(sk_listener));
	subflow_req->allow_join_id0 = mptcp_allow_join_id0(sock_net(sk_listener));
	subflow_req->msk = NULL;
	mptcp_token_init_request(req);
}

and the only assignment of request_bkup for a request sock happens on the
SYN, in subflow_token_join_request():

	subflow_req->local_id = local_id;
	subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);

On the cookie ACK path that function is not called, and the state restore
does not cover request_bkup either:

net/mptcp/syncookies.c:mptcp_token_join_cookie_init_state() {
	subflow_req->remote_nonce = e->remote_nonce;
	subflow_req->local_nonce = e->local_nonce;
	subflow_req->backup = e->backup;
	subflow_req->remote_id = e->join_id;
	subflow_req->local_id = e->local_id;
	subflow_req->token = e->token;
	subflow_req->msk = msk;
}

struct join_entry has no request_bkup member at this commit, so the bit
carries whatever was in the freshly allocated request sock.

Unlike thmac, the copied value is consumed afterwards, for instance in
mptcp_subflow_get_send() and mptcp_subflow_get_retrans():

net/mptcp/protocol.c:mptcp_subflow_get_send() {
	bool backup = subflow->backup || subflow->request_bkup;

It is also put on the wire by mptcp_established_options_mp_prio() and
exported to userspace by the mptcp diag code.

So when a remote MP_JOIN is accepted while tcp_syncookies is active, can a
joined subflow end up randomly flagged as backup and reported that way in
diag, along with the same class of KMSAN uninit-value branch report that
this patch removes for thmac?

For the record, the next patch in this series, "mptcp: syncookies: remember
the request backup flag", adds request_bkup:1 to struct join_entry and
stores/restores it, and the store happens after subflow_token_join_request()
has set the field (subflow_token_join_request() runs before
subflow_init_req_cookie_join_save() in subflow_check_req()), so the field is
properly initialised at the end of the series.  Would it be worth mentioning
that dependency here, given this patch carries a Cc: stable tag?

>  		WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
>  		new_ctx->token = subflow_req->token;
> -		new_ctx->thmac = subflow_req->thmac;
>  
>  		/* the subflow req id is valid, fetched via subflow_check_req()
>  		 * and subflow_token_join_request()

Re: [PATCH net 04/14] mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

This makes the in-kernel path manager's id 0 removal path tear down a
pending ADD_ADDR announcement, matching what the non-zero id path already
does, and decrements pm.add_addr_signaled when an entry was removed.
The lookup key is built from the msk local address with the port forced to
zero.

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

A few questions on the changelog and on the new lookup key.

> When the PM later reselects id 0 after adding another signal endpoint, it
> finds the stale anno_list entry and hits WARN_ON_ONCE(mptcp_pm_is_kernel())
> in mptcp_pm_alloc_anno_list().
>
> Root cause: asymmetry between removal paths.
> - Non-zero id path: mptcp_nl_remove_subflow_and_signal_addr() calls
>   mptcp_pm_remove_anno_addr() to clean up.

[Severity: Low]
Do these two helper names exist in this tree?  Grepping for
mptcp_pm_alloc_anno_list and mptcp_pm_remove_anno_addr at this commit
returns no matches.

The WARN_ON_ONCE(mptcp_pm_is_kernel(msk)) described here lives in
mptcp_pm_announced_alloc() in net/mptcp/pm.c, and the non-zero id path
calls mptcp_pm_remove_announced() from
mptcp_nl_remove_subflow_and_signal_addr() in net/mptcp/pm_kernel.c.

The mechanism described looks right, so should the names be updated to the
current spelling?

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 424f1a7f92482..1a77508132354 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c

[ ... ]

> @@ -1146,7 +1148,13 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
>  			goto next;
>  
>  		lock_sock(sk);
> +		/* Drop a possibly pending ADD_ADDR for this address. */
> +		anno_addr = msk_local;
> +		anno_addr.port = 0;
> +		announced = mptcp_pm_announced_remove(msk, &anno_addr);

[Severity: Medium]
This isn't a bug introduced by this patch, since before it the id 0 path
removed no announcement at all, but does forcing the port to zero here miss
signal endpoints that carry a port?

The commit message says:

	Subtle detail: signal endpoints are stored in anno_list with port 0

Is that always true?  mptcp_pm_nl_add_addr_doit() explicitly accepts a
non-zero port for signal-only endpoints:

	if (addr.addr.port && !address_use_port(&addr)) {
		NL_SET_ERR_MSG_ATTR(info->extack, attr,
				    "flags must have signal and not subflow when using port");

and mptcp_pm_nl_append_new_local_addr() only zeroes the port when
!address_use_port(), so an endpoint A:P can end up announced as A:P in
anno_list.  mptcp_pm_announced_lookup() always compares the port:

net/mptcp/pm.c:mptcp_pm_announced_lookup() {
	list_for_each_entry(entry, &msk->pm.anno_list, list) {
		if (mptcp_addresses_equal(&entry->addr, addr, true))
			return entry;
}

For an msk accepted on an MPTCP listener bound to A:P, msk_local is A:P and
mptcp_mpc_endpoint_setup() -> __lookup_addr() can match that endpoint
(use_port = entry->addr.port), so mpc_endpoint_id becomes its id and the
announcement is stored with port P.

In that case the port-zeroed key never matches, announced stays false and
the entry stays linked, while __mark_subflow_endp_available(msk, 0) still
re-arms the mpc_endpoint_id bit.  On the next pass select_signal_address()
reselects that endpoint (port-carrying endpoints are put at the head of
endp_list by mptcp_pm_nl_append_new_local_addr()), and
mptcp_pm_announced_alloc() finds the surviving entry:

net/mptcp/pm.c:mptcp_pm_announced_alloc() {
	add_entry = mptcp_pm_announced_lookup(msk, addr);
	if (add_entry) {
		if (WARN_ON_ONCE(mptcp_pm_is_kernel(msk)))
			return false;

Is that the same splat the patch is meant to remove, and would a
port-insensitive lookup (or a second lookup with the original port) close
this case too?

>  		spin_lock_bh(&msk->pm.lock);
> +		if (announced)
> +			msk->pm.add_addr_signaled--;
>  		mptcp_pm_remove_addr(msk, &list);

[Severity: Medium]
Can the entry freed here belong to a different id than the RM_ADDR that is
queued right after?

mptcp_pm_announced_remove() passes check_id == false, so
mptcp_pm_announced_del_timer() unlinks whatever address matches, regardless
of the announced id:

net/mptcp/pm.c:mptcp_pm_announced_del_timer() {
	entry = mptcp_pm_announced_lookup(msk, addr);
	if (entry && (!check_id || entry->addr.id == addr->id)) {
	...
	if (!check_id && entry)
		list_del(&entry->list);
}

while mptcp_nl_remove_id_zero_address() always queues id 0:

	list.ids[list.nr++] = 0;

With a signal endpoint A:P (id M) matched as mpc_endpoint_id, and a second
signal endpoint A with port 0 (id N), mptcp_pm_create_subflow_or_signal_addr()
keeps id N because endp_id != mpc_endpoint_id:

	endp_id = local.addr.id;
	if (endp_id == msk->mpc_endpoint_id)
		local.addr.id = 0;

so anno_list holds {A, port 0, id N}.  A following del id 0 A matches this
msk and the port-zeroed, id-agnostic lookup frees that id N entry and
decrements add_addr_signaled for it, while the peer is only told id 0 was
removed.

If endpoint N is deleted later at a moment when no subflow in conn_list has
saddr A, mptcp_pm_remove_announced() sees announced == false and force ==
false, so no RM_ADDR is sent:

	announced = mptcp_pm_announced_remove(msk, addr);
	if (announced || force) {

Does the peer then keep advertising that address indefinitely?  The commit
message describes the change only as making the id 0 path symmetric and does
not mention this cross-id effect.

>  		mptcp_pm_rm_subflow(msk, &list);
>  		__mark_subflow_endp_available(msk, 0);

Re: [PATCH net 05/14] mptcp: options: handle MPC data + csum reqd + no csum

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
mptcp: options: handle MPC data + csum reqd + no csum

The parser in mptcp_parse_option() no longer sets OPTION_MPTCP_CSUMREQD from
the MP_CAPABLE 'A' flag when the option carries a data-len, so only the
presence of the 2-byte checksum field marks a data-bearing MP_CAPABLE as
checksummed.  This avoids using the uninitialised 'csum' field when a peer
sets the flag but omits the checksum bytes.

Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.1-23

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index b8318e0301389..92f27b9e087a6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -93,7 +93,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  		 * In other words, the only way for checksums not to be used
>  		 * is if both hosts in their SYNs set A=0."
>  		 */
> -		if (flags & MPTCP_CAP_CHECKSUM_REQD)
> +		if ((flags & MPTCP_CAP_CHECKSUM_REQD) &&
> +		    opsize < TCPOLEN_MPTCP_MPC_ACK_DATA)
>  			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;

[Severity: Medium]
On the passive side, is the third ACK's 'A' bit not the only place where the
peer's checksum requirement is recorded?

subflow_init_req() derives the request state purely from the local sysctl:

net/mptcp/subflow.c:subflow_init_req() {
	...
	subflow_req->csum_reqd = mptcp_is_checksum_enabled(sock_net(sk_listener));
	...
}

There is only that single store of csum_reqd in net/mptcp/subflow.c, so the
'A' bit seen in the remote SYN is not saved anywhere.  The only server-side
promotion into connection state is in mptcp_sk_clone_init():

net/mptcp/protocol.c:mptcp_sk_clone_init() {
	...
	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
		WRITE_ONCE(msk->csum_enabled, true);
	...
}

With net.mptcp.checksum_enabled=0 (the default in net/mptcp/ctrl.c) and a
peer whose SYN had A=1, does the new opsize test mean msk->csum_enabled now
stays false for a 22-byte MP_CAPABLE + ACK that carries a data-len, sets A=1
and omits the checksum field?  That option length is still accepted by the
size check just above:

		if (opsize != expected_opsize &&
		    (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA ||
		     opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM))
			break;

Downstream, mptcp_incoming_options() then sets mpext->csum_reqd = 0, and
get_mapping_status() compares:

net/mptcp/subflow.c:get_mapping_status() {
	...
	/* Cfr RFC 8684 Section 3.3.0 */
	if (unlikely(subflow->map_csum_reqd != csum_reqd))
		return MAPPING_INVALID;
	...
}

Both sides being false, the mapping is accepted rather than rejected, so no
RST is sent for the non-conforming peer and the connection runs without
checksums even though the peer asked for them.  Our own writer still emits
the bit on data-carrying MPC + ACKs:

net/mptcp/options.c:mptcp_write_options() {
	...
	if (opts->csum_reqd)
		flag |= MPTCP_CAP_CHECKSUM_REQD;
	...
}

Related to this, the changelog says the csum field "is only used to compare
with the expected one, if previously negotiated in the 3WHS".  Since the
passive side never records the peer's SYN 'A' bit, does that statement hold
for a listener?

[Severity: Low]
This isn't an issue introduced by this patch, but while touching this area:
the trailing pr_debug() in mptcp_parse_option() prints fields that are only
conditionally assigned:

		pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u\n",
			 version, flags, opsize, mp_opt->sndr_key,
			 mp_opt->rcvr_key, mp_opt->data_len, mp_opt->csum);

sndr_key is only set for opsize >= TCPOLEN_MPTCP_MPC_SYNACK, rcvr_key for
opsize >= TCPOLEN_MPTCP_MPC_ACK, data_len for opsize >=
TCPOLEN_MPTCP_MPC_ACK_DATA and csum only for opsize ==
TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM.

mptcp_get_options() only clears the status group:

net/mptcp/options.c:mptcp_get_options() {
	...
	*(u32 *)&mp_opt->status = 0;
	...
}

and struct mptcp_options_received in net/mptcp/protocol.h places sndr_key,
rcvr_key, data_len and csum before struct_group(status, ...), while callers
such as mptcp_incoming_options(), subflow_check_req() and
mptcp_subflow_init_cookie_req() declare the struct on the stack without a
memset.

When the dynamic-debug callsite is enabled, does this print stale caller
stack bytes for MP_CAPABLE SYN, SYN/ACK, plain MPC + ACK, and the
data-without-checksum case this patch targets?  Would initialising csum and
data_len (or extending the zeroed region) make the stated goal of the patch
hold regardless of the debug setting, including for the stable backport?

Re: [PATCH net 08/14] mptcp: pm: reset retrans_time when ADD_ADDR entry is reused

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
mptcp: pm: reset retrans_time when ADD_ADDR entry is reused

This moves the retrans_times reset from the fresh-allocation path into the
shared reset_timer block of mptcp_pm_announced_alloc(), so a reused
ADD_ADDR entry gets its retransmission budget back when the timer is
re-armed.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803-net-mptcp-misc-fixes-7-2-rc6-v2-0-b8f496d71664%40kernel.org?part=4

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 8b68868255c50..b0b71adefb8fe 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -462,10 +462,10 @@ bool mptcp_pm_announced_alloc(struct mptcp_sock *msk,
>  
>  	add_entry->addr = *addr;
>  	add_entry->sock = msk;
> -	add_entry->retrans_times = 0;
>  
>  	timer_setup(&add_entry->timer, mptcp_pm_add_addr_timer, 0);
>  reset_timer:
> +	add_entry->retrans_times = 0;
>  	add_entry->timer_done = false;
>  	timeout = mptcp_adjust_add_addr_timeout(msk);
>  	if (timeout)

[Severity: High]
This isn't a bug introduced by this patch, since the reset_timer block
already contained the timer_done = false plus sk_reset_timer() pair, but
since the patch adds another store into exactly this block, is the block
sufficiently serialized against the timer callback?

The reuse path runs from mptcp_pm_nl_announce_doit(), which holds
lock_sock(sk) and msk->pm.lock:

net/mptcp/pm_userspace.c:mptcp_pm_nl_announce_doit() {
	lock_sock(sk);
	spin_lock_bh(&msk->pm.lock);

	if (mptcp_pm_announced_alloc(msk, &addr_val.addr)) {
	...
}

mptcp_pm_add_addr_timer() writes timer_done under bh_lock_sock() only, and
its TCP_CLOSE early-out comes before the sock_owned_by_user() deferral:

net/mptcp/pm.c:mptcp_pm_add_addr_timer() {
	bh_lock_sock(sk);
	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
		goto out;

	if (sock_owned_by_user(sk)) {
		/* Try again later. */
		timeout = HZ / 20;
		goto out;
	}
	...
out:
	if (timeout)
		sk_reset_timer(sk, timer, jiffies + timeout);
	else
		/* if sock_put calls sk_free: avoid waiting for this timer */
		entry->timer_done = true;
	bh_unlock_sock(sk);
	sock_put(sk);
}

In TCP_CLOSE the callback reaches out: with timeout == 0 and stores
timer_done = true even when a process-context task owns the socket, since
lock_sock() only sets the owned flag and bh_lock_sock() still succeeds.
The reuse path does not take slock and the callback does not take pm.lock
around that store, so can the two stores interleave like this?

CPU0 (MPTCP_PM_CMD_ANNOUNCE re-announce, msk in TCP_CLOSE)
mptcp_pm_announced_alloc()
    add_entry->timer_done = false;
    sk_reset_timer()   /* timer becomes pending again */

CPU1 (previously armed timer expiring)
mptcp_pm_add_addr_timer()
    bh_lock_sock(sk);
    inet_sk_state_load(sk) == TCP_CLOSE -> goto out
    entry->timer_done = true;   /* lands after CPU0 re-armed */

That would leave the timer pending with timer_done == true, and teardown
via mptcp_pm_destroy()->mptcp_pm_free_announced_list() skips the sync stop
in that case:

net/mptcp/pm.c:mptcp_pm_free_announced_list() {
	list_for_each_entry_safe(entry, tmp, &free_list, list) {
		if (!entry->timer_done)
			sk_stop_timer_sync(sk, &entry->timer);
		kfree_rcu(entry, rcu);
	}
}

Can the still-queued timer then fire on the freed entry after the RCU
grace period, reading entry->sock and calling sock_put() on it? The
remaining expiry can be as long as the add_addr_timeout value, which is
much longer than a grace period.

For reachability, mptcp_pm_announced_alloc() only rejects
MPTCP_PM_DESTROYING, and mptcp_pm_nl_announce_doit() does no socket-state
check, so an msk sitting in TCP_CLOSE with a live token and an open fd
still reaches the reuse path.

Would stopping the timer (or taking bh_lock_sock()) before re-arming in
the reset_timer block, or moving the TCP_CLOSE check after the
sock_owned_by_user() check in mptcp_pm_add_addr_timer(), close this?

Also noting that no later patch in this series touches net/mptcp/pm.c, so
this state persists at the end of the series.

Re: [PATCH net 01/14] mptcp: do not reschedule the RTX timer for fallback sockets

[Paolo Abeni]

On 8/27/26 9:07 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> mptcp: do not reschedule the RTX timer for fallback sockets
> 
> This replaces the TCP_CLOSE state test in mptcp_reset_rtx_timer() with a
> new MPTCP_RTX_DISABLED bit in msk->flags, set both on fallback in
> __mptcp_try_fallback() and on the transition to TCP_CLOSE in
> mptcp_set_state().
> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index b474d03620a75..23dde0b7144bb 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
> 
> [ ... ]
> 
>> @@ -1084,13 +1085,14 @@ static bool mptcp_rtx_timer_pending(struct sock *sk)
>>  
>>  static void mptcp_reset_rtx_timer(struct sock *sk)
>>  {
>> +	struct mptcp_sock *msk = mptcp_sk(sk);
>>  	unsigned long tout;
>>  
>> -	/* prevent rescheduling on close */
>> -	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
>> +	/* Prevent rescheduling on close and in case of fallback. */
>> +	if (test_bit(MPTCP_RTX_DISABLED, &msk->flags))
>>  		return;
> 
> [Severity: Medium]
> The new test depends on a flag that mptcp_disconnect() clears while the
> socket is still in TCP_CLOSE, and after mptcp_stop_rtx_timer() already ran.
> The order inside mptcp_disconnect() is:
> 
> 	mptcp_set_state(sk, TCP_CLOSE);    /* sets MPTCP_RTX_DISABLED */
> 	...
> 	mptcp_stop_rtx_timer(sk);          /* non-sync, timer_ival = 0 */
> 	...
> 	WRITE_ONCE(msk->flags, 0);         /* clears MPTCP_RTX_DISABLED */
> 	msk->cb_flags = 0;
> 
> Can the rtx timer be armed again after that point?
> 
> mptcp_stop_rtx_timer() uses sk_stop_timer() and therefore the non-sync
> timer_delete(), so an already-expired mptcp_retransmit_timer() can still be
> running on another CPU. It only takes bh_lock_sock(), and its
> __set_bit(MPTCP_RETRANSMIT, &msk->cb_flags) can land after the plain
> msk->cb_flags = 0 store above.
> 
> On the release_sock() that follows, mptcp_release_cb() calls
> __mptcp_retrans(sk) with no sk_state check, and that reaches
> __mptcp_clean_una():
> 
> out:
> 	if (snd_una == msk->snd_nxt && snd_una == msk->write_seq) {
> 		if (mptcp_rtx_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
> 			mptcp_stop_rtx_timer(sk);
> 	} else {
> 		mptcp_reset_rtx_timer(sk);
> 	}
> 
> mptcp_disconnect() does not reset snd_una/snd_nxt/write_seq, so for a socket
> disconnected with unacked data the else branch runs and arms the timer with
> timer_ival == 0 on a TCP_CLOSE socket whose timers were deliberately
> stopped. The previous inet_sk_state_load(sk) == TCP_CLOSE test returned
> early here.
> 
> When that immediately-expiring timer fires, mptcp_retransmit_timer() does
> test_and_set_bit(MPTCP_WORK_RTX, &msk->flags) and then calls
> mptcp_schedule_work(), which refuses to queue the worker for a closed
> socket without clearing the bit:
> 
> 	if (inet_sk_state_load(sk) == TCP_CLOSE)
> 		return false;
> 
> If the application then reuses the socket with a new connect(), does that
> leftover MPTCP_WORK_RTX bit keep mptcp_worker() from being scheduled on
> subsequent rtx expiries, until some other mptcp_schedule_work() caller lets
> the worker clear it?
> 
>>  
>> -	tout = mptcp_sk(sk)->timer_ival;
>> +	tout = msk->timer_ival;
>>  	sk_reset_timer(sk, &sk->mptcp_retransmit_timer, jiffies + tout);
>>  }
>>  
>> @@ -3324,6 +3326,9 @@ void mptcp_set_state(struct sock *sk, int state)
>>  		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
>>  		 */
>>  		break;
>> +	case TCP_CLOSE:
>> +		set_bit(MPTCP_RTX_DISABLED, &mptcp_sk(sk)->flags);
>> +		fallthrough;
> 
> [Severity: High]
> This latches the bit on every transition to TCP_CLOSE, and there is no
> clear_bit(MPTCP_RTX_DISABLED) anywhere in the tree. The only site that
> resets the word is mptcp_disconnect():
> 
> 	WRITE_ONCE(msk->flags, 0);
> 
> Can this leave the bit set on a socket that afterwards carries a normal,
> non-fallback MPTCP connection?
> 
> First path, a failed connect() followed by a retry on the same fd. The
> error path of mptcp_connect() does:
> 
> 	if (unlikely(err)) {
> 		/* avoid leaving a dangling token in an unconnected socket */
> 		mptcp_token_destroy(msk);
> 		mptcp_set_state(sk, TCP_CLOSE);
> 		return err;
> 	}
> 
> For a negative return from sk->sk_prot->connect(), __inet_stream_connect()
> takes:
> 
> 		err = sk->sk_prot->connect(sk, uaddr, addr_len);
> 		if (err < 0)
> 			goto out;
> 
> so it never reaches the sock_error: label that calls ->disconnect(), and
> mptcp_disconnect() does not run. The socket stays SS_UNCONNECTED with
> sk_state TCP_CLOSE, which is exactly what __inet_stream_connect() requires
> to accept a second connect(), and __mptcp_nmpc_sk() also permits
> TCPF_CLOSE. The later mptcp_set_state(sk, TCP_SYN_SENT) and
> mptcp_set_state(sk, TCP_ESTABLISHED) do not clear the bit.
> 
> Second path, a failed listen() and inheritance by accepted sockets.
> mptcp_listen() unconditionally does:
> 
> 	err = __inet_listen_sk(ssk, backlog);
> 	release_sock(ssk);
> 	mptcp_set_state(sk, inet_sk_state_load(ssk));
> 
> which is TCP_CLOSE on failure, so the bit is latched on the listener; a
> later successful listen() only sets TCP_LISTEN. mptcp_sk_clone_init() then
> creates accepted sockets with sk_clone_lock(), which copies the whole
> struct mptcp_sock, msk->flags included, and __mptcp_init_sock() re-inits
> the lists, timers and allow_* fields but not msk->flags.
> 
> With the bit stuck, mptcp_reset_rtx_timer() is a permanent no-op.
> MPTCP_WORK_RTX and MPTCP_RETRANSMIT are set only by
> mptcp_retransmit_timer(), so does that mean __mptcp_retrans() can never run
> for such a socket, leaving data reinjected into msk->rtx_queue when a
> subflow stalls or dies unretransmitted, and DATA_FIN retransmission
> disabled, while the write side keeps its sndbuf pinned?
> 
> The previous guard read the live socket state, so it stopped applying on the
> next state transition. Would clearing the bit in __mptcp_init_sock(), and on
> the connect()/listen() retry paths, restore that property?
Both remarks here are a combo of pre-existing races and behaviour change
with this patch. Still I think they should be addresses within the same
scope. A new revision will be needed.

/P

Re: [PATCH net 02/14] mptcp: subflow: no need to copy thmac during ulp_clone

[Matthieu Baerts]

Hi Jakub,

On 27/08/2026 21:07, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.

Thank you for having forwarded this, I didn't manage to look at it before.

> ---
> mptcp: subflow: no need to copy thmac during ulp_clone
> 
> This drops the copy of subflow_req->thmac into the new subflow context in
> subflow_ulp_clone(), since the truncated HMAC is no longer needed once the
> passive-side request is over.  The commit message notes that thmac is left
> uninitialised on the SYN cookie path, so removing the copy also avoids a
> KMSAN uninitialised-read report.
> 
> (no Link: tags in the commit header)
> 
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index af81ad5e699d1..01db7edce18a6 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -2084,7 +2084,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
>>  		new_ctx->request_bkup = subflow_req->request_bkup;
>               ^^^^
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but while looking at the
> neighbouring store: does request_bkup have exactly the same
> uninitialised-on-SYN-cookies problem that motivated removing the thmac
> copy here?
> 
> subflow_init_req() only clears a subset of the fields:
> 
> net/mptcp/subflow.c:subflow_init_req() {
> 	subflow_req->mp_capable = 0;
> 	subflow_req->mp_join = 0;
> 	subflow_req->csum_reqd = mptcp_is_checksum_enabled(sock_net(sk_listener));
> 	subflow_req->allow_join_id0 = mptcp_allow_join_id0(sock_net(sk_listener));
> 	subflow_req->msk = NULL;
> 	mptcp_token_init_request(req);
> }
> 
> and the only assignment of request_bkup for a request sock happens on the
> SYN, in subflow_token_join_request():
> 
> 	subflow_req->local_id = local_id;
> 	subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);
> 
> On the cookie ACK path that function is not called, and the state restore
> does not cover request_bkup either:
> 
> net/mptcp/syncookies.c:mptcp_token_join_cookie_init_state() {
> 	subflow_req->remote_nonce = e->remote_nonce;
> 	subflow_req->local_nonce = e->local_nonce;
> 	subflow_req->backup = e->backup;
> 	subflow_req->remote_id = e->join_id;
> 	subflow_req->local_id = e->local_id;
> 	subflow_req->token = e->token;
> 	subflow_req->msk = msk;
> }
> 
> struct join_entry has no request_bkup member at this commit, so the bit
> carries whatever was in the freshly allocated request sock.
> 
> Unlike thmac, the copied value is consumed afterwards, for instance in
> mptcp_subflow_get_send() and mptcp_subflow_get_retrans():
> 
> net/mptcp/protocol.c:mptcp_subflow_get_send() {
> 	bool backup = subflow->backup || subflow->request_bkup;
> 
> It is also put on the wire by mptcp_established_options_mp_prio() and
> exported to userspace by the mptcp diag code.
> 
> So when a remote MP_JOIN is accepted while tcp_syncookies is active, can a
> joined subflow end up randomly flagged as backup and reported that way in
> diag, along with the same class of KMSAN uninit-value branch report that
> this patch removes for thmac?
> 
> For the record, the next patch in this series, "mptcp: syncookies: remember
> the request backup flag", adds request_bkup:1 to struct join_entry and
> stores/restores it, and the store happens after subflow_token_join_request()
> has set the field (subflow_token_join_request() runs before
> subflow_init_req_cookie_join_save() in subflow_check_req()), so the field is
> properly initialised at the end of the series.

All of this to say that it is fixed in the next patch...

> Would it be worth mentioning
> that dependency here, given this patch carries a Cc: stable tag?

Do we really need to mention that?? This report says "there is a
pre-existing bug in the code around", then there is no direct
dependence. Or should we add for each patch: other fixes follow this one?

Note that there is an issue on Sashiko's side to improve that:

https://github.com/sashiko-dev/sashiko/issues/392

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