[PATCH net 1/2] net: mptcp: take token lookup references with acquire ordering

[PATCH net 1/2] net: mptcp: take token lookup references with acquire ordering

[Hang Nan]

mptcp_token_get_sock() validates the token and the network namespace,
takes a reference on the socket and then validates them again.  The
second validation only rejects a socket that is being torn down if it is
ordered after the reference acquisition: mptcp_token_destroy() unlinks
the socket and does WRITE_ONCE(msk->token, 0), so a reader that observes
the token before the acquisition can return a socket whose token has
already been cleared.

refcount_inc_not_zero() does not provide acquire ordering since the
refcount_t consolidation in fb041bb7c0a9, which removed the ordering
that the previous atomic_add_unless() based implementation gave.  On
weakly ordered CPUs the loads of msk->token and sock_net(sk) can
therefore be reordered before the refcount increment, defeating the
re-validation.

The same pattern was fixed for the IPv4 and IPv6 established lookups,
inet_twsk_purge() and bpf_sk_assign() in 5b5d8f351dd9.  The iterator
below has the same shape and is converted as well.

No behaviour change on strongly ordered architectures; this only
restores the ordering the second validation relies on.

Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
Cc: stable@vger.kernel.org
Signed-off-by: Hang Nan <2122295973@qq.com>
---
 net/mptcp/token.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index e0f2823f92ef..3429b43a9f7c 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -309,7 +309,7 @@ struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token)
 		    !net_eq(sock_net(sk), net))
 			continue;
 
-		if (!refcount_inc_not_zero(&sk->sk_refcnt))
+		if (!refcount_inc_not_zero_acquire(&sk->sk_refcnt))
 			goto not_found;
 
 		if (READ_ONCE(msk->token) != token ||
@@ -370,7 +370,7 @@ struct mptcp_sock *mptcp_token_iter_next(const struct net *net, long *s_slot,
 			if (num <= *s_num)
 				continue;
 
-			if (!refcount_inc_not_zero(&sk->sk_refcnt))
+			if (!refcount_inc_not_zero_acquire(&sk->sk_refcnt))
 				continue;
 
 			if (!net_eq(sock_net(sk), net)) {
-- 
2.53.0

[PATCH net 2/2] net: core: take the reuseport migration reference with acquire ordering

[Hang Nan]

reuseport_migrate_sock() returns a listener with a reference held, and
every caller immediately reads that listener: tcp_v4_rcv() and
tcp_v6_rcv() pass it to tcp_filter() and tcp_check_req(), and the
request-socket paths clone its state.  A listener removed from the
reuseport group concurrently with that lookup must not be observed in a
half-torn-down state after the reference was successfully taken.

refcount_inc_not_zero() lost its acquire ordering in the refcount_t
consolidation (fb041bb7c0a9), so these subsequent reads are not ordered
against the reference acquisition.  Use
refcount_inc_not_zero_acquire(), as was done for the established-socket
lookups, inet_twsk_purge() and bpf_sk_assign() in 5b5d8f351dd9.

No behaviour change on strongly ordered architectures.

Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
Cc: stable@vger.kernel.org
Signed-off-by: Hang Nan <2122295973@qq.com>
---
 net/core/sock_reuseport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
index fccbdd0430b0..78d1d7103122 100644
--- a/net/core/sock_reuseport.c
+++ b/net/core/sock_reuseport.c
@@ -666,7 +666,8 @@ struct sock *reuseport_migrate_sock(struct sock *sk,
 	if (!nsk)
 		nsk = reuseport_select_sock_by_hash(reuse, hash, socks);
 
-	if (IS_ERR_OR_NULL(nsk) || unlikely(!refcount_inc_not_zero(&nsk->sk_refcnt))) {
+	if (IS_ERR_OR_NULL(nsk) ||
+	    unlikely(!refcount_inc_not_zero_acquire(&nsk->sk_refcnt))) {
 		nsk = NULL;
 		goto failure;
 	}
-- 
2.53.0

Re: [PATCH net 1/2] net: mptcp: take token lookup references with acquire ordering

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 1/2] net: mptcp: take token lookup references with acquire ordering

[Hang Nan]

Hi,

Thanks for the automated review.  Here is the missing information for the
series.

How it was found: manual code inspection, not a runtime report.  While
looking at net/ lockless lookups right after 5b5d8f351dd9 landed, I
enumerated every refcount_inc_not_zero() call site in net/ (101 of them)
and classified each one by whether the reference is taken only to keep
the object alive, or whether object state is re-validated after the
acquisition.  mptcp_token_get_sock() and mptcp_token_iter_next() are in
the second group: they re-read msk->token and sock_net(sk) after the
increment, and mptcp_token_destroy() unlinks the socket and then does
WRITE_ONCE(msk->token, 0), so the re-validation is the only thing that
rejects a socket whose token has already been cleared.
reuseport_migrate_sock() in 2/2 is the only other site with that shape
in net/.  The enumeration of the call sites was tool-assisted; the
classification and both changes were checked by hand against the
callers and against the teardown paths.

Whether it was triggered: no.  This is a theoretical ordering gap, not
an observed crash.  I have no syzbot report, no KCSAN splat, and no
runtime reproducer.  The reordering is not observable on x86-64 (TSO
does not allow it), and demonstrating it would need a weakly ordered
CPU (arm64) or KCSAN, neither of which I have access to.  So the series
does not claim to fix an observed bug.

What was verified: the re-validation exists and is the guard described
above; the teardown order is __sk_nulls_del_node_init_rcu() followed by
WRITE_ONCE(msk->token, 0) in mptcp_token_destroy(); both touched objects
compile cleanly (net/mptcp/token.o, net/core/sock_reuseport.o) and the
change is a no-op on strongly ordered CPUs.  The argument is the same
one used in 5b5d8f351dd9 for the inet lookups: a successful reference
acquisition has to order the validation that follows it.

Happy to drop the series or fold it into a larger ordering series if
reviewers would rather wait for a KCSAN/arm64 demonstration.

Hang Nan