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

Hi Ruide,

(+cc netdev ML)

Thank you for having replied to Sashiko's review.

On 20/08/2026 12:58, Wei Ren wrote:

On Thu, Aug 13, 2026 at 06:56:30AM +0000, sashiko-bot@kernel.org wrote:

quoted

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:

Indeed, Sashiko Gemini didn't look at the next patch. Maybe adding a
note in the commit message to say that a similar issue is visible with
MP_CAPABLE and addressed in the next patch? Still, that doesn't feel
natural to add that...

Do you mind also looking at Clashiko's review, please? The suggestion of
replacing sock_hold() by refcount_inc_not_zero() might be valid (to be
double checked):

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1786497414.git.yuantan098%40gmail.com

------------------------------ 8< ---------------------------------

[ ... ]

Discovered by: gpt-5-6-sol, opus-5 · Missed by: opus-4-7, sashiko-gemini

Can the sock_hold() here run on an msk whose last reference has already
been dropped?

The pointer being held was snapshotted by the plain (unsafe_)memcpy() in
inet_reqsk_clone(), and the cloning CPU owns no reference to that msk at
this point.  The clone happens optimistically in reqsk_timer_handler()
before any ownership arbitration:

net/ipv4/inet_connection_sock.c:reqsk_timer_handler() {
	...
	nreq = inet_reqsk_clone(req, nsk);
	if (!nreq)
		goto drop;
	...
	/* Even if we can clone the req, we may need not retransmit any more
	 * SYN+ACKs (nreq->num_timeout > max_syn_ack_retries, etc), or another
	 * CPU may win the "own_req" race so that inet_ehash_insert() fails.
	 */
}

So the third-ACK path can be running on the same request concurrently.
There, the request's msk reference is handed to the child and can then be
released synchronously:

net/mptcp/subflow.c:subflow_syn_recv_sock() {
	...
		/* move the msk reference ownership to the subflow */
		subflow_req->msk = NULL;
		ctx->conn = (struct sock *)owner;

		if (subflow_use_different_sport(owner, sk)) {
			...
			if (!mptcp_pm_announced_has_ssk(owner, sk)) {
				...
				goto dispose_child;
			}
		}

		if (!mptcp_finish_join(child)) {
			...
			goto dispose_child;
		}
}

and dispose_child reaches mptcp_subflow_drop_ctx():

	if (ctx->conn)
		sock_put(ctx->conn);

If that put (or a later teardown of the joined subflow) is the last
reference, for example because userspace closed the MPTCP connection
concurrently, then the refcount_inc() in sock_hold() lands on freed memory
and on a zero refcount, which is the refcount_warn_saturate() "addition on
0; use-after-free" case.

The window between the memcpy() snapshot and this sock_hold() is not
bounded either: in the TFO branch of inet_reqsk_clone() it even contains
spin_lock_bh(&fastopenq->lock).

Would refcount_inc_not_zero() be more appropriate here, with
new_req->msk cleared when it fails so subflow_req_destructor() does not put
a reference that was never taken?  Alternatively, could the hold be taken
under the same exclusion that performs the ownership transfer in
subflow_syn_recv_sock()?

Also, the helper takes req but never uses it, so the original request is
not re-validated at all.  Is the req argument intended to be used?


Discovered by: opus-5 · Missed by: opus-4-7, gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol

This is a pre-existing issue and is not introduced by this patch, but as
long as this hook is the designated fixup point for cloned subflow
requests: inet_reqsk_clone() also byte-copies subflow_req->token_node, the
hlist_nulls node that mptcp_token_new_request() linked into the MPTCP token
bucket for MP_CAPABLE requests, and this version of the helper repairs only
->msk.

The generic detach helper does not cover it either:

net/ipv4/inet_connection_sock.c:reqsk_migrate_reset() {
	req->saved_syn = NULL;
	...
}

With only this patch applied, the clone's token_node looks hashed while it
is not reachable from the bucket chain, so after a successful migration the
surviving clone trips the pedantic check in mptcp_token_accept():

net/mptcp/token.c:mptcp_token_accept() {
	...
	/* pedantic lookup check for the moved token */
	pos = __token_lookup_req(bucket, req->token);
	if (!WARN_ON_ONCE(pos != req))
		hlist_nulls_del_init_rcu(&req->token_node);
}

and the token ends up no longer reserved.

This is resolved later in the same series: the following patch, "mptcp: fix
MP_CAPABLE token migration when cloning reqsk", adds
mptcp_token_move_request() and calls it from mptcp_subflow_reqsk_clone(),
so the state does not exist at the end of the series.  Would it be worth
noting the ordering dependency between the two patches in the changelog,
given the Cc: stable tag on this one?
------------------------------ 8< ------------------------------

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.