Skip to contentSkip to conversation

mptcp: hold subflow request owners when cloning reqsk

PATCH In review Ren Wei 5 participants 11 messages Apr 15 – May 7 1 message not in the archive

Subsystems (4)
Networking Network drivers TCP Networking MPTCP
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ren Wei

author
#1 lore raw

PATCH 1/1 mptcp: hold subflow request owners 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 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 [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)
include/net/request_sock.h +2 -0
index 5a9c826a7092..560e464c400f 100644
@@ -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 {
net/ipv4/inet_connection_sock.c +3 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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;
 }
net/mptcp/subflow.c +13 -0
index 4ff5863aa9fd..5f4069647822 100644
@@ -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");
Signature · 1 line
2.34.1
#2 lore raw

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): Unstable: 1 failed test(s): bpftest_test_progs_mptcp ⚠️
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/24469279829

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b360c31cd841
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1081484

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-normal

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

maintainer Reviewed-by on v1 1/1
#3 lore raw
In reply to Ren Wei

Hi Ren,

Quoted 6 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

Quoted 8 lines

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?

Quoted 20 lines
Fixes c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)
include/net/request_sock.h +0 -0
index 5a9c826a7092..560e464c400f 100644

include/net/request_sock.h struct request_sock_ops { · line 40

Quoted 5 lines
include/net/request_sock.h +2 -0
@@ -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.

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 10 lines
include/net/request_sock.h +0 -0
 };
 
 struct saved_syn {
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 11 lines
net/ipv4/inet_connection_sock.c +3 -0
+	if (req->rsk_ops->init_clone)
+		req->rsk_ops->init_clone(req, nreq);
+
 	return nreq;
 }
net/mptcp/subflow.c +0 -0
index 4ff5863aa9fd..5f4069647822 100644

net/mptcp/subflow.c static void subflow_req_destructor(struct request_sock *req) · line 55

Quoted 9 lines
net/mptcp/subflow.c +6 -0
@@ -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) <[email protected]>

Cheers,
Matt

Signature · 1 line
Sponsored by the NGI0 Core fund.
reviewer
#4 lore raw
Quoted 68 lines · Matthieu Baerts

Hi Ren,

Quoted 6 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

Quoted 8 lines

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?

Quoted 20 lines
Fixes c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)
include/net/request_sock.h +0 -0
index 5a9c826a7092..560e464c400f 100644

include/net/request_sock.h struct request_sock_ops { · line 40

Quoted 5 lines
include/net/request_sock.h +2 -0
@@ -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.

Quoted 2 lines
include/net/request_sock.h +0 -0
 };
Quoted 14 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 8 lines
include/net/request_sock.h +0 -0
 struct saved_syn {
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     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

Quoted 35 lines
Quoted 9 lines
net/ipv4/inet_connection_sock.c +1 -0
+
      return nreq;
 }
net/mptcp/subflow.c +0 -0
index 4ff5863aa9fd..5f4069647822 100644

net/mptcp/subflow.c static void subflow_req_destructor(struct request_sock *req) · line 55

Quoted 9 lines
net/mptcp/subflow.c +6 -0
@@ -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) <[email protected]>

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

maintainer
#5 lore raw

Hi Kuniyuki,

Thank you for your reply!

16 Apr 2026 20:48:58 Kuniyuki Iwashima <[email protected]>:

Quoted 19 lines · Matthieu Baerts
Quoted 18 lines

Hi Ren,

Quoted 14 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

(...)

Quoted 14 lines
Quoted 13 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 7 lines
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     if (req->rsk_ops->init_clone)
+             req->rsk_ops->init_clone(req, nreq);
Quoted 6 lines

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

reviewer
#6 lore raw
Quoted 50 lines · Matthieu Baerts

Hi Kuniyuki,

Thank you for your reply!

16 Apr 2026 20:48:58 Kuniyuki Iwashima <[email protected]>:

Quoted 19 lines · Matthieu Baerts
Quoted 18 lines

Hi Ren,

Quoted 14 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

(...)

