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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2 (1)

- [Low] mptcp_subflow_reqsk_clone() (net/mptcp/subflow.c:50-79) performs
  a lockless dereference of a possibly recycled SLAB_TYPESAFE_BY_RCU…
- [Low] mptcp_subflow_reqsk_clone() (net/mptcp/subflow.c:66-76) uses the
  fully relaxed refcount_inc_not_zero() to take a reference on an object…

Pre-existing issues:
- [Low] Incomplete clone fixup for MP_CAPABLE token ownership:
  inet_reqsk_clone() byte-copies the whole mptcp_subflow_request_sock…

--

mptcp: hold MP_JOIN msk ref when cloning reqsk

TCP request migration byte-copies subflow_req->msk into the cloned request
without taking a reference, so both requests can drop the same msk
reference.  This adds mptcp_subflow_reqsk_clone(), called from
inet_reqsk_clone(), which conditionally acquires a reference for the clone
and re-validates the pointer, and marks the ownership-transfer store in
subflow_syn_recv_sock() with WRITE_ONCE().

Closes: https://lore.kernel.org/all/20260804095051.715355-1-david.lee@trailofbits.com/

> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 485d55b66ea67..8e063454fcf6e 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -219,6 +219,8 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
>  struct request_sock *mptcp_subflow_reqsk_alloc(const struct request_sock_ops *ops,
>  					       struct sock *sk_listener,
>  					       bool attach_listener);
> +void mptcp_subflow_reqsk_clone(struct request_sock *req,
> +			       struct request_sock *new_req);

[Severity: Low]
Would it be worth documenting the calling-context requirement here?

The safety of mptcp_subflow_reqsk_clone() depends on the caller being in an
RCU read-side critical section, because the msk it dereferences comes from a
SLAB_TYPESAFE_BY_RCU cache.  That requirement is only stated in a comment
inside net/mptcp/subflow.c; the prototype and the !CONFIG_MPTCP stub in
include/net/mptcp.h say nothing about it, and there is no
RCU_LOCKDEP_WARN() or lockdep_assert_in_softirq() in the helper itself.

All three inet_reqsk_clone() call sites satisfy it only implicitly:
reqsk_timer_handler() runs in the rsk_timer softirq,
inet_csk_complete_hashdance() runs in NET_RX softirq, and
inet_csk_listen_stop() is inside local_bh_disable().  In the last case the
explicit rcu_read_lock() is taken only after inet_reqsk_clone() returns:

	nreq = inet_reqsk_clone(req, nsk);
	if (nreq) {
		refcount_set(&nreq->rsk_refcnt, 1);

		rcu_read_lock();

which reads as though the clone needs no RCU protection at all.  A comment
on the prototype plus a lockdep assertion in the helper would keep this from
silently breaking if a future caller runs in process context.

[ ... ]

> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 6257459bcee24..896f472dcba29 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c

[ ... ]

> @@ -961,6 +962,9 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
>  		rcu_assign_pointer(tcp_sk(nreq->sk)->fastopen_rsk, nreq);
>  	}
>  
> +	if (rsk_is_mptcp(req))
> +		mptcp_subflow_reqsk_clone(req, nreq);
> +
>  	return nreq;
>  }

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index af81ad5e699d1..f13011f591b78 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -47,6 +47,37 @@ 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 = mptcp_subflow_rsk(req);
> +	struct mptcp_subflow_request_sock *new_subflow_req;
> +	struct mptcp_sock *msk;
> +
> +	new_subflow_req = mptcp_subflow_rsk(new_req);
> +
> +	/* A non-NULL ->msk means the request owns one reference.  The clone
> +	 * copied only the pointer, while the original request can concurrently
> +	 * transfer its reference to the child.  Acquire a reference for the
> +	 * clone, then verify that the original request still owns the same msk.
> +	 * MPTCP sockets use SLAB_TYPESAFE_BY_RCU and all clone callers run in
> +	 * an RCU read-side critical section, keeping the memory stable here.
> +	 */
> +	msk = READ_ONCE(subflow_req->msk);
> +	if (msk) {
> +		struct sock *msk_sk = (struct sock *)msk;
> +
> +		if (!refcount_inc_not_zero(&msk_sk->sk_refcnt)) {
> +			msk = NULL;
> +		} else if (READ_ONCE(subflow_req->msk) != msk) {
> +			sock_put(msk_sk);
> +			msk = NULL;
> +		}
> +	}

[Severity: Low]
Should this use refcount_inc_not_zero_acquire() instead?

The increment here is followed by a validation load that re-reads
subflow_req->msk and compares addresses.  include/linux/refcount.h describes
refcount_inc_not_zero_acquire() for exactly that pattern:

 * This function should be used when memory occupied by the object might be
 * reused to store another object -- consider SLAB_TYPESAFE_BY_RCU.

refcount_inc_not_zero() provides no memory ordering and only orders future
stores through the control dependency, so the second READ_ONCE() is not
ordered after the refcount RMW.  Can the second read then observe the stale
non-NULL pointer while the increment landed on a freshly reused mptcp_sock
at the same address, letting the address comparison pass?

If so, the clone would own a reference to an unrelated live msk, and the
third ACK would compute the HMAC against the wrong keys in
subflow_hmac_valid(), rejecting the join with
SUBFLOW_REQ_INC_STATS(MPTCP_MIB_JOINACKMAC).

This needs load-load reordering on a weakly ordered architecture plus
immediate slab reuse, so no concrete failing execution is demonstrated here,
and the relaxed idiom is what the rest of net/ currently uses (for example
__inet_lookup_established() and mptcp_token_get_sock()).  An explicit
smp_rmb() before the recheck would also cover it.

> +
> +	new_subflow_req->msk = msk;
> +}

[Severity: Low]
This isn't a bug introduced by this patch, but with only this patch applied
the clone fixup is incomplete for MP_CAPABLE token state.

inet_reqsk_clone() copies req->rsk_ops->obj_size bytes, so the whole
mptcp_subflow_request_sock is duplicated including ->token and the hashed
->token_node, and reqsk_migrate_reset() clears only saved_syn and the ireq
option pointers.  This helper repairs ->msk only.

After a successful migration the original request is freed and unlinks the
shared token:

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--;
	}
	...
}

The surviving clone is then left with a token_node that looks hashed but is
no longer reachable, and mptcp_token_accept() trips its
WARN_ON_ONCE(pos != req) while bucket->chain_len accounting drifts.

The follow-up patch in this series, "mptcp: fix MP_CAPABLE token migration
when cloning reqsk", adds mptcp_token_move_request() and calls it from here,
and converts mptcp_token_accept()/mptcp_token_destroy_request() to
lock-rechecked, non-warning variants, so this is only a property of the
intermediate state.  Given both patches carry the same Fixes tag and are
tagged for stable, is it worth noting in the changelog that applying 1/2
alone leaves that reachable WARN_ON_ONCE in place?

[ ... ]

> @@ -923,7 +954,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>  			}
>  
>  			/* move the msk reference ownership to the subflow */
> -			subflow_req->msk = NULL;
> +			WRITE_ONCE(subflow_req->msk, NULL);
>  			ctx->conn = (struct sock *)owner;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788202924.git.caoruide123%40gmail.com