[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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] mptcp_token_get_sock() uses the token value as its only liveness
  check, but it accepts a requested token of 0.
- [High] mptcp_token_iter_next() re-checks only net_eq() after taking the
  reference (token.c:376).
- [Medium] The commit message gives the wrong reason for the change, and
  it never mentions the race the change actually closes.
- [Low] net: mptcp: Fixes trailer cites commit predating token.c lockless
  lookups

--

net: mptcp: take token lookup references with acquire ordering

This patch changes mptcp_token_get_sock() and mptcp_token_iter_next() to
use refcount_inc_not_zero_acquire() instead of refcount_inc_not_zero().
The aim is to order the re-validation of msk->token and sock_net() after
the reference is taken, as 5b5d8f351dd9 did for the inet lookups.

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

[Severity: Medium]
Is this the race that the acquire ordering actually closes?

mptcp_token_destroy() clears the token without touching sk_refcnt:

net/mptcp/token.c:mptcp_token_destroy() {
    ...
	spin_unlock_bh(&bucket->lock);
	WRITE_ONCE(msk->token, 0);
}

That leaves no release on the refcount for
refcount_inc_not_zero_acquire() to pair with. With or without acquire,
the re-check can still read the old token. mptcp_token_destroy() can also
run right after the re-check succeeds.

The case the acquire does fix looks like object reuse. mptcp_prot uses
SLAB_TYPESAFE_BY_RCU and only listeners set SOCK_RCU_FREE. So an
established msk can be freed and reallocated while an RCU reader still
holds a pointer to it. The new owner publishes the refcount in
sock_init_data_uid():

	smp_wmb();
	refcount_set(&sk->sk_refcnt, 1);

Without acquire, the re-check loads of msk->token and sock_net() can be
satisfied before the increment. They would return the old object's values
while the increment succeeds on the new object. The lookup could then
return an msk from a different connection, possibly in a different netns.
include/linux/refcount.h documents this case as the reason for the
acquire variants.

Since this is tagged for stable, could the commit message describe the
SLAB_TYPESAFE_BY_RCU reuse case instead of the teardown race?

> diff --git a/net/mptcp/token.c b/net/mptcp/token.c
> index e0f2823f92efb..3429b43a9f7c3 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 ||

