[PATCH net v5 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk

[PATCH net v5 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk

[Ren Wei]

From: Ruide Cao <caoruide123@gmail.com>

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 own a reference.

The original and cloned requests can consequently drop the same msk
reference, leaving one request with a dangling pointer.  This manifests
as a KASAN slab-use-after-free in subflow_req_destructor().

A non-NULL subflow_req->msk means that the request owns one reference.
The third ACK can concurrently transfer the original request reference
to the child and release it, so taking an unconditional hold on the
copied pointer is unsafe.

MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all current
inet_reqsk_clone() callers run in an RCU read-side critical section.
Read the pointer from the original request, acquire a reference only if
it is still live, then re-read the original request to verify that it
still owns the same msk.  If either check fails, clear the clone pointer;
otherwise its normal destructor balances the new reference.  Mark the
ownership-transfer store with WRITE_ONCE() to match the lockless reads.

Patch 2/2 completes the clone fixup for MP_CAPABLE token ownership.  Both
patches carry the same Fixes tag and are required for stable backports.

Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc: stable@vger.kernel.org
Reported-by: Kyle Zeng <kylebot@openai.com>
Reported-by: David Lee <david.lee@trailofbits.com>
Closes: https://lore.kernel.org/all/20260804095051.715355-1-david.lee@trailofbits.com/
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: Codex:gpt-5.4
Signed-off-by: Ruide Cao <caoruide123@gmail.com>
Signed-off-by: Ren Wei <weir@nebusec.ai>
---
 include/net/mptcp.h             |  7 +++++++
 net/ipv4/inet_connection_sock.c |  4 ++++
 net/mptcp/subflow.c             | 33 ++++++++++++++++++++++++++++++++-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 71b9fc5a5796..0a02ac1ed22d 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -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 reques
 	return 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) { }
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 6257459bcee2..896f472dcba2 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -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;
 }
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8e386899ceb9..e08d1036ad78 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -47,6 +47,37 @@ 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 = mptcp_subflow_rsk(req);
+	struct mptcp_subflow_request_sock *new_subflow_req;
+	struct mptcp_sock *msk;
+
+	new_subflow_req = mptcp_subflow_rsk(new_req);
+
+	/* A non-NULL ->msk means the request owns one reference.  The clone
+	 * copied only the pointer, while the original request can concurrently
+	 * transfer its reference to the child.  Acquire a reference for the
+	 * clone, then verify that the original request still owns the same msk.
+	 * MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all clone callers run in
+	 * an RCU read-side critical section, keeping the memory stable here.
+	 */
+	msk = READ_ONCE(subflow_req->msk);
+	if (msk) {
+		struct sock *msk_sk = (struct sock *)msk;
+
+		if (!refcount_inc_not_zero(&msk_sk->sk_refcnt)) {
+			msk = NULL;
+		} else if (READ_ONCE(subflow_req->msk) != msk) {
+			sock_put(msk_sk);
+			msk = NULL;
+		}
+	}
+
+	new_subflow_req->msk = msk;
+}
+
 static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
 				  void *hmac)
 {
@@ -919,7 +950,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			}
 
 			/* move the msk reference ownership to the subflow */
