[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

Re: [PATCH net 0/2] net: restore acquire ordering for two remaining

[Hang Nan]

[-- Attachment #1: Type: text/plain, Size: 3883 bytes --]

Hi,

Thanks for the automated review.  Here is the missing information for the
series, plus the instruction-level evidence that was asked for.

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/ (102 of them
today) 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.

Instruction-level evidence (new): the two helpers do compile differently
on a weakly ordered CPU.  Built from this series' base (7.4.0-rc2) with
aarch64-linux-gnu-gcc 14.2 and arm64 defconfig, which enables
CONFIG_ARM64_USE_LSE_ATOMICS, net/core/sock_reuseport.o disassembles as:

  before 2/2 applied:
        fc4:  88a37c02        cas     w3, w2, [x0]     <!-- relaxed CAS -->
        fc8:  6b03003f        cmp     w1, w3
        fcc:  54000981        b.ne    ...

  with 2/2 applied:
        fd4:  88e17c43        casa    w1, w3, [x2]     <!-- acquire CAS -->
        fd8:  6b01001f        cmp     w0, w1

  i.e. CAS -> CASA.  For builds without LSE the same change turns the
  ldxr/stxr loop into ldaxr/stlxr, and refcount_set_release() turns the
  plain str into stlr.

On x86-64 both variants compile to the same "lock cmpxchg", so the
series is a no-op there, which is why the ordering violation is not
observable on the machine I tested on.

Reproduce:
  make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- net/core/sock_reuseport.o
  aarch64-linux-gnu-objdump -d net/core/sock_reuseport.o \
        --disassemble=reuseport_migrate_sock

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 for the reordering itself; demonstrating that would
need a weakly ordered machine (the instruction sequences above are the
strongest evidence I can produce here).  So the series does not claim to
fix an observed bug.

Reachability of the path (for completeness): with
net.ipv4.tcp_migrate_req=1 and four SO_REUSEPORT listeners on loopback,
a 20 second churn run (4 connector threads, 2 threads closing and
re-adding listeners) moved TCPMigrateReqSuccess from 16426 to 17103 and
TCPMigrateReqFailure from 1858 to 1875 on 6.18.58.  On 7.4.0-rc2 the
same load drove 6,625 successful migrations across three 80 second runs
(KASAN kernel, no reports).  A minimal two-listener case (64 established
but unaccepted connections, then close one listener) moves the counter
by 27-29.  This is only a reachability check, not a demonstration of the
reordering.



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) on x86-64
and arm64; 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 runtime demonstration.

Hang Nan

[-- Attachment #2: arm64-unpatched.txt --]
[-- Type: text/plain, Size: 671 bytes --]

     fac:	b9408261 	ldr	w1, [x19, #128]
     fb0:	91020260 	add	x0, x19, #0x80
     fb4:	340005a1 	cbz	w1, 1068 <reuseport_migrate_sock+0x148>
     fb8:	11000422 	add	w2, w1, #0x1
     fbc:	14000010 	b	ffc <reuseport_migrate_sock+0xdc>
     fc0:	2a0103e3 	mov	w3, w1
     fc4:	88a37c02 	cas	w3, w2, [x0]
     fc8:	6b03003f 	cmp	w1, w3
     fcc:	54000981 	b.ne	10fc <reuseport_migrate_sock+0x1dc>  // b.any
     fd0:	2a010042 	orr	w2, w2, w1
     fd4:	37f80502 	tbnz	w2, #31, 1074 <reuseport_migrate_sock+0x154>
     fd8:	f94023f9 	ldr	x25, [sp, #64]
     fdc:	94000000 	bl	0 <__rcu_read_unlock>
     fe0:	aa1303e0 	mov	x0, x19
     fe4:	a94153f3 	ldp	x19, x20, [sp, #16]

[-- Attachment #3: arm64-patched.txt --]
[-- Type: text/plain, Size: 688 bytes --]

     fac:	b9408260 	ldr	w0, [x19, #128]
     fb0:	91020262 	add	x2, x19, #0x80
     fb4:	12b00004 	mov	w4, #0x7fffffff            	// #2147483647
     fb8:	4b000081 	sub	w1, w4, w0
     fbc:	340005a0 	cbz	w0, 1070 <reuseport_migrate_sock+0x150>
     fc0:	7100003f 	cmp	w1, #0x0
     fc4:	5400056d 	b.le	1070 <reuseport_migrate_sock+0x150>
     fc8:	11000403 	add	w3, w0, #0x1
     fcc:	1400000e 	b	1004 <reuseport_migrate_sock+0xe4>
     fd0:	2a0003e1 	mov	w1, w0
     fd4:	88e17c43 	casa	w1, w3, [x2]
     fd8:	6b01001f 	cmp	w0, w1
     fdc:	540007e1 	b.ne	10d8 <reuseport_migrate_sock+0x1b8>  // b.any
     fe0:	f94023f9 	ldr	x25, [sp, #64]
     fe4:	94000000 	bl	0 <__rcu_read_unlock>

[-- Attachment #4: x86-unpatched.txt --]
[-- Type: text/plain, Size: 479 bytes --]

     865:	85 d2                	test   %edx,%edx
     867:	74 2a                	je     893 <reuseport_migrate_sock+0xc3>
     869:	8d 4a 01             	lea    0x1(%rdx),%ecx
     86c:	89 d0                	mov    %edx,%eax
     86e:	f0 0f b1 0f          	lock cmpxchg %ecx,(%rdi)
     872:	75 5f                	jne    8d3 <reuseport_migrate_sock+0x103>
     874:	09 ca                	or     %ecx,%edx
     876:	78 5f                	js     8d7 <reuseport_migrate_sock+0x107>

[-- Attachment #5: x86-patched.txt --]
[-- Type: text/plain, Size: 477 bytes --]

     870:	29 c1                	sub    %eax,%ecx
     872:	85 c9                	test   %ecx,%ecx
     874:	7e 24                	jle    89a <reuseport_migrate_sock+0xca>
     876:	8d 48 01             	lea    0x1(%rax),%ecx
     879:	f0 0f b1 0a          	lock cmpxchg %ecx,(%rdx)
     87d:	75 eb                	jne    86a <reuseport_migrate_sock+0x9a>
     87f:	e8 00 00 00 00       	call   884 <reuseport_migrate_sock+0xb4>
     884:	48 89 d8             	mov    %rbx,%rax

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

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]

Thanks.  All four points are correct; respun as v3 with two additional
patches.  Details below.

[High] token 0 accepted by mptcp_token_get_sock()

Agreed, and it is a real defect rather than a consequence of the missing
ordering.  Both validations compare the socket's token with the requested
one, and for a requested token of 0 that comparison is satisfied by
exactly the states a socket must not be accepted in: the value
mptcp_token_destroy() stores, and the zeroed state of a newly allocated
msk.  The ordering change does not help there, because the comparison
itself is vacuous.

What I verified statically before sending v3:

  - mptcp_parse_option() copies the MP_JOIN SYN token with
    get_unaligned_be32() and does not reject 0;
  - subflow_token_join_request() passes the result straight to
    mptcp_pm_get_local_id(), and for a subflow with a different source
    port to mptcp_pm_announced_has_ssk(), which walks
    msk->pm.anno_list under msk->pm.lock;
  - that list is initialised from mptcp_pm_data_init(), i.e. well after
    a socket can be found on a chain.

Patch 3 now rejects token 0 before walking the chain.  I did not manage
to reproduce the resulting crash, so I am not claiming to have observed
it: the description in the patch is a static argument, and the Fixes: tag
points at the commit that introduced the lookup, not at an observed
failure.

[High] mptcp_token_iter_next() re-checks only net_eq()

Agreed.  Patch 4 re-reads the token after the acquisition and drops the
socket unless it is non-zero and still hashes to the slot being walked.
A socket on a chain always hashes to that slot, so the check only rejects
objects whose memory has been reused in the meantime.  I kept the
severity assessment lower than the one above because both consumers
(mptcp_diag, the userspace path manager) are netlink paths that require
privileges in the target network namespace, so the bad state is reached
locally rather than from the wire.

[Medium] the commit message gives the wrong reason

Agreed, and the correction is in patch 1.  The acquire does not pair with
anything in mptcp_token_destroy(): that function does not touch
sk_refcnt.  What it pairs with is the publication in
sock_init_data_uid(), smp_wmb() followed by refcount_set(&sk->sk_refcnt,
1), in the SLAB_TYPESAFE_BY_RCU reuse window, which is the case
refcount.h documents.  The message now says that, and keeps the teardown
comment only as background for what the re-check is looking at.

[Low] Fixes: trailer

Agreed, and corrected in v2 (now v3): 2c5ebd001d4f and 96d890daad05 for
the two mptcp sites, and 1cd62c21572c for the reuseport patch, whose site
also postdates the consolidation.

On testing: the ordering change itself has no runtime observable on
x86-64 (both variants compile to lock cmpxchg) and we have no way to
observe the reordering on the hardware at hand, so patches 1 and 2 carry
an instruction-level argument only (on arm64 the CAS becomes CASA; I can
include the disassembly if useful).  Patches 3 and 4 are plain
functional checks and both objects build; I have not run a KUnit case
for token 0 yet.

pw-bot: cr