Quoted 14 lines
Quoted 13 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 7 lines
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     if (req->rsk_ops->init_clone)
+             req->rsk_ops->init_clone(req, nreq);
Quoted 6 lines

I think a simple direct call is better.

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

#endif

Quoted 6 lines

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.

Quoted 8 lines
      if TFO
          ...
      if MPTCP (+ifdef)
          ...

}

Cheers,
Matt

#7 lore raw
In reply to Kuniyuki Iwashima
Quoted 77 lines · Kuniyuki Iwashima
Quoted 76 lines · Matthieu Baerts

Hi Ren,

Quoted 6 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

Quoted 8 lines

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?

Quoted 20 lines
Fixes c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)
include/net/request_sock.h +0 -0
index 5a9c826a7092..560e464c400f 100644

include/net/request_sock.h struct request_sock_ops { · line 40

Quoted 5 lines
include/net/request_sock.h +2 -0
@@ -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.

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 10 lines
include/net/request_sock.h +0 -0
 };
 
 struct saved_syn {
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 9 lines
Quoted 2 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 958

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     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

 Thank you very much for your suggestion. We will use this approach in the next version of the patch. Would you like us to add your Suggested-by tag?

Quoted 34 lines
Quoted 34 lines
Quoted 9 lines
net/ipv4/inet_connection_sock.c +1 -0
+
      return nreq;
 }
net/mptcp/subflow.c +0 -0
index 4ff5863aa9fd..5f4069647822 100644

net/mptcp/subflow.c static void subflow_req_destructor(struct request_sock *req) · line 55

Quoted 9 lines
net/mptcp/subflow.c +6 -0
@@ -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) <[email protected]>

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

maintainer
#8 lore raw

Hi Yuan Tan,

Quoted 20 lines · Yuan Tan
Quoted 18 lines · Kuniyuki Iwashima
Quoted 17 lines · Matthieu Baerts

Hi Ren,

Quoted 14 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

(...)

Quoted 14 lines
Quoted 14 lines
Quoted 12 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 7 lines
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     if (req->rsk_ops->init_clone)
+             req->rsk_ops->init_clone(req, nreq);

I think a simple direct call is better.

Quoted 8 lines
Quoted 5 lines
#ifdef CONFIG_MPTCP
    if (tcp_rsk(req)->is_mptcp)
        mptcp_reqsk_clone(nreq);

#endif

 Thank you very much for your suggestion. We will use this approach in the next version of the patch. Would you like us to add your Suggested-by tag?

No need to add a Suggested-by tag: this tag is used when the whole patch idea has been suggested by someone. That's not the case here: we only proposed a small modification in the code, without changing the idea.

https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

Cheers,
Matt

Signature · 1 line
Sponsored by the NGI0 Core fund.
maintainer
#9 lore raw
In reply to Yuan Tan

Hi Yuan, Ren, Ruide,

(-netdev list)

Quoted 83 lines · Yuan Tan
Quoted 81 lines · Kuniyuki Iwashima
Quoted 65 lines · Matthieu Baerts

Hi Ren,

Quoted 6 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

Quoted 8 lines

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?

Quoted 20 lines
Fixes c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)
include/net/request_sock.h +0 -0
index 5a9c826a7092..560e464c400f 100644

include/net/request_sock.h struct request_sock_ops { · line 40

Quoted 5 lines
include/net/request_sock.h +2 -0
@@ -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.

Quoted 2 lines
include/net/request_sock.h +0 -0
 };
Quoted 13 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 8 lines
include/net/request_sock.h +0 -0
 struct saved_syn {
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     if (req->rsk_ops->init_clone)
+             req->rsk_ops->init_clone(req, nreq);

I think a simple direct call is better.

Quoted 8 lines
Quoted 5 lines
#ifdef CONFIG_MPTCP
    if (tcp_rsk(req)->is_mptcp)
        mptcp_reqsk_clone(nreq);

#endif

 Thank you very much for your suggestion. We will use this approach in the next version of the patch. Would you like us to add your Suggested-by tag?

For a question of organisation, do you still plan to send a v2 addressing the review comments?

Cheers,
Matt

Signature · 1 line
Sponsored by the NGI0 Core fund.
#10 lore raw
Quoted 97 lines · Matthieu Baerts

Hi Yuan, Ren, Ruide,

(-netdev list)

Quoted 83 lines · Yuan Tan
Quoted 81 lines · Kuniyuki Iwashima
Quoted 65 lines · Matthieu Baerts

Hi Ren,

Quoted 6 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

Quoted 8 lines

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?

Quoted 20 lines
Fixes c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)
include/net/request_sock.h +0 -0
index 5a9c826a7092..560e464c400f 100644

