[PATCH net 1/1] mptcp: hold subflow request owners when cloning reqsk

[PATCH net 1/1] mptcp: hold subflow request owners 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 raw-copies
subflow_req->msk, but the cloned request does not take a new reference.

Both the original and the cloned request can later drop the same msk in
subflow_req_destructor(), and a migrated request may keep a dangling msk
pointer after the original owner has already been released.

Add a request_sock clone callback and let MPTCP grab a reference for cloned
subflow requests that carry an msk. This keeps ownership balanced across
both successful migrations and failed clone/insert paths without changing
other protocols.

Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Ruide Cao <caoruide123@gmail.com>
Tested-by: Ren Wei <enjou1224z@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)

diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 5a9c826a7092..560e464c400f 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -36,6 +36,8 @@ struct request_sock_ops {
 				      struct sk_buff *skb,
 				      enum sk_rst_reason reason);
 	void		(*destructor)(struct request_sock *req);
+	void		(*init_clone)(const struct request_sock *req,
+				      struct request_sock *new_req);
 };
 
 struct saved_syn {
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index e961936b6be7..140a9e96ad58 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -954,6 +954,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
 	if (sk->sk_protocol == IPPROTO_TCP && tcp_rsk(nreq)->tfo_listener)
 		rcu_assign_pointer(tcp_sk(nreq->sk)->fastopen_rsk, nreq);
 
+	if (req->rsk_ops->init_clone)
+		req->rsk_ops->init_clone(req, nreq);
+
 	return nreq;
 }
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 4ff5863aa9fd..5f4069647822 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)
 	mptcp_token_destroy_request(req);
 }
 
