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

[Yuan Tan <yuantan098@gmail.com>]

On 4/16/2026 11:48 AM, Kuniyuki Iwashima wrote:
> 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
>
 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?


>>> +
>>>       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.
>>