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

[Geliang Tang <geliang@kernel.org>]

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))
> {