include/net/request_sock.h struct request_sock_ops { · line 40

Quoted 5 lines
include/net/request_sock.h +2 -0
@@ -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.

Quoted 2 lines
include/net/request_sock.h +0 -0
 };
Quoted 13 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 8 lines
include/net/request_sock.h +0 -0
 struct saved_syn {
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     if (req->rsk_ops->init_clone)
+             req->rsk_ops->init_clone(req, nreq);

I think a simple direct call is better.

Quoted 8 lines
Quoted 5 lines
#ifdef CONFIG_MPTCP
    if (tcp_rsk(req)->is_mptcp)
        mptcp_reqsk_clone(nreq);

#endif

Thank you very much for your suggestion. We will use this approach in
the next version of the patch. Would you like us to add your Suggested-by tag?

Quoted 8 lines

For a question of organisation, do you still plan to send a v2 addressing the review comments?

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

Sorry for the delay. Please forgive us that we are all working on the patches part-time :)

Ruide will send v2 this weekend.

maintainer
#11 lore raw

Hi Yuan,

Quoted 101 lines · Yuan Tan
Quoted 100 lines · Matthieu Baerts

Hi Yuan, Ren, Ruide,

(-netdev list)

Quoted 83 lines · Yuan Tan
Quoted 81 lines · Kuniyuki Iwashima
Quoted 65 lines · Matthieu Baerts

Hi Ren,

Quoted 6 lines · Ren Wei

From: Ruide Cao <[email protected]>

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.

Quoted 8 lines

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?

Quoted 20 lines
Fixes c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 include/net/request_sock.h      |  2 ++
 net/ipv4/inet_connection_sock.c |  3 +++
 net/mptcp/subflow.c             | 13 +++++++++++++
 3 files changed, 18 insertions(+)
include/net/request_sock.h +0 -0
index 5a9c826a7092..560e464c400f 100644

include/net/request_sock.h struct request_sock_ops { · line 40

Quoted 5 lines
include/net/request_sock.h +2 -0
@@ -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.

Quoted 2 lines
include/net/request_sock.h +0 -0
 };
Quoted 13 lines

net/ipv4/inet_connection_sock.c static struct request_sock *inet_reqsk_clone(struct request_sock *req, · line 955

Quoted 8 lines
include/net/request_sock.h +0 -0
 struct saved_syn {
net/ipv4/inet_connection_sock.c +0 -0
index e961936b6be7..140a9e96ad58 100644
@@ -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))

Quoted 2 lines
net/ipv4/inet_connection_sock.c +2 -0
+     if (req->rsk_ops->init_clone)
+             req->rsk_ops->init_clone(req, nreq);

I think a simple direct call is better.

Quoted 8 lines
Quoted 5 lines
#ifdef CONFIG_MPTCP
    if (tcp_rsk(req)->is_mptcp)
        mptcp_reqsk_clone(nreq);

#endif

Thank you very much for your suggestion. We will use this approach in
the next version of the patch. Would you like us to add your Suggested-by tag?

For a question of organisation, do you still plan to send a v2 addressing the review comments?

Quoted 8 lines
Quoted 5 lines

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

Sorry for the delay. Please forgive us that we are all working on the patches part-time :)

No problem. This patch is not urgent, but because you originally cced the security ML, it's adding a bit more pressure to release a fix "quickly".

Quoted 1 line

Ruide will send v2 this weekend.

Sounds good to me!

Cheers,
Matt

Signature · 1 line
Sponsored by the NGI0 Core fund.