Re: [PATCH net v3 2/2] mptcp: fix MP_CAPABLE token migration when cloning reqsk

Hi Matt,

Thank you for your comment. We are adding a
mptcp_sk_clone_destroy(nsk) in our v4.

Regarding the comments from Clashiko, my reply is as follows:

Comment 1:

> This isn't a bug introduced by this patch, but does this clone helper also
> need to migrate the MPTCP token_node for MP_CAPABLE requests?

I agree. Patch 1 only fixes the MP_JOIN msk reference ownership problem, but
the same request clone helper also needs to handle MP_CAPABLE token_node
ownership.

Comment 2:

> Is it ok to pass an MPTCP socket to tcp_done() in this failure path?

I agree that passing nsk to tcp_done() was wrong. nsk is an MPTCP master
socket, not a TCP socket. tcp_done() and inet_csk_prepare_forced_close() are
TCP-only here because they use tcp_sk(), so they can interpret the private
MPTCP socket layout as struct tcp_sock after the shared inet_connection_sock
header.

We are reviewing v4, and will send it out soon.

Best,
Ruide


Matthieu Baerts <[email protected]> 于2026年8月10日周一 02:03写道：
>
> Hi Ren, Ruide,
>
> On 06/08/2026 13:14, Ren Wei wrote:
> > From: Ruide Cao <[email protected]>
> >
> > TCP request migration clones pending request sockets with
> > inet_reqsk_clone().  For MPTCP MP_CAPABLE requests this byte-copies the
> > token_node hlist state into the clone even though the token table still
> > names the original request.
> >
> > Moving the token only after inet_ehash_insert() succeeds leaves a window
> > where the cloned request is already globally visible from the ehash but
> > the token table still points at the original request.  Reordering the
> > move after clone but before ehash exposure closes that window, but
> > concurrent RX can still race on the old request and observe that the
> > token was already moved.
> >
> > Move MP_CAPABLE token request ownership during MPTCP request cloning
> > under the token bucket lock.  Make mptcp_token_accept() and
> > mptcp_token_destroy_request() re-check token_node under the same lock and
> > treat an already moved or removed request as a normal race instead of
> > warning.
> >
> > If the passive MP_CAPABLE socket cannot claim the token, fail
> > mptcp_sk_clone_init() and let the subflow fall back instead of
> > installing a socket with mismatched token ownership.
>
> Thank you for the new version!
>
> I have one major comment, please see below:
> > Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
> > Cc: [email protected]
> > Reported-by: Vega <[email protected]>
> > Assisted-by: Codex:gpt-5.4
> > Reported-by: Sashiko <[email protected]>
> > Closes: https://sashiko.dev/#/patchset/86e2514b533bf4d55d4aa2fdbf1404022e8c9430.1776149210.git.caoruide123%40gmail.com
> > Signed-off-by: Ruide Cao <[email protected]>
> > Signed-off-by: Ren Wei <[email protected]>
> > ---
> >  net/mptcp/protocol.c   | 12 +++++----
> >  net/mptcp/protocol.h   |  4 ++-
> >  net/mptcp/subflow.c    |  2 ++
> >  net/mptcp/token.c      | 61 +++++++++++++++++++++++++++++++++++++-----
> >  net/mptcp/token_test.c |  4 +--
> >  5 files changed, 68 insertions(+), 15 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index ca644ec53eed..07eaa6858d05 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -3608,17 +3608,19 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
> >        */
> >       mptcp_set_state(nsk, TCP_ESTABLISHED);
> >
> > +     if (!mptcp_token_accept(subflow_req, msk)) {
> > +             mptcp_release_sched(msk);
> > +             inet_csk_prepare_forced_close(nsk);
> > +             tcp_done(nsk);
>
> Here, you cannot call tcp_done(nsk): nsk is an MPTCP socket, not a TCP
> one. You probably want to destroy the msk instead.
>
> (In theory, your reproducer launched on a kernel with your patches
> applied on top of MPTCP's export branch [1] should produce a warning)
>
> Apart from that, Clashiko has a few comments. Do you mind checking them
> and reply to each comment here, before sending a new version, whether
> you agree/disagree or if it is unclear, please?
>
> [1] https://github.com/multipath-tcp/mptcp_net-next/
>
> Cheers,
> Matt
> --
> Sponsored by the NGI0 Core fund.
> pw-bot: cr
>