mptcp: fix request migration ownership
Subsystems (4)
v4 cover mptcp: fix request migration ownership
From: Ruide Cao <[email protected]>
Hi,
TCP request migration clones pending requests with inet_reqsk_clone(). Some MPTCP request fields carry ownership which cannot be transferred by a plain byte copy.
For MP_JOIN requests, subflow_req->msk holds a socket reference. The clone inherits the pointer without acquiring its own reference, so the original and cloned requests can drop the same reference. Patch 1 lets the cloned request take an additional reference.
For MP_CAPABLE requests, token_node belongs to the original request and is hashed in the token table. Copying the node gives the clone invalid hash state, while concurrent request migration and RX processing can observe different owners in the ehash and token tables. Patch 2 moves token-table ownership from the original request to the clone under the bucket lock, and makes token acceptance and destruction tolerate a token that has already been removed.
--------------------
Changes in v4:
- Rebuilt the series from the final v3 patches.
- Kept MP_JOIN and MP_CAPABLE ownership fixes on the MPTCP request clone path, matching v3.
- Replaced the TCP-only tcp_done()/inet_csk_prepare_forced_close() failure path with MPTCP-specific clone teardown.
- v3 link:
https://lore.kernel.org/all/[email protected]/
Changes in v3:
- Split MP_JOIN and MP_CAPABLE into two patches.
- Reworked MP_CAPABLE token migration to happen during MPTCP request cloning.
- Made MP_CAPABLE accept/destroy paths tolerant of already moved or removed request tokens.
- Added a packetdrill MP_CAPABLE reproducer and decoded warning.
- Dropped the redundant IPPROTO_TCP guard around the direct MPTCP clone call, as request migration is TCP-only.
- Based v3 on the latest net tree rather than the current mptcp_net-next export, which has not yet merged commit a0ab2ba83e35 ("tcp: fix TFO max_qlen accounting across reuseport migration") and therefore lacks the overlapping inet_reqsk_clone() changes.
- v2 link:
https://lore.kernel.org/all/40fd38e7a368e5b7bc9bc83364a32241f977d53f.1778404619.git.caoruide123@gmail.com/
Changes in v2:
- drop the generic request_sock clone callback
- call MPTCP directly from inet_reqsk_clone() under the TCP protocol check
- keep cloned MP_JOIN requests holding an msk reference
- clear raw-copied MP_CAPABLE token hash state in the clone
- move MP_CAPABLE token ownership only after successful req migration
- avoid exposing token internals to inet_connection_sock.c
- update the commit message accordingly
- v1 link:
https://lore.kernel.org/all/86e2514b533bf4d55d4aa2fdbf1404022e8c9430.1776149210.git.caoruide123@gmail.com/
------------------------------
// poc for MP_JOIN:
// Minimal reproducer for a stale subflow_req->msk after reqsk migration. --tolerance_usecs=200000
--non_fatal=packet
`sysctl -q net.mptcp.enabled=1
sysctl -q net.ipv4.tcp_migrate_req=1
sysctl -q net.ipv4.tcp_synack_retries=1`
// Listener A and the owning MPTCP connection.
+0 socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3 +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 +0 setsockopt(3, SOL_SOCKET, SO_REUSEPORT, [1], 4) = 0 +0 bind(3, ..., ...) = 0 +0 listen(3, 8) = 0
+0.0 < addr[caddr0] > addr[saddr0] S 0:0(0) win 65535 <mss 1460, sackOK, TS val 1000 ecr 0, nop, wscale 8, mpcapable v1 flags[flag_h] nokey> +0.0 > S. 0:0(0) ack 1 <mss 1460, sackOK, TS val 1000 ecr 1000, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]> +0.1 < . 1:1(0) ack 1 win 256 <nop, nop, TS val 1000 ecr 1000, mpcapable v1 flags[flag_h] key[ckey=2, skey]> +0 accept(3, ..., ...) = 4
// Make the MPTCP socket fully established so it accepts MP_JOIN.
+0.1 < P. 1:3(2) ack 1 win 256 <nop, nop, TS val 1001 ecr 1000, mpcapable v1 flags[flag_h] key[skey, ckey] mpcdatalen 2, nop, nop> +0.0 > . 1:1(0) ack 3 <nop, nop, TS val 1001 ecr 1001, dss dack8=3 dll=0 nocs>
// Leave exactly one MP_JOIN request half-open.
+0.1 < addr[caddr1] > addr[saddr0] S 0:0(0) win 65535 <mss 1460, sackOK, TS val 2000 ecr 0, nop, wscale 8, mp_join_syn address_id=1 token=sha256_32(skey)> +0.0 > S. 0:0(0) ack 1 <mss 1460, sackOK, TS val 2000 ecr 2000, nop, wscale 8, mp_join_syn_ack address_id=0 sender_hmac=auto>
// Listener B joins the reuseport group, then A is closed. The next request // timer clones and migrates the half-open MP_JOIN request to B.
+0.1 socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 5 +0 setsockopt(5, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 +0 setsockopt(5, SOL_SOCKET, SO_REUSEPORT, [1], 4) = 0 +0 bind(5, ..., ...) = 0 +0 listen(5, 8) = 0 +0 close(3) = 0
// Wait past the first SYN+ACK RTO, then release the owning MPTCP socket.
+1.5 setsockopt(4, SOL_SOCKET, SO_LINGER, {onoff=1, linger=0}, 8) = 0
+0 close(4) = 0
// The migrated request expires on its next timer and its destructor uses msk.
+4.0 true
------------------------------
crash log of MP_JOIN
[ 280.449259] [ C0] BUG: KASAN: slab-use-after-free in subflow_req_destructor (net/mptcp/subflow.c:45) [ 280.449417] [ C0] Write of size 4 at addr ff1100010e008d40 by task swapper/0/0 [ 280.449525] [ C0] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 7.2.0-rc5-00353-gc27e36054537 #10 PREEMPT(full)
[ 280.449637] [ C0] Call Trace: [ 280.450422] [ C0] subflow_req_destructor (net/mptcp/subflow.c:45) [ 280.450504] [ C0] subflow_v4_req_destructor (net/mptcp/subflow.c:694) [ 280.450581] [ C0] __reqsk_free (net/ipv4/inet_connection_sock.c:906) [ 280.450681] [ C0] reqsk_timer_handler (include/net/request_sock.h:137 net/ipv4/inet_connection_sock.c:1147)
[ 280.454937] [ C0] Allocated by task 10014: [ 280.455381] [ C0] sk_prot_alloc (net/core/sock.c:2246) [ 280.455516] [ C0] sk_clone (net/core/sock.c:2488) [ 280.455611] [ C0] mptcp_sk_clone_init (include/net/sock.h:1848 net/mptcp/protocol.c:3564) [ 280.455683] [ C0] subflow_syn_recv_sock (net/mptcp/subflow.c:883) [ 280.455772] [ C0] tcp_check_req (net/ipv4/tcp_minisocks.c:934)
[ 280.457177] [ C0] Freed by task 0: [ 280.457603] [ C0] slab_free_after_rcu_debug (include/linux/kasan.h:235 mm/slub.c:2677 mm/slub.c:6439) [ 280.457688] [ C0] rcu_core (kernel/rcu/tree.c:2645 kernel/rcu/tree.c:2897)
[ 280.458199] [ C0] Last potentially related work creation: [ 280.458426] [ C0] kmem_cache_free (mm/slub.c:2638 mm/slub.c:6377 mm/slub.c:6504) [ 280.458518] [ C0] __sk_destruct (net/core/sock.c:2289 net/core/sock.c:2391) [ 280.458607] [ C0] sk_destruct (net/core/sock.c:2419) [ 280.458700] [ C0] __sk_free (net/core/sock.c:2430) [ 280.458793] [ C0] sk_free (net/core/sock.c:2441) [ 280.458885] [ C0] mptcp_close (include/net/sock.h:2020 net/mptcp/protocol.c:3399) [ 280.458969] [ C0] inet_release (net/ipv4/af_inet.c:442)
[ 280.459536] [ C0] The buggy address belongs to the cache MPTCP of size 2968 [ 280.459593] [ C0] The buggy address is 128 bytes inside a freed 2968-byte region
------------------------------
MP_CAPABLE packetdrill reproducer:
// Reproducer for MP_CAPABLE request token ownership during TCP req migration. //
// The first listener owns the request created by the MP_CAPABLE SYN. A second // SO_REUSEPORT listener is added only after that SYN, then the first listener is // closed. The SYN+ACK retransmission timer migrates the request to the second // listener, and a later request timer destroys the migrated request. //
// On a vulnerable kernel, inet_reqsk_clone() raw-copies token_node. The clone // is not the token table owner, so destroying the migrated request triggers the // MPTCP token ownership bug.
--tolerance_usecs=250000
+0 `sysctl -q net.mptcp.enabled=1` +0 `sysctl -q net.ipv4.tcp_migrate_req=1` +0 `sysctl -q net.ipv4.tcp_synack_retries=2` +0 `sysctl -q net.ipv4.tcp_timestamps=1` +0 `sysctl -q kernel.panic_on_warn=0` +0 `sysctl -q kernel.panic_on_oops=0` +0 `ip tcp_metrics flush all >/dev/null 2>&1 || true` +0 `tc qdisc replace dev tun0 root pfifo >/dev/null 2>&1 || true`
+0 socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3 +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 +0 setsockopt(3, SOL_SOCKET, SO_REUSEPORT, [1], 4) = 0 +0 getsockopt(3, SOL_TCP, TCP_IS_MPTCP, [1], [4]) = 0 +0 bind(3, ..., ...) = 0 +0 listen(3, 1) = 0
+0 < S 0:0(0) win 32792 <mss 1000, sackOK, nop, nop, nop, wscale 7, mpcapable v1 flags[flag_h] nokey> +0 > S. 0:0(0) ack 1 <mss 1460, nop, nop, sackOK, nop, wscale 9, mpcapable v1 flags[flag_h] key[skey]>
+0 socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 4 +0 setsockopt(4, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 +0 setsockopt(4, SOL_SOCKET, SO_REUSEPORT, [1], 4) = 0 +0 getsockopt(4, SOL_TCP, TCP_IS_MPTCP, [1], [4]) = 0 +0 bind(4, ..., ...) = 0 +0 listen(4, 1) = 0
+0 close(3) = 0
// Let the retransmission timer migrate the request to fd 4 and let the migrated // request expire. On vulnerable kernels its raw-copied token_node is not the // token-table owner, so the request timer trips the token owner assertion.
+8.0 true
+0 close(4) = 0
------------------------------
decoded warning from MP_CAPABLE reproducer:
[ 314.661418] [ C2] ------------[ cut here ]------------ [ 314.661636] [ C2] WARNING: net/mptcp/token.c:364 at mptcp_token_destroy_request+0x2b0/0x330, CPU#2: swapper/2/0 [ 314.661931] [ C2] CPU: 2 UID: 0 PID: 0 Comm: swapper/2 Not tainted 7.2.0-rc5-00353-gc27e36054537 #10 PREEMPT(full) [ 314.662092] [ C2] RIP: 0010:mptcp_token_destroy_request (build/../net/mptcp/token.c:364 (discriminator 1)) [ 314.663001] [ C2] Call Trace: [ 314.663043] [ C2] <IRQ> [ 314.663107] [ C2] subflow_v4_req_destructor (build/../net/mptcp/subflow.c:694) [ 314.663213] [ C2] __reqsk_free (build/../net/ipv4/inet_connection_sock.c:906) [ 314.663325] [ C2] reqsk_timer_handler (build/../include/net/request_sock.h:137 build/../net/ipv4/inet_connection_sock.c:1147) [ 314.663762] [ C2] call_timer_fn (build/../kernel/time/timer.c:1748) [ 314.668915] [ C2] ---[ end trace 0000000000000000 ]---
Ruide Cao (2):
mptcp: hold MP_JOIN msk ref when cloning reqsk
mptcp: fix MP_CAPABLE token migration when cloning reqsk
include/net/mptcp.h | 7 ++++ net/ipv4/inet_connection_sock.c | 4 +++ net/mptcp/protocol.c | 31 ++++++++++++++--- net/mptcp/protocol.h | 4 ++- net/mptcp/subflow.c | 13 +++++++ net/mptcp/token.c | 61 +++++++++++++++++++++++++++++---- net/mptcp/token_test.c | 4 +-- 7 files changed, 109 insertions(+), 15 deletions(-)
Signature · 1 line
2.43.0
PATCH v4 1/2 mptcp: hold MP_JOIN msk ref when cloning reqsk
From: Ruide Cao <[email protected]>
TCP request migration clones pending request sockets with inet_reqsk_clone(). For MPTCP MP_JOIN requests this byte-copies subflow_req->msk, but the clone does not take its own reference.
The original and cloned request can consequently drop the same msk reference from subflow_req_destructor(), leaving one request with a dangling pointer after the other is released.
Add an MPTCP clone helper on the TCP request migration path and let cloned subflow requests grab their own msk reference when one is present. The clone's normal destructor balances this reference on both successful migration and failed clone/insert paths.
--- include/net/mptcp.h | 7 +++++++ net/ipv4/inet_connection_sock.c | 4 ++++ net/mptcp/subflow.c | 11 +++++++++++ 3 files changed, 22 insertions(+)
include/net/mptcp.h +7 -0
@@ -223,6 +223,8 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,struct request_sock *mptcp_subflow_reqsk_alloc (const struct request_sock_ops *ops,struct sock *sk_listener,bool attach_listener); +void mptcp_subflow_reqsk_clone (struct request_sock *req, +struct request_sock *new_req);__be32 mptcp_get_reset_option (const struct sk_buff *skb);
@@ -309,6 +311,11 @@ static inline struct request_sock *mptcp_subflow_reqsk_alloc(const struct requesreturn NULL ; } +static inline void mptcp_subflow_reqsk_clone (struct request_sock *req, +struct request_sock *new_req) +{ +} +static inline __be32 mptcp_reset_option (const struct sk_buff *skb) {return htonl (0u ); }static inline void mptcp_active_detect_blackhole (struct sock *sk,bool expired) { }
net/ipv4/inet_connection_sock.c +4 -0
@@ -21,6 +21,7 @@#include <net/xfrm.h> #include <net/tcp.h> #include <net/tcp_ecn.h> +#include <net/mptcp.h> #include <net/sock_reuseport.h> #include <net/addrconf.h>
@@ -961,6 +962,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,rcu_assign_pointer (tcp_sk (nreq->sk)->fastopen_rsk, nreq); } +if (rsk_is_mptcp (req)) +mptcp_subflow_reqsk_clone (req, nreq); +return nreq; }
net/mptcp/subflow.c +11 -0
@@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)mptcp_token_destroy_request (req); } +void mptcp_subflow_reqsk_clone (struct request_sock *req, +struct request_sock *new_req) +{ +struct mptcp_subflow_request_sock *subflow_req; + + subflow_req =mptcp_subflow_rsk (new_req); + +if (subflow_req->msk) +sock_hold ((struct sock *)subflow_req->msk); +} +static void subflow_generate_hmac (u64 key1,u64 key2,u32 nonce1,u32 nonce2,void *hmac) {
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH net v4 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] TCP request migration clones MPTCP request sockets without updating or re-hashing the token_node list pointers for MP_CAPABLE requests, leading to hash list corruption and a reliable kernel warning (WARN_ON_ONCE) that can cause a Denial of Service. --
--- Patch [1]: [PATCH net v4 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk --- commit 2b2ebcd4a3ea728b0d1c8d74d1e09d5e75f65848
Author: Ruide Cao <[email protected]>
mptcp: hold MP_JOIN msk ref when cloning reqsk
TCP request migration clones pending request sockets with inet_reqsk_clone(). For MPTCP MP_JOIN requests this byte-copies subflow_req->msk, but the clone does not take its own reference.
Quoted 8 lines
net/mptcp/subflow.c +0
-0@@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)mptcp_token_destroy_request (req); }
net/mptcp/subflow.c static void subflow_req_destructor(struct request_sock *req) · line 59
Quoted 10 lines
net/mptcp/subflow.c +10
-0+void mptcp_subflow_reqsk_clone (struct request_sock *req, +struct request_sock *new_req) +{ +struct mptcp_subflow_request_sock *subflow_req; + + subflow_req =mptcp_subflow_rsk (new_req); + +if (subflow_req->msk) +sock_hold ((struct sock *)subflow_req->msk); +}
[Severity: High]
This is a pre-existing issue, but does this clone helper also need to handle the migration of token_node for MP_CAPABLE requests?
When inet_reqsk_clone() performs a byte-copy of the request socket, it copies the token_node list pointers (pprev and next) as-is:
net/ipv4/inet_connection_sock.c:inet_reqsk_clone() {
...
unsafe_memcpy(&nreq_sk->sk_dontcopy_end, &req_sk->sk_dontcopy_end,
req->rsk_ops->obj_size - offsetof(struct sock, sk_dontcopy_end),
/* alloc is larger than struct, see above */);
...
}Because the list pointers are copied without re-hashing or updating them, the original request is destroyed and removes itself from the token hash table, leaving the clone with a stale pprev pointer.
When the cloned request is eventually destroyed, mptcp_token_destroy_request() checks hlist_nulls_unhashed(), which returns false due to the stale pointer. It then fails to find the token in the bucket, which triggers a WARN_ON_ONCE:
net/mptcp/token.c:mptcp_token_destroy_request() {
...
pos = __token_lookup_req(bucket, subflow_req->token);
if (!WARN_ON_ONCE(pos != subflow_req)) {
hlist_nulls_del_init_rcu(&pos->token_node);
bucket->chain_len--;
}
...
}This could cause a kernel panic on systems with panic_on_warn enabled, and permanently lose the MP_CAPABLE token from the hash table, breaking subsequent MP_JOIN requests for that connection.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH net v4 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk
Quoted 2 lines · On Thu, Aug 13, 2026 at 06:56:30AM +0000, [email protected] wrote:
This is a pre-existing issue, but does this clone helper also need to handle the migration of
token_nodeforMP_CAPABLErequests?
That is what patch 2/2 of this series does:
It extends this helper with mptcp_token_move_request(), which re-hashes the token node onto the clone with hlist_nulls_replace_init_rcu() under the token bucket lock, so the clone becomes the single token-table owner and the original is left unhashed. mptcp_token_accept() and mptcp_token_destroy_request() re-check ownership under the same lock.
The same question was raised on v3, and v4 was restructured to answer it. Reviewing 1/2 in isolation makes it read as an unfixed issue.
Two details of the analysis are inaccurate, for the archive:
Quoted 1 line
... leading to hash list corruption and a reliable kernel warning
There is no list corruption. Nothing in the bucket chain points at the clone's copied token_node, so __token_lookup_req() can never return it, and both list mutations are gated on "pos == subflow_req". The stale node is never passed to hlist_nulls_del_init_rcu(). The splat is the only observable effect: the panic_on_warn DoS holds, the corruption does not.
Quoted 2 lines
permanently lose the
MP_CAPABLEtoken from the hash table, breaking subsequentMP_JOINrequests for that connection.
The msk insertion sits outside the WARN_ON_ONCE() guard:
if (!WARN_ON_ONCE(pos != req)) hlist_nulls_del_init_rcu(&req->token_node); __sk_nulls_add_node_rcu((struct sock *)msk, &bucket->msk_chain);
so the msk still enters msk_chain, mptcp_token_get_sock() keeps finding it, and MP_JOIN for that connection keeps working. What does drift is bucket->chain_len: a net -1 per migrated connection that completes the handshake. It is a signed int, so it goes negative and __token_bucket_busy()'s TOKEN_MAX_CHAIN_LEN cap stops limiting that bucket. Patch 2/2 fixes that as well.
Thanks,
Ruide
Quoted 78 lines · <[email protected]> 于2026年8月13日周四 14:56写道:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] TCP request migration clones MPTCP request sockets without updating or re-hashing thetoken_nodelist pointers forMP_CAPABLErequests, leading to hash list corruption and a reliable kernel warning (WARN_ON_ONCE) that can cause a Denial of Service. ----- Patch [1]: [PATCH net v4 1/2] mptcp: hold
MP_JOINmsk ref when cloning reqsk --- commit2b2ebcd4a3ea728b0d1c8d74d1e09d5e75f65848
Author: Ruide Cao <[email protected]>mptcp: hold
MP_JOINmsk ref when cloning reqskTCP request migration clones pending request sockets with
inet_reqsk_clone(). For MPTCPMP_JOINrequests this byte-copiessubflow_req->msk, but the clone does not take its own reference.Quoted 8 lines
net/mptcp/subflow.c +0
-0@@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)mptcp_token_destroy_request (req); }net/mptcp/subflow.c static void subflow_req_destructor(struct request_sock *req) · line 59
Quoted 10 lines
net/mptcp/subflow.c +10
-0+void mptcp_subflow_reqsk_clone (struct request_sock *req, +struct request_sock *new_req) +{ +struct mptcp_subflow_request_sock *subflow_req; + + subflow_req =mptcp_subflow_rsk (new_req); + +if (subflow_req->msk) +sock_hold ((struct sock *)subflow_req->msk); +}[Severity: High]
This is a pre-existing issue, but does this clone helper also need to handle the migration oftoken_nodeforMP_CAPABLErequests?When
inet_reqsk_clone()performs a byte-copy of the request socket, it copies thetoken_nodelist pointers (pprev and next) as-is:net/ipv4/inet_connection_sock.c:inet_reqsk_clone() { ... unsafe_memcpy(&nreq_sk->sk_dontcopy_end, &req_sk->sk_dontcopy_end, req->rsk_ops->obj_size - offsetof(struct sock, sk_dontcopy_end), /* alloc is larger than struct, see above */); ... }Because the list pointers are copied without re-hashing or updating them, the original request is destroyed and removes itself from the token hash table, leaving the clone with a stale pprev pointer.
When the cloned request is eventually destroyed,
mptcp_token_destroy_request()checkshlist_nulls_unhashed(), which returns false due to the stale pointer. It then fails to find the token in the bucket, which triggers aWARN_ON_ONCE:net/mptcp/token.c:mptcp_token_destroy_request() { ... pos = __token_lookup_req(bucket, subflow_req->token); if (!WARN_ON_ONCE(pos != subflow_req)) { hlist_nulls_del_init_rcu(&pos->token_node); bucket->chain_len--; } ... }This could cause a kernel panic on systems with
panic_on_warnenabled, and permanently lose theMP_CAPABLEtoken from the hash table, breaking subsequentMP_JOINrequests for that connection.--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Matthieu Baerts
maintainerRe: [PATCH net v4 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk
Hi Ruide,
(+cc netdev ML)
Thank you for having replied to Sashiko's review.
Quoted 5 lines · Wei Ren
Quoted 2 lines · On Thu, Aug 13, 2026 at 06:56:30AM +0000, [email protected] wrote:
This is a pre-existing issue, but does this clone helper also need to handle the migration of
token_nodeforMP_CAPABLErequests?That is what patch 2/2 of this series does:
Indeed, Sashiko Gemini didn't look at the next patch. Maybe adding a note in the commit message to say that a similar issue is visible with MP_CAPABLE and addressed in the next patch? Still, that doesn't feel natural to add that...
Do you mind also looking at Clashiko's review, please? The suggestion of replacing sock_hold() by refcount_inc_not_zero() might be valid (to be double checked):
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1786497414.git.yuantan098%40gmail.com
------------------------------ 8< ---------------------------------
Quoted 4 lines
net/ipv4/inet_connection_sock.c +0
-0
[ ... ]
Quoted 18 lines
net/ipv4/inet_connection_sock.c +3
-0@@ -961,6 +962,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,rcu_assign_pointer (tcp_sk (nreq->sk)->fastopen_rsk, nreq); } +if (rsk_is_mptcp (req)) +mptcp_subflow_reqsk_clone (req, nreq); +return nreq; }net/mptcp/subflow.c +0
-0@@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)mptcp_token_destroy_request (req); }
net/mptcp/subflow.c static void subflow_req_destructor(struct request_sock *req) · line 59
Quoted 10 lines
net/mptcp/subflow.c +10
-0+void mptcp_subflow_reqsk_clone (struct request_sock *req, +struct request_sock *new_req) +{ +struct mptcp_subflow_request_sock *subflow_req; + + subflow_req =mptcp_subflow_rsk (new_req); + +if (subflow_req->msk) +sock_hold ((struct sock *)subflow_req->msk); +}
Discovered by: gpt-5-6-sol, opus-5 · Missed by: opus-4-7, sashiko-gemini
Can the sock_hold() here run on an msk whose last reference has already been dropped?
The pointer being held was snapshotted by the plain (unsafe_)memcpy() in inet_reqsk_clone(), and the cloning CPU owns no reference to that msk at this point. The clone happens optimistically in reqsk_timer_handler() before any ownership arbitration:
net/ipv4/inet_connection_sock.c:reqsk_timer_handler() {
...
nreq = inet_reqsk_clone(req, nsk);
if (!nreq)
goto drop;
...
/* Even if we can clone the req, we may need not retransmit any more
* SYN+ACKs (nreq->num_timeout > max_syn_ack_retries, etc), or another
* CPU may win the "own_req" race so that inet_ehash_insert() fails.
*/
}So the third-ACK path can be running on the same request concurrently. There, the request's msk reference is handed to the child and can then be released synchronously:
net/mptcp/subflow.c:subflow_syn_recv_sock() {
...
/* move the msk reference ownership to the subflow */
subflow_req->msk = NULL;
ctx->conn = (struct sock *)owner;
if (subflow_use_different_sport(owner, sk)) {
...
if (!mptcp_pm_announced_has_ssk(owner, sk)) {
...
goto dispose_child;
}
}
if (!mptcp_finish_join(child)) {
...
goto dispose_child;
}
}and dispose_child reaches mptcp_subflow_drop_ctx():
if (ctx->conn) sock_put(ctx->conn);
If that put (or a later teardown of the joined subflow) is the last reference, for example because userspace closed the MPTCP connection concurrently, then the refcount_inc() in sock_hold() lands on freed memory and on a zero refcount, which is the refcount_warn_saturate() "addition on 0; use-after-free" case.
The window between the memcpy() snapshot and this sock_hold() is not bounded either: in the TFO branch of inet_reqsk_clone() it even contains spin_lock_bh(&fastopenq->lock).
Would refcount_inc_not_zero() be more appropriate here, with new_req->msk cleared when it fails so subflow_req_destructor() does not put a reference that was never taken? Alternatively, could the hold be taken under the same exclusion that performs the ownership transfer in subflow_syn_recv_sock()?
Also, the helper takes req but never uses it, so the original request is not re-validated at all. Is the req argument intended to be used?
Discovered by: opus-5 · Missed by: opus-4-7, gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
This is a pre-existing issue and is not introduced by this patch, but as long as this hook is the designated fixup point for cloned subflow
requests: inet_reqsk_clone() also byte-copies subflow_req->token_node, the hlist_nulls node that mptcp_token_new_request() linked into the MPTCP token bucket for MP_CAPABLE requests, and this version of the helper repairs only ->msk.
The generic detach helper does not cover it either:
net/ipv4/inet_connection_sock.c:reqsk_migrate_reset() {
req->saved_syn = NULL;
...
}With only this patch applied, the clone's token_node looks hashed while it is not reachable from the bucket chain, so after a successful migration the surviving clone trips the pedantic check in mptcp_token_accept():
net/mptcp/token.c:mptcp_token_accept() {
...
/* pedantic lookup check for the moved token */
pos = __token_lookup_req(bucket, req->token);
if (!WARN_ON_ONCE(pos != req))
hlist_nulls_del_init_rcu(&req->token_node);
}and the token ends up no longer reserved.
This is resolved later in the same series: the following patch, "mptcp: fix MP_CAPABLE token migration when cloning reqsk", adds mptcp_token_move_request() and calls it from mptcp_subflow_reqsk_clone(), so the state does not exist at the end of the series. Would it be worth noting the ordering dependency between the two patches in the changelog, given the Cc: stable tag on this one? ------------------------------ 8< ------------------------------
Cheers,
Matt
Signature · 1 line
Sponsored by the NGI0 Core fund.
Re: [PATCH net v4 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk
Quoted 170 lines · Matthieu Baerts <[email protected]> 于2026年8月20日周四 19:18写道:
Hi Ruide,
(+cc netdev ML)
Thank you for having replied to Sashiko's review.
Quoted 5 lines · Wei Ren
Quoted 2 lines · On Thu, Aug 13, 2026 at 06:56:30AM +0000, [email protected] wrote:
This is a pre-existing issue, but does this clone helper also need to handle the migration of
token_nodeforMP_CAPABLErequests?That is what patch 2/2 of this series does:
Indeed, Sashiko Gemini didn't look at the next patch. Maybe adding a note in the commit message to say that a similar issue is visible with
MP_CAPABLEand addressed in the next patch? Still, that doesn't feel natural to add that...Do you mind also looking at Clashiko's review, please? The suggestion of replacing
sock_hold()byrefcount_inc_not_zero()might be valid (to be double checked):https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1786497414.git.yuantan098%40gmail.com
------------------------------ 8< ---------------------------------
Quoted 4 lines
net/ipv4/inet_connection_sock.c +0
-0[ ... ]
Quoted 18 lines
net/ipv4/inet_connection_sock.c +3
-0@@ -961,6 +962,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,rcu_assign_pointer (tcp_sk (nreq->sk)->fastopen_rsk, nreq); } +if (rsk_is_mptcp (req)) +mptcp_subflow_reqsk_clone (req, nreq); +return nreq; }net/mptcp/subflow.c +0
-0@@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)mptcp_token_destroy_request (req); }net/mptcp/subflow.c static void subflow_req_destructor(struct request_sock *req) · line 59
Quoted 10 lines
net/mptcp/subflow.c +10
-0+void mptcp_subflow_reqsk_clone (struct request_sock *req, +struct request_sock *new_req) +{ +struct mptcp_subflow_request_sock *subflow_req; + + subflow_req =mptcp_subflow_rsk (new_req); + +if (subflow_req->msk) +sock_hold ((struct sock *)subflow_req->msk); +}Discovered by: gpt-5-6-sol, opus-5 · Missed by: opus-4-7, sashiko-gemini
Can the
sock_hold()here run on an msk whose last reference has already been dropped?The pointer being held was snapshotted by the plain (unsafe_)memcpy() in
inet_reqsk_clone(), and the cloning CPU owns no reference to that msk at this point. The clone happens optimistically inreqsk_timer_handler()before any ownership arbitration:net/ipv4/inet_connection_sock.c:reqsk_timer_handler() { ... nreq = inet_reqsk_clone(req, nsk); if (!nreq) goto drop; ... /* Even if we can clone the req, we may need not retransmit any more * SYN+ACKs (nreq->num_timeout > max_syn_ack_retries, etc), or another * CPU may win the "own_req" race so that inet_ehash_insert() fails. */ }So the third-ACK path can be running on the same request concurrently. There, the request's msk reference is handed to the child and can then be released synchronously:
net/mptcp/subflow.c:subflow_syn_recv_sock() { ... /* move the msk reference ownership to the subflow */ subflow_req->msk = NULL; ctx->conn = (struct sock *)owner; if (subflow_use_different_sport(owner, sk)) { ... if (!mptcp_pm_announced_has_ssk(owner, sk)) { ... goto dispose_child; } } if (!mptcp_finish_join(child)) { ... goto dispose_child; } }and
dispose_childreachesmptcp_subflow_drop_ctx():if (ctx->conn) sock_put(ctx->conn);If that put (or a later teardown of the joined subflow) is the last reference, for example because userspace closed the MPTCP connection concurrently, then the
refcount_inc()insock_hold()lands on freed memory and on a zero refcount, which is therefcount_warn_saturate()"addition on 0; use-after-free" case.The window between the
memcpy()snapshot and thissock_hold()is not bounded either: in the TFO branch ofinet_reqsk_clone()it even containsspin_lock_bh(&fastopenq->lock).Would
refcount_inc_not_zero()be more appropriate here, withnew_req->mskcleared when it fails sosubflow_req_destructor()does not put a reference that was never taken? Alternatively, could the hold be taken under the same exclusion that performs the ownership transfer insubflow_syn_recv_sock()?Also, the helper takes req but never uses it, so the original request is not re-validated at all. Is the req argument intended to be used?
Discovered by: opus-5 · Missed by: opus-4-7, gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
This is a pre-existing issue and is not introduced by this patch, but as long as this hook is the designated fixup point for cloned subflow
requests:inet_reqsk_clone()also byte-copiessubflow_req->token_node, thehlist_nullsnode thatmptcp_token_new_request()linked into the MPTCP token bucket forMP_CAPABLErequests, and this version of the helper repairs only ->msk.The generic detach helper does not cover it either:
net/ipv4/inet_connection_sock.c:reqsk_migrate_reset() { req->saved_syn = NULL; ... }With only this patch applied, the clone's
token_nodelooks hashed while it is not reachable from the bucket chain, so after a successful migration the surviving clone trips the pedantic check inmptcp_token_accept():net/mptcp/token.c:mptcp_token_accept() { ... /* pedantic lookup check for the moved token */ pos = __token_lookup_req(bucket, req->token); if (!WARN_ON_ONCE(pos != req)) hlist_nulls_del_init_rcu(&req->token_node); }and the token ends up no longer reserved.
This is resolved later in the same series: the following patch, "mptcp: fix
MP_CAPABLEtoken migration when cloning reqsk", addsmptcp_token_move_request()and calls it frommptcp_subflow_reqsk_clone(), so the state does not exist at the end of the series. Would it be worth noting the ordering dependency between the two patches in the changelog, given the Cc: stable tag on this one? ------------------------------ 8< ------------------------------
Quoted 5 lines
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Thank you for catching this. The suggestion is valid. We've fixed it in our v5.
PATCH v4 2/2 mptcp: fix MP_CAPABLE token migration when cloning reqsk
In reply to Ren WeiFrom: Ruide Cao <[email protected]>
TCP request migration clones pending request sockets with inet_reqsk_clone(). For MPTCP MP_CAPABLE requests this byte-copies the token_node hlist state into the clone even though the token table still names the original request.
Moving the token only after inet_ehash_insert() succeeds leaves a window where the cloned request is already globally visible from the ehash but the token table still points at the original request. Reordering the move after clone but before ehash exposure closes that window, but concurrent RX can still race on the old request and observe that the token was already moved.
Move MP_CAPABLE token request ownership during MPTCP request cloning under the token bucket lock. Make mptcp_token_accept() and mptcp_token_destroy_request() re-check token_node under the same lock and treat an already moved or removed request as a normal race instead of warning.
If the passive MP_CAPABLE socket cannot claim the token, fail mptcp_sk_clone_init() and let the subflow fall back instead of installing a socket with mismatched token ownership.
Keep token publication after the cloned msk has its first subflow and connection list initialized. If token accept still fails, tear the cloned MPTCP socket down by open-coding the inet-level forced-close preparation, avoiding TCP-only helpers such as tcp_done() or inet_csk_prepare_forced_close() on the MPTCP master socket.
--- net/mptcp/protocol.c | 31 +++++++++++++++++---- net/mptcp/protocol.h | 4 ++- net/mptcp/subflow.c | 2 ++ net/mptcp/token.c | 61 +++++++++++++++++++++++++++++++++++++----- net/mptcp/token_test.c | 4 +-- 5 files changed, 87 insertions(+), 15 deletions(-)
net/mptcp/protocol.c +26 -5
@@ -3561,6 +3561,24 @@ static void mptcp_copy_ip_options(struct sock *newsk, const struct sock *sk)rcu_read_unlock (); } +static void mptcp_sk_clone_destroy (struct sock *nsk) +{ +struct mptcp_sock *msk =mptcp_sk (nsk); + +mptcp_release_sched (msk); +mptcp_set_state (nsk,TCP_CLOSE ); +/* inet_csk_prepare_forced_close() clears TCP sock_ops state via +* tcp_sk(), but nsk is an MPTCP master socket; keep the inet-level +* destroy preparation here. +*/ +bh_unlock_sock (nsk); +sock_put (nsk); +sock_set_flag (nsk,SOCK_DEAD ); +tcp_orphan_count_inc (); +inet_sk (nsk)->inet_num =0 ; +inet_csk_destroy_sock (nsk); +} + struct sock *mptcp_sk_clone_init(const struct sock *sk, const struct mptcp_options_received *mp_opt, struct sock *ssk,
@@ -3620,11 +3638,6 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,list_add (&subflow->node, &msk->conn_list);sock_hold (ssk); -/* new mpc subflow takes ownership of the newly -* created mptcp socket -*/ -mptcp_token_accept (subflow_req, msk); -/* set msk addresses early to ensure mptcp_pm_get_local_id() * uses the correct data */
@@ -3633,6 +3646,14 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,mptcp_rcv_space_init (msk, ssk); msk->rcvq_space.time =mptcp_stamp (); +if (!mptcp_token_accept (subflow_req, msk)) { +list_del_init (&subflow->node); +WRITE_ONCE (msk->first,NULL ); +sock_put (ssk); +mptcp_sk_clone_destroy (nsk); +return NULL ; + } +if (mp_opt->suboptions &OPTION_MPTCP_MPC_ACK )__mptcp_subflow_fully_established (msk, subflow, mp_opt);bh_unlock_sock (nsk);
net/mptcp/protocol.h +3 -1
@@ -1076,9 +1076,11 @@ static inline void mptcp_token_init_request(struct request_sock *req) }int mptcp_token_new_request (struct request_sock *req); +void mptcp_token_move_request (struct request_sock *req, +struct request_sock *new_req);void mptcp_token_destroy_request (struct request_sock *req);int mptcp_token_new_connect (struct sock *ssk); -void mptcp_token_accept (struct mptcp_subflow_request_sock *r, +bool mptcp_token_accept (struct mptcp_subflow_request_sock *r,struct mptcp_sock *msk);bool mptcp_token_exists (u32 token);struct mptcp_sock *mptcp_token_get_sock (struct net *net,u32 token);
net/mptcp/subflow.c +2 -0
@@ -56,6 +56,8 @@ void mptcp_subflow_reqsk_clone(struct request_sock *req,if (subflow_req->msk)sock_hold ((struct sock *)subflow_req->msk); + +mptcp_token_move_request (req, new_req); } static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
net/mptcp/token.c +54 -7
100 lines not on this page · raw message
net/mptcp/token_test.c +2 -2
@@ -99,7 +99,7 @@ static void mptcp_token_test_accept(struct kunit *test)KUNIT_ASSERT_EQ (test,0 ,mptcp_token_new_request ((struct request_sock *)req)); msk->token = req->token; -mptcp_token_accept (req, msk); +KUNIT_EXPECT_TRUE (test,mptcp_token_accept (req, msk));KUNIT_EXPECT_PTR_EQ (test, msk,mptcp_token_get_sock (&init_net, msk->token));/* this is now a no-op */
@@ -122,7 +122,7 @@ static void mptcp_token_test_destroyed(struct kunit *test)KUNIT_ASSERT_EQ (test,0 ,mptcp_token_new_request ((struct request_sock *)req)); msk->token = req->token; -mptcp_token_accept (req, msk); +KUNIT_EXPECT_TRUE (test,mptcp_token_accept (req, msk));/* simulate race on removal */ refcount_set (&sk->sk_refcnt,0 );
Signature · 1 line
2.43.0
Hi Ruide,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/31573413030
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/a56198440b38
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1144501
If there are some issues, you can reproduce them using the same environment as the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normalFor more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a stable tests suite when executed on a public CI like here, it is possible some reported issues are not due to your modifications. Still, do not hesitate to help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)