-			subflow_req->msk = NULL;
+			WRITE_ONCE(subflow_req->msk, NULL);
 			ctx->conn = (struct sock *)owner;
 
 			if (subflow_use_different_sport(owner, sk)) {
-- 
2.34.1

Re: [PATCH net v5 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk

[Geliang Tang]

Hi Ren,

On Tue, 2026-09-01 at 18:33 +0800, Ren Wei wrote:
> From: Ruide Cao <caoruide123@gmail.com>
> 
> 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 own a reference.
> 
> The original and cloned requests can consequently drop the same msk
> reference, leaving one request with a dangling pointer.  This
> manifests
> as a KASAN slab-use-after-free in subflow_req_destructor().
> 
> A non-NULL subflow_req->msk means that the request owns one
> reference.
> The third ACK can concurrently transfer the original request
> reference
> to the child and release it, so taking an unconditional hold on the
> copied pointer is unsafe.
> 
> MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all current
> inet_reqsk_clone() callers run in an RCU read-side critical section.
> Read the pointer from the original request, acquire a reference only
> if
> it is still live, then re-read the original request to verify that it
> still owns the same msk.  If either check fails, clear the clone
> pointer;
> otherwise its normal destructor balances the new reference.  Mark the
> ownership-transfer store with WRITE_ONCE() to match the lockless
> reads.
> 
> Patch 2/2 completes the clone fixup for MP_CAPABLE token ownership. 
> Both
> patches carry the same Fixes tag and are required for stable
> backports.
> 
> Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at
> retransmitting SYN+ACKs.")
> Cc: stable@vger.kernel.org
> Reported-by: Kyle Zeng <kylebot@openai.com>
> Reported-by: David Lee <david.lee@trailofbits.com>
> Closes:
> https://lore.kernel.org/all/20260804095051.715355-1-david.lee@trailofbits.com/
> Reported-by: Vega <vega@nebusec.ai>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Ruide Cao <caoruide123@gmail.com>
> Signed-off-by: Ren Wei <weir@nebusec.ai>
> ---
>  include/net/mptcp.h             |  7 +++++++
>  net/ipv4/inet_connection_sock.c |  4 ++++
>  net/mptcp/subflow.c             | 33
> ++++++++++++++++++++++++++++++++-
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 71b9fc5a5796..0a02ac1ed22d 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -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 reques
>  	return 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) { }
> diff --git a/net/ipv4/inet_connection_sock.c
> b/net/ipv4/inet_connection_sock.c
> index 6257459bcee2..896f472dcba2 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -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;
>  }
>  
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8e386899ceb9..e08d1036ad78 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -47,6 +47,37 @@ 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 =
> mptcp_subflow_rsk(req);
> +	struct mptcp_subflow_request_sock *new_subflow_req;
> +	struct mptcp_sock *msk;
> +
> +	new_subflow_req = mptcp_subflow_rsk(new_req);
> +
> +	/* A non-NULL ->msk means the request owns one reference. 
> The clone
> +	 * copied only the pointer, while the original request can
> concurrently
> +	 * transfer its reference to the child.  Acquire a reference
> for the
> +	 * clone, then verify that the original request still owns
> the same msk.
> +	 * MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all clone
> callers run in
> +	 * an RCU read-side critical section, keeping the memory
> stable here.
> +	 */
> +	msk = READ_ONCE(subflow_req->msk);
> +	if (msk) {
> +		struct sock *msk_sk = (struct sock *)msk;

Here we usually name this variable "sk", and correspondingly, the
socket for the subflow is generally named "ssk".

Thanks,
-Geliang

> +
> +		if (!refcount_inc_not_zero(&msk_sk->sk_refcnt)) {
> +			msk = NULL;
> +		} else if (READ_ONCE(subflow_req->msk) != msk) {
> +			sock_put(msk_sk);
> +			msk = NULL;
> +		}
> +	}
> +
> +	new_subflow_req->msk = msk;
> +}
> +
>  static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1,
> u32 nonce2,
>  				  void *hmac)
>  {
> @@ -919,7 +950,7 @@ static struct sock *subflow_syn_recv_sock(const
> struct sock *sk,
>  			}
>  
>  			/* move the msk reference ownership to the
> subflow */
> -			subflow_req->msk = NULL;
> +			WRITE_ONCE(subflow_req->msk, NULL);
>  			ctx->conn = (struct sock *)owner;
>  
>  			if (subflow_use_different_sport(owner, sk))
> {

Re: [PATCH net v5 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk

[Ruide Cao]

Hi Geliang,

Thanks for catching, agreed.

Since no functional change, would it be OK to fix it up while
applying? Otherwise, I can respin.

Best,
Ruide

Geliang Tang <geliang@kernel.org> 于2026年9月3日周四 10:07写道：
>
> Hi Ren,
>
> On Tue, 2026-09-01 at 18:33 +0800, Ren Wei wrote:
> > From: Ruide Cao <caoruide123@gmail.com>
> >
> > 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 own a reference.
> >
> > The original and cloned requests can consequently drop the same msk
> > reference, leaving one request with a dangling pointer.  This
> > manifests
> > as a KASAN slab-use-after-free in subflow_req_destructor().
> >
> > A non-NULL subflow_req->msk means that the request owns one
> > reference.
> > The third ACK can concurrently transfer the original request
> > reference
> > to the child and release it, so taking an unconditional hold on the
> > copied pointer is unsafe.
> >
> > MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all current
> > inet_reqsk_clone() callers run in an RCU read-side critical section.
> > Read the pointer from the original request, acquire a reference only
> > if
> > it is still live, then re-read the original request to verify that it
> > still owns the same msk.  If either check fails, clear the clone
> > pointer;
> > otherwise its normal destructor balances the new reference.  Mark the
> > ownership-transfer store with WRITE_ONCE() to match the lockless
> > reads.
> >
> > Patch 2/2 completes the clone fixup for MP_CAPABLE token ownership.
> > Both
> > patches carry the same Fixes tag and are required for stable
> > backports.
> >
> > Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at
> > retransmitting SYN+ACKs.")
> > Cc: stable@vger.kernel.org
> > Reported-by: Kyle Zeng <kylebot@openai.com>
> > Reported-by: David Lee <david.lee@trailofbits.com>
> > Closes:
> > https://lore.kernel.org/all/20260804095051.715355-1-david.lee@trailofbits.com/
> > Reported-by: Vega <vega@nebusec.ai>
> > Assisted-by: Codex:gpt-5.4
> > Signed-off-by: Ruide Cao <caoruide123@gmail.com>
> > Signed-off-by: Ren Wei <weir@nebusec.ai>
> > ---
> >  include/net/mptcp.h             |  7 +++++++
> >  net/ipv4/inet_connection_sock.c |  4 ++++
> >  net/mptcp/subflow.c             | 33
> > ++++++++++++++++++++++++++++++++-
> >  3 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index 71b9fc5a5796..0a02ac1ed22d 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -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 reques
> >       return 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) { }
> > diff --git a/net/ipv4/inet_connection_sock.c
> > b/net/ipv4/inet_connection_sock.c
> > index 6257459bcee2..896f472dcba2 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -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;
> >  }
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 8e386899ceb9..e08d1036ad78 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -47,6 +47,37 @@ 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 =
> > mptcp_subflow_rsk(req);
> > +     struct mptcp_subflow_request_sock *new_subflow_req;
> > +     struct mptcp_sock *msk;
> > +
> > +     new_subflow_req = mptcp_subflow_rsk(new_req);
> > +
> > +     /* A non-NULL ->msk means the request owns one reference.
> > The clone
> > +      * copied only the pointer, while the original request can
> > concurrently
> > +      * transfer its reference to the child.  Acquire a reference
> > for the
> > +      * clone, then verify that the original request still owns
> > the same msk.
> > +      * MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all clone
> > callers run in
> > +      * an RCU read-side critical section, keeping the memory
> > stable here.
> > +      */
> > +     msk = READ_ONCE(subflow_req->msk);
> > +     if (msk) {
> > +             struct sock *msk_sk = (struct sock *)msk;
>
> Here we usually name this variable "sk", and correspondingly, the
> socket for the subflow is generally named "ssk".
>
> Thanks,
> -Geliang
>
> > +
> > +             if (!refcount_inc_not_zero(&msk_sk->sk_refcnt)) {
> > +                     msk = NULL;
> > +             } else if (READ_ONCE(subflow_req->msk) != msk) {
> > +                     sock_put(msk_sk);
> > +                     msk = NULL;
> > +             }
> > +     }
> > +
> > +     new_subflow_req->msk = msk;
> > +}
> > +
> >  static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1,
> > u32 nonce2,
> >                                 void *hmac)
> >  {
> > @@ -919,7 +950,7 @@ static struct sock *subflow_syn_recv_sock(const
> > struct sock *sk,
> >                       }
> >
> >                       /* move the msk reference ownership to the
> > subflow */
> > -                     subflow_req->msk = NULL;
> > +                     WRITE_ONCE(subflow_req->msk, NULL);
> >                       ctx->conn = (struct sock *)owner;
> >
> >                       if (subflow_use_different_sport(owner, sk))
> > {

Re: [PATCH net v5 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk

[netdev-bot+sashiko]

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

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

- [Low] mptcp_subflow_reqsk_clone() (net/mptcp/subflow.c:50-79) performs
  a lockless dereference of a possibly recycled SLAB_TYPESAFE_BY_RCU…
- [Low] mptcp_subflow_reqsk_clone() (net/mptcp/subflow.c:66-76) uses the
  fully relaxed refcount_inc_not_zero() to take a reference on an object…

Pre-existing issues:
- [Low] Incomplete clone fixup for MP_CAPABLE token ownership:
  inet_reqsk_clone() byte-copies the whole mptcp_subflow_request_sock…

--

mptcp: hold MP_JOIN msk ref when cloning reqsk

TCP request migration byte-copies subflow_req->msk into the cloned request
without taking a reference, so both requests can drop the same msk
reference.  This adds mptcp_subflow_reqsk_clone(), called from
inet_reqsk_clone(), which conditionally acquires a reference for the clone
and re-validates the pointer, and marks the ownership-transfer store in
subflow_syn_recv_sock() with WRITE_ONCE().

Closes: https://lore.kernel.org/all/20260804095051.715355-1-david.lee@trailofbits.com/

> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 485d55b66ea67..8e063454fcf6e 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -219,6 +219,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);

[Severity: Low]
Would it be worth documenting the calling-context requirement here?

The safety of mptcp_subflow_reqsk_clone() depends on the caller being in an
RCU read-side critical section, because the msk it dereferences comes from a
SLAB_TYPESAFE_BY_RCU cache.  That requirement is only stated in a comment
inside net/mptcp/subflow.c; the prototype and the !CONFIG_MPTCP stub in
include/net/mptcp.h say nothing about it, and there is no
RCU_LOCKDEP_WARN() or lockdep_assert_in_softirq() in the helper itself.

All three inet_reqsk_clone() call sites satisfy it only implicitly:
reqsk_timer_handler() runs in the rsk_timer softirq,
inet_csk_complete_hashdance() runs in NET_RX softirq, and
inet_csk_listen_stop() is inside local_bh_disable().  In the last case the
explicit rcu_read_lock() is taken only after inet_reqsk_clone() returns:

	nreq = inet_reqsk_clone(req, nsk);
	if (nreq) {
		refcount_set(&nreq->rsk_refcnt, 1);

		rcu_read_lock();

which reads as though the clone needs no RCU protection at all.  A comment
on the prototype plus a lockdep assertion in the helper would keep this from
silently breaking if a future caller runs in process context.

[ ... ]

> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 6257459bcee24..896f472dcba29 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c

[ ... ]

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

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index af81ad5e699d1..f13011f591b78 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -47,6 +47,37 @@ 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 = mptcp_subflow_rsk(req);
> +	struct mptcp_subflow_request_sock *new_subflow_req;
> +	struct mptcp_sock *msk;
> +
> +	new_subflow_req = mptcp_subflow_rsk(new_req);
> +
> +	/* A non-NULL ->msk means the request owns one reference.  The clone
> +	 * copied only the pointer, while the original request can concurrently
> +	 * transfer its reference to the child.  Acquire a reference for the
> +	 * clone, then verify that the original request still owns the same msk.
> +	 * MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all clone callers run in
> +	 * an RCU read-side critical section, keeping the memory stable here.
> +	 */
> +	msk = READ_ONCE(subflow_req->msk);
> +	if (msk) {
> +		struct sock *msk_sk = (struct sock *)msk;
> +
> +		if (!refcount_inc_not_zero(&msk_sk->sk_refcnt)) {
> +			msk = NULL;
> +		} else if (READ_ONCE(subflow_req->msk) != msk) {
> +			sock_put(msk_sk);
> +			msk = NULL;
> +		}
> +	}

[Severity: Low]
Should this use refcount_inc_not_zero_acquire() instead?

The increment here is followed by a validation load that re-reads
subflow_req->msk and compares addresses.  include/linux/refcount.h describes
refcount_inc_not_zero_acquire() for exactly that pattern:

 * This function should be used when memory occupied by the object might be
 * reused to store another object -- consider SLAB_TYPESAFE_BY_RCU.

refcount_inc_not_zero() provides no memory ordering and only orders future
stores through the control dependency, so the second READ_ONCE() is not
ordered after the refcount RMW.  Can the second read then observe the stale
non-NULL pointer while the increment landed on a freshly reused mptcp_sock
at the same address, letting the address comparison pass?

If so, the clone would own a reference to an unrelated live msk, and the
third ACK would compute the HMAC against the wrong keys in
subflow_hmac_valid(), rejecting the join with
SUBFLOW_REQ_INC_STATS(MPTCP_MIB_JOINACKMAC).

This needs load-load reordering on a weakly ordered architecture plus
immediate slab reuse, so no concrete failing execution is demonstrated here,
and the relaxed idiom is what the rest of net/ currently uses (for example
__inet_lookup_established() and mptcp_token_get_sock()).  An explicit
smp_rmb() before the recheck would also cover it.

> +
> +	new_subflow_req->msk = msk;
> +}

[Severity: Low]
This isn't a bug introduced by this patch, but with only this patch applied
the clone fixup is incomplete for MP_CAPABLE token state.

inet_reqsk_clone() copies req->rsk_ops->obj_size bytes, so the whole
mptcp_subflow_request_sock is duplicated including ->token and the hashed
->token_node, and reqsk_migrate_reset() clears only saved_syn and the ireq
option pointers.  This helper repairs ->msk only.

After a successful migration the original request is freed and unlinks the
shared token:

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

The surviving clone is then left with a token_node that looks hashed but is
no longer reachable, and mptcp_token_accept() trips its
WARN_ON_ONCE(pos != req) while bucket->chain_len accounting drifts.

The follow-up patch in this series, "mptcp: fix MP_CAPABLE token migration
when cloning reqsk", adds mptcp_token_move_request() and calls it from here,
and converts mptcp_token_accept()/mptcp_token_destroy_request() to
lock-rechecked, non-warning variants, so this is only a property of the
intermediate state.  Given both patches carry the same Fixes tag and are
tagged for stable, is it worth noting in the changelog that applying 1/2
alone leaves that reachable WARN_ON_ONCE in place?

[ ... ]

> @@ -923,7 +954,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>  			}
>  
>  			/* move the msk reference ownership to the subflow */
> -			subflow_req->msk = NULL;
> +			WRITE_ONCE(subflow_req->msk, NULL);
>  			ctx->conn = (struct sock *)owner;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788202924.git.caoruide123%40gmail.com