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

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