Re: [PATCH net v4 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk

On Thu, Aug 13, 2026 at 06:56:30AM +0000, [email protected] wrote:
> This is a pre-existing issue, but does this clone helper also need to
> handle the migration of token_node for MP_CAPABLE requests?

That is what patch 2/2 of this series does:

  https://lore.kernel.org/all/6abadc83940143e099fa3b54ec6dea2fb95da090.1786497414.git.yuantan098@gmail.com/

It extends this helper with mptcp_token_move_request(), which re-hashes
the token node onto the clone with hlist_nulls_replace_init_rcu() under
the token bucket lock, so the clone becomes the single token-table owner
and the original is left unhashed.  mptcp_token_accept() and
mptcp_token_destroy_request() re-check ownership under the same lock.

The same question was raised on v3, and v4 was restructured to answer it.
Reviewing 1/2 in isolation makes it read as an unfixed issue.

Two details of the analysis are inaccurate, for the archive:

> ... leading to hash list corruption and a reliable kernel warning

There is no list corruption.  Nothing in the bucket chain points at the
clone's copied token_node, so __token_lookup_req() can never return it,
and both list mutations are gated on "pos == subflow_req".  The stale
node is never passed to hlist_nulls_del_init_rcu().  The splat is the
only observable effect: the panic_on_warn DoS holds, the corruption does
not.

> permanently lose the MP_CAPABLE token from the hash table, breaking
> subsequent MP_JOIN requests for that connection.

The msk insertion sits outside the WARN_ON_ONCE() guard:

if (!WARN_ON_ONCE(pos != req))
hlist_nulls_del_init_rcu(&req->token_node);
__sk_nulls_add_node_rcu((struct sock *)msk, &bucket->msk_chain);

so the msk still enters msk_chain, mptcp_token_get_sock() keeps finding
it, and MP_JOIN for that connection keeps working.  What does drift is
bucket->chain_len: a net -1 per migrated connection that completes the
handshake.  It is a signed int, so it goes negative and
__token_bucket_busy()'s TOKEN_MAX_CHAIN_LEN cap stops limiting that
bucket.  Patch 2/2 fixes that as well.

Thanks,
Ruide


<[email protected]> 于2026年8月13日周四 14:56写道：
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] TCP request migration clones MPTCP request sockets without updating or re-hashing the `token_node` list pointers for MP_CAPABLE requests, leading to hash list corruption and a reliable kernel warning (`WARN_ON_ONCE`) that can cause a Denial of Service.
> --
>
> --- Patch [1]: [PATCH net v4 1/2] mptcp: hold MP_JOIN msk ref when cloning reqsk ---
> commit 2b2ebcd4a3ea728b0d1c8d74d1e09d5e75f65848
> Author: Ruide Cao <[email protected]>
>
> mptcp: hold MP_JOIN msk ref when cloning reqsk
>
> TCP request migration clones pending request sockets with
> inet_reqsk_clone(). For MPTCP MP_JOIN requests this byte-copies
> subflow_req->msk, but the clone does not take its own reference.
>
> Closes: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index e1f20ff8fdb42..8f8e1229766d4 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);
> >  }
> >
> > +void mptcp_subflow_reqsk_clone(struct request_sock *req,
> > +                            struct request_sock *new_req)
> > +{
> > +     struct mptcp_subflow_request_sock *subflow_req;
> > +
> > +     subflow_req = mptcp_subflow_rsk(new_req);
> > +
> > +     if (subflow_req->msk)
> > +             sock_hold((struct sock *)subflow_req->msk);
> > +}
>
> [Severity: High]
> This is a pre-existing issue, but does this clone helper also need to
> handle the migration of token_node for MP_CAPABLE requests?
>
> When inet_reqsk_clone() performs a byte-copy of the request socket, it
> copies the token_node list pointers (pprev and next) as-is:
>
> net/ipv4/inet_connection_sock.c:inet_reqsk_clone() {
>     ...
>         unsafe_memcpy(&nreq_sk->sk_dontcopy_end, &req_sk->sk_dontcopy_end,
>                       req->rsk_ops->obj_size - offsetof(struct sock, sk_dontcopy_end),
>                       /* alloc is larger than struct, see above */);
>     ...
> }
>
> Because the list pointers are copied without re-hashing or updating them,
> the original request is destroyed and removes itself from the token hash
> table, leaving the clone with a stale pprev pointer.
>
> When the cloned request is eventually destroyed,
> mptcp_token_destroy_request() checks hlist_nulls_unhashed(), which returns
> false due to the stale pointer. It then fails to find the token in the
> bucket, which triggers a WARN_ON_ONCE:
>
> net/mptcp/token.c:mptcp_token_destroy_request() {
>     ...
>         pos = __token_lookup_req(bucket, subflow_req->token);
>         if (!WARN_ON_ONCE(pos != subflow_req)) {
>                 hlist_nulls_del_init_rcu(&pos->token_node);
>                 bucket->chain_len--;
>         }
>     ...
> }
>
> This could cause a kernel panic on systems with panic_on_warn enabled, and
> permanently lose the MP_CAPABLE token from the hash table, breaking
> subsequent MP_JOIN requests for that connection.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1