+static void subflow_req_clone(const struct request_sock *req,
+			      struct request_sock *new_req)
+{
+	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(new_req);
+
+	(void)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)
 {
@@ -2143,6 +2154,7 @@ void __init mptcp_subflow_init(void)
 	mptcp_subflow_v4_request_sock_ops = tcp_request_sock_ops;
 	mptcp_subflow_v4_request_sock_ops.slab_name = "request_sock_subflow_v4";
 	mptcp_subflow_v4_request_sock_ops.destructor = subflow_v4_req_destructor;
+	mptcp_subflow_v4_request_sock_ops.init_clone = subflow_req_clone;
 
 	if (subflow_ops_init(&mptcp_subflow_v4_request_sock_ops) != 0)
 		panic("MPTCP: failed to init subflow v4 request sock ops\n");
@@ -2184,6 +2196,7 @@ void __init mptcp_subflow_v6_init(void)
 	mptcp_subflow_v6_request_sock_ops = tcp6_request_sock_ops;
 	mptcp_subflow_v6_request_sock_ops.slab_name = "request_sock_subflow_v6";
 	mptcp_subflow_v6_request_sock_ops.destructor = subflow_v6_req_destructor;
+	mptcp_subflow_v6_request_sock_ops.init_clone = subflow_req_clone;
 
 	if (subflow_ops_init(&mptcp_subflow_v6_request_sock_ops) != 0)
 		panic("MPTCP: failed to init subflow v6 request sock ops\n");
-- 
2.34.1

Re: [PATCH net 1/1] mptcp: hold subflow request owners when cloning reqsk

[Matthieu Baerts]

Hi Ren,

On 15/04/2026 11:31, 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 raw-copies
> subflow_req->msk, but the cloned request does not take a new reference.
> 
> Both the original and the cloned request can later drop the same msk in
> subflow_req_destructor(), and a migrated request may keep a dangling msk
> pointer after the original owner has already been released.
> 
> Add a request_sock clone callback and let MPTCP grab a reference for cloned
> subflow requests that carry an msk. This keeps ownership balanced across
> both successful migrations and failed clone/insert paths without changing
> other protocols.

Thank you for this patch!

Indeed, it looks like this path has not been covered by the MPTCP test
suite so far.

By chance, do you have any simpler reproducer using packetdrill? (the
MPTCP fork)

  https://github.com/multipath-tcp/packetdrill

Also, I see Sashiko is pointing to a potential issue with MP_CAPABLE and
the token, also only visible with net.ipv4.tcp_migrate_req=1:

https://sashiko.dev/#/patchset/86e2514b533bf4d55d4aa2fdbf1404022e8c9430.1776149210.git.caoruide123%40gmail.com

Are you also looking at that?

> Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Ruide Cao <caoruide123@gmail.com>
> Tested-by: Ren Wei <enjou1224z@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
>  include/net/request_sock.h      |  2 ++
>  net/ipv4/inet_connection_sock.c |  3 +++
>  net/mptcp/subflow.c             | 13 +++++++++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 5a9c826a7092..560e464c400f 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -36,6 +36,8 @@ struct request_sock_ops {
>  				      struct sk_buff *skb,
>  				      enum sk_rst_reason reason);
>  	void		(*destructor)(struct request_sock *req);
> +	void		(*init_clone)(const struct request_sock *req,
> +				      struct request_sock *new_req);

@TCP/INET maintainers: are you OK with this new function pointer?

Currently, MPTCP is the only user, and "req" is not used, see below.

>  };
>  
>  struct saved_syn {
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index e961936b6be7..140a9e96ad58 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -954,6 +954,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
>  	if (sk->sk_protocol == IPPROTO_TCP && tcp_rsk(nreq)->tfo_listener)
>  		rcu_assign_pointer(tcp_sk(nreq->sk)->fastopen_rsk, nreq);

(Maybe TCP with fastopen could be this other user to call
rcu_assign_pointer()? (net-next material))

> +	if (req->rsk_ops->init_clone)
> +		req->rsk_ops->init_clone(req, nreq);
> +
>  	return nreq;
>  }
>  
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 4ff5863aa9fd..5f4069647822 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)
>  	mptcp_token_destroy_request(req);
>  }
>  
> +static void subflow_req_clone(const struct request_sock *req,
> +			      struct request_sock *new_req)
> +{
> +	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(new_req);
> +
> +	(void)req;


Note: if 'req' is not used, and MPTCP is currently the only user of this
new init_clone() callback, perhaps enough to pass only 'new_req' for the
moment? 'req' can be added later if other users need it, no?

With only init_close(nreq) in a v2, or if TCP maintainers prefer to keep
it, feel free to add:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>


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

Re: [PATCH net 1/1] mptcp: hold subflow request owners when cloning reqsk

[Kuniyuki Iwashima]

On Thu, Apr 16, 2026 at 10:45 AM Matthieu Baerts <matttbe@kernel.org> wrote:
>
> Hi Ren,
>
> On 15/04/2026 11:31, 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 raw-copies
> > subflow_req->msk, but the cloned request does not take a new reference.
> >
> > Both the original and the cloned request can later drop the same msk in
> > subflow_req_destructor(), and a migrated request may keep a dangling msk
> > pointer after the original owner has already been released.
> >
> > Add a request_sock clone callback and let MPTCP grab a reference for cloned
> > subflow requests that carry an msk. This keeps ownership balanced across
> > both successful migrations and failed clone/insert paths without changing
> > other protocols.
>
> Thank you for this patch!
>
> Indeed, it looks like this path has not been covered by the MPTCP test
> suite so far.
>
> By chance, do you have any simpler reproducer using packetdrill? (the
> MPTCP fork)
>
>   https://github.com/multipath-tcp/packetdrill
>
> Also, I see Sashiko is pointing to a potential issue with MP_CAPABLE and
> the token, also only visible with net.ipv4.tcp_migrate_req=1:
>
> https://sashiko.dev/#/patchset/86e2514b533bf4d55d4aa2fdbf1404022e8c9430.1776149210.git.caoruide123%40gmail.com
>
> Are you also looking at that?
>
> > Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
> > Cc: stable@kernel.org
> > Reported-by: Yuan Tan <yuantan098@gmail.com>
> > Reported-by: Yifan Wu <yifanwucs@gmail.com>
> > Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> > Reported-by: Xin Liu <bird@lzu.edu.cn>
> > Signed-off-by: Ruide Cao <caoruide123@gmail.com>
> > Tested-by: Ren Wei <enjou1224z@gmail.com>
> > Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> > ---
> >  include/net/request_sock.h      |  2 ++
> >  net/ipv4/inet_connection_sock.c |  3 +++
> >  net/mptcp/subflow.c             | 13 +++++++++++++
> >  3 files changed, 18 insertions(+)
> >
> > diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> > index 5a9c826a7092..560e464c400f 100644
> > --- a/include/net/request_sock.h
> > +++ b/include/net/request_sock.h
> > @@ -36,6 +36,8 @@ struct request_sock_ops {
> >                                     struct sk_buff *skb,
> >                                     enum sk_rst_reason reason);
> >       void            (*destructor)(struct request_sock *req);
> > +     void            (*init_clone)(const struct request_sock *req,
> > +                                   struct request_sock *new_req);
>
> @TCP/INET maintainers: are you OK with this new function pointer?
>
> Currently, MPTCP is the only user, and "req" is not used, see below.
>
> >  };
> >
> >  struct saved_syn {
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index e961936b6be7..140a9e96ad58 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -954,6 +954,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
> >       if (sk->sk_protocol == IPPROTO_TCP && tcp_rsk(nreq)->tfo_listener)
> >               rcu_assign_pointer(tcp_sk(nreq->sk)->fastopen_rsk, nreq);
>
> (Maybe TCP with fastopen could be this other user to call
> rcu_assign_pointer()? (net-next material))
>
> > +     if (req->rsk_ops->init_clone)
> > +             req->rsk_ops->init_clone(req, nreq);

I think a simple direct call is better.

#ifdef CONFIG_MPTCP
    if (tcp_rsk(req)->is_mptcp)
        mptcp_reqsk_clone(nreq);
#endif


> > +
> >       return nreq;
> >  }
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 4ff5863aa9fd..5f4069647822 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -47,6 +47,17 @@ static void subflow_req_destructor(struct request_sock *req)
> >       mptcp_token_destroy_request(req);
> >  }
> >
> > +static void subflow_req_clone(const struct request_sock *req,
> > +                           struct request_sock *new_req)
> > +{
> > +     struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(new_req);
> > +
> > +     (void)req;
>
>
> Note: if 'req' is not used, and MPTCP is currently the only user of this
> new init_clone() callback, perhaps enough to pass only 'new_req' for the
> moment? 'req' can be added later if other users need it, no?
>
> With only init_close(nreq) in a v2, or if TCP maintainers prefer to keep
> it, feel free to add:
>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
> Cheers,
> Matt
> --
> Sponsored by the NGI0 Core fund.
>

Re: [PATCH net 1/1] mptcp: hold subflow request owners when cloning reqsk

[Matthieu Baerts]

Hi Kuniyuki,

Thank you for your reply!

16 Apr 2026 20:48:58 Kuniyuki Iwashima <kuniyu@google.com>:

> On Thu, Apr 16, 2026 at 10:45 AM Matthieu Baerts <matttbe@kernel.org> wrote:
>>
>> Hi Ren,
>>
>> On 15/04/2026 11:31, 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 raw-copies
>>> subflow_req->msk, but the cloned request does not take a new reference.
>>>
>>> Both the original and the cloned request can later drop the same msk in
>>> subflow_req_destructor(), and a migrated request may keep a dangling msk
>>> pointer after the original owner has already been released.
>>>
>>> Add a request_sock clone callback and let MPTCP grab a reference for cloned
>>> subflow requests that carry an msk. This keeps ownership balanced across
>>> both successful migrations and failed clone/insert paths without changing
>>> other protocols.

(...)

>>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
>>> index e961936b6be7..140a9e96ad58 100644
>>> --- a/net/ipv4/inet_connection_sock.c
>>> +++ b/net/ipv4/inet_connection_sock.c
>>> @@ -954,6 +954,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
>>>       if (sk->sk_protocol == IPPROTO_TCP && tcp_rsk(nreq)->tfo_listener)
>>>               rcu_assign_pointer(tcp_sk(nreq->sk)->fastopen_rsk, nreq);
>>
>> (Maybe TCP with fastopen could be this other user to call
>> rcu_assign_pointer()? (net-next material))
>>
>>> +     if (req->rsk_ops->init_clone)
>>> +             req->rsk_ops->init_clone(req, nreq);
>
> I think a simple direct call is better.
>
> #ifdef CONFIG_MPTCP
>     if (tcp_rsk(req)->is_mptcp)
>         mptcp_reqsk_clone(nreq);
> #endif

Fine by me!

I guess it is needed to check the protocol, similar to what is fine with
TFO above:

  if (sk->sk_protocol == IPPROTO_TCP) {
      if TFO
          ...
      if MPTCP (+ifdef)
          ...
  }

Cheers,
Matt

Re: [PATCH net 1/1] mptcp: hold subflow request owners when cloning reqsk

[Kuniyuki Iwashima]

On Thu, Apr 16, 2026 at 2:18 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>
> Hi Kuniyuki,
>
> Thank you for your reply!
>
> 16 Apr 2026 20:48:58 Kuniyuki Iwashima <kuniyu@google.com>:
>
> > On Thu, Apr 16, 2026 at 10:45 AM Matthieu Baerts <matttbe@kernel.org> wrote:
> >>
> >> Hi Ren,
> >>
> >> On 15/04/2026 11:31, 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 raw-copies
> >>> subflow_req->msk, but the cloned request does not take a new reference.
> >>>
> >>> Both the original and the cloned request can later drop the same msk in
> >>> subflow_req_destructor(), and a migrated request may keep a dangling msk
> >>> pointer after the original owner has already been released.
> >>>
> >>> Add a request_sock clone callback and let MPTCP grab a reference for cloned
> >>> subflow requests that carry an msk. This keeps ownership balanced across
> >>> both successful migrations and failed clone/insert paths without changing
> >>> other protocols.
>
> (...)
>
> >>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> >>> index e961936b6be7..140a9e96ad58 100644
> >>> --- a/net/ipv4/inet_connection_sock.c
> >>> +++ b/net/ipv4/inet_connection_sock.c
> >>> @@ -954,6 +954,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
> >>>       if (sk->sk_protocol == IPPROTO_TCP && tcp_rsk(nreq)->tfo_listener)
> >>>               rcu_assign_pointer(tcp_sk(nreq->sk)->fastopen_rsk, nreq);
> >>
> >> (Maybe TCP with fastopen could be this other user to call
> >> rcu_assign_pointer()? (net-next material))
> >>
> >>> +     if (req->rsk_ops->init_clone)
> >>> +             req->rsk_ops->init_clone(req, nreq);
> >
> > I think a simple direct call is better.
> >
> > #ifdef CONFIG_MPTCP
> >     if (tcp_rsk(req)->is_mptcp)
> >         mptcp_reqsk_clone(nreq);
> > #endif
>
> Fine by me!
>
> I guess it is needed to check the protocol, similar to what is fine with
> TFO above:
>
>   if (sk->sk_protocol == IPPROTO_TCP) {

I don't remember why I added this check for TFO,
but I think this is not needed.  The migration feature was
not supported by DCCP and it's already removed anyway.
I'll remove the check once net-next opens.


>       if TFO
>           ...
>       if MPTCP (+ifdef)
>           ...
>   }
>
> Cheers,
> Matt