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

[Matthieu Baerts <matttbe@kernel.org>]

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.