[Severity: Low]
The Fixes tag points at fb041bb7c0a9 ("locking/refcount: Consolidate
implementations of refcount_t"), which landed in v5.5-rc1. At that
point net/mptcp/token.c did not exist yet, so there was no MPTCP code
for that commit to break.

The lockless lookup in mptcp_token_get_sock() came in with
2c5ebd001d4f ("mptcp: refactor token container"). That is the
refcount_inc_not_zero() on sk->sk_refcnt followed by the token and
netns re-check.

mptcp_token_iter_next() came in with 96d890daad05 ("mptcp: add msk
interations helper"). That added its refcount_inc_not_zero() and the
net_eq() re-check.

Both of those are in v5.9-rc1.

Stable tooling uses the Fixes tag to decide which trees get the
backport. Pointing at a v5.5 locking commit would have it try v5.5
through v5.8. In those trees this file is either missing or has no
lockless lookup at all.

Could the tags be changed to:

Fixes: 2c5ebd001d4f ("mptcp: refactor token container")
Fixes: 96d890daad05 ("mptcp: add msk interations helper")

[Severity: High]
What happens in mptcp_token_get_sock() when the requested token is 0?

Token 0 is never assigned, because __token_bucket_busy() treats it as
busy:

	return !token || t->chain_len >= TOKEN_MAX_CHAIN_LEN ||

However, 0 is exactly what mptcp_token_destroy() writes into a dying msk.
It is also what a newly reallocated msk holds, since sk_alloc() zeroes
the object. For token == 0, both the first check and this re-check pass
on a cleared or recycled msk. So the "socket whose token has already been
cleared" case from the commit message can still happen with this patch.

The token comes from the wire. mptcp_parse_option() copies the MP_JOIN
SYN token without checking it, and then:

subflow_check_req()
  subflow_token_join_request()
    mptcp_token_get_sock(net, 0)

Suppose an RCU reader holds a pointer to an msk on bucket 0's chain while
that msk is unhashed, freed and reallocated by inet_create() in the same
netns. Between sock_init_data() and sk_prot->init(), the new msk has
refcount 1, token 0 and the matching netns. It would pass both checks and
be returned.

subflow_check_req() then calls subflow_use_different_sport(). That sees
an inet_sport of 0 and calls mptcp_pm_announced_has_ssk():

	list_for_each_entry(entry, &msk->pm.anno_list, list) {

pm.anno_list is not initialised until mptcp_pm_data_init() runs from
__mptcp_init_sock(), so head->next is still NULL. Can this oops in
softirq?

Even without reuse, a torn-down msk whose token was just cleared would be
returned to the join path and pinned by the request socket.
mptcp_diag_dump_one() with idiag_cookie[0] == 0 and the userspace PM
token lookup seem to have the same problem.

Should mptcp_token_get_sock() return early for token 0, for example:

	if (!token)
		return NULL;

or should the re-check also confirm the msk is still hashed?

> @@ -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)) {

[Severity: High]
The commit message says the iterator "has the same shape and is converted
as well". The re-check in mptcp_token_iter_next() only looks at net_eq(),
though. Can it reject an msk that was torn down or recycled in the same
netns?

With SLAB_TYPESAFE_BY_RCU, a preempted RCU reader in
mptcp_token_iter_next() can hold a pointer to an msk while it is
unhashed, freed and reallocated by inet_create() in the same netns.

There is a window between sock_init_data_uid() publishing the refcount:

	smp_wmb();
	refcount_set(&sk->sk_refcnt, 1);

and __mptcp_init_sock() running:

	INIT_LIST_HEAD(&msk->conn_list);

In that window the msk has refcount 1, the same netns, and zeroed
conn_list and pm state. A zeroed pm.pm_type looks like
MPTCP_PM_TYPE_KERNEL.

The iterator would take the reference, pass the net_eq() check and return
this msk. An endpoint delete then does:

mptcp_nl_remove_subflow_and_signal_addr()
  lock_sock(sk)
  mptcp_pm_has_subflow_saddr()
    mptcp_for_each_subflow(msk, subflow)

Would this walk the zeroed conn_list and dereference a near-NULL entry?

The other iterator users, mptcp_nl_flush_addrs_list(),
mptcp_pm_nl_set_flags_all() and mptcp_nl_remove_id_zero_address(), would
also act on a half-built or unrelated msk.

Should the re-check after the acquire also confirm the msk is still
hashed in this slot, for example:

	!sk_unhashed(sk) && (READ_ONCE(msk->token) & token_mask) == slot

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_28A2BD1631FE531992E91753BA6419FFC506%40qq.com

[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 2/2] net: core: take the reuseport migration reference with acquire ordering

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the reuseport_migrate_sock() refcount
  change prevents a listener that is concurrently removed from the…
- [Low] The comment in inet_reqsk_clone()
  (net/ipv4/inet_connection_sock.c:925) says its sock_put(sk) on the…

--

net: core: take the reuseport migration reference with acquire ordering

This commit changes reuseport_migrate_sock() to take the reference on the
selected listener with refcount_inc_not_zero_acquire() instead of
refcount_inc_not_zero(). The stated goal is to stop callers from observing
a listener in a half-torn-down state when it is removed from the reuseport
group at the same time. The patch carries Fixes: and Cc: stable tags.

> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> index b9e5648fa4c51..56230aa55a3e2 100644
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c
> @@ -667,7 +667,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))) {

[Severity: Medium]
What does the acquire ordering on this increment order against? The commit
message says:

  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.

reuseport_migrate_sock() picks nsk from reuse->socks[] while holding
rcu_read_lock(). Every hashed listener gets SOCK_RCU_FREE in
__inet_hash():

net/ipv4/inet_hashtables.c:__inet_hash() {
    ...
	sock_set_flag(sk, SOCK_RCU_FREE);
    ...
}

So the listener's memory cannot be freed or reused for another socket
inside this RCU section. Also, nothing after the increment re-validates
nsk. There is no sk_state or sk_reuseport_cb re-check before nsk is
returned.

The contract for the helper in include/linux/refcount.h limits it to that
reuse case:

  refcount_{add|inc}_not_zero_acquire() and refcount_set_release() provide
  acquire and release ordering for cases when the memory occupied by the
  object might be reused to store another object. This is important for the
  cases where secondary validation is required to detect such reuse, e.g.
  SLAB_TYPESAFE_BY_RCU. The secondary validation checks have to happen after
  the refcount is taken, hence acquire order is necessary.

Neither condition holds here. Commit 5b5d8f351dd9, cited as precedent, only
changed sites where the socket is validated again after the reference is
taken. __inet_lookup_established() re-runs inet_match() and checks the
unhashed state, and inet_twsk_purge() re-checks check_net(). Its
bpf_sk_assign() change had a separate justification: a BTF socket pointer
obtained without a reference.

A listener that is being closed is torn down while the closer still holds
its own reference. That teardown is not published through a release on
sk_refcnt. So even with the acquire, can't a caller still see the closing
listener?

Callers already handle that state with an explicit check, for example in
inet_csk_reqsk_queue_add():

	spin_lock(&queue->rskq_lock);
	if (unlikely(sk->sk_state != TCP_LISTEN)) {
		inet_child_forget(sk, req, child);

The same pattern also exists elsewhere and is left alone. inet_lookup(),
inet6_lookup(), and nf_tproxy_get_sock_v4()/nf_tproxy_get_sock_v6() all
take a reference on an unreferenced listener with plain
refcount_inc_not_zero() and then use it. Those sites look correct for the
same RCU-stable reason, which suggests this one is correct as well without
the change.

Given that, are the Fixes: fb041bb7c0a9 and Cc: stable tags justified?
Could the commit message name a concrete load that can see stale data here,
or include a reproducer? As written, the patch adds a barrier on weakly
ordered architectures but does not change what callers can observe.

>  		nsk = NULL;
>  		goto failure;
>  	}

[Severity: Low]
This isn't a bug, but the comment on the matching release in
inet_reqsk_clone() still names the old primitive:

net/ipv4/inet_connection_sock.c:inet_reqsk_clone() {
    ...
	if (!nreq) {
		__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQFAILURE);

		/* paired with refcount_inc_not_zero() in reuseport_migrate_sock() */
		sock_put(sk);
		return NULL;
	}
    ...
}

Should this comment be updated if the switch to
refcount_inc_not_zero_acquire() is kept?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_28A2BD1631FE531992E91753BA6419FFC506%40qq.com

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