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

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/ (local)

[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.

[ ... ]

[ ... ]

[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?

[ ... ]

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