[PATCH net v3 1/2] inet: reject unhashed sockets during established lookups

[PATCH net v3 1/2] inet: reject unhashed sockets during established lookups

[Hyunwoo Kim]

TCP established ehash lookups are lockless. The socket caches involved
use SLAB_TYPESAFE_BY_RCU, so memory can be reused before an RCU reader
that retained its old address has finished.

After a tuple match, __inet_lookup_established() and
__inet6_lookup_established() acquire a reference and repeat the tuple
check. They do not check whether the new object is hashed. If the
address is reused for a socket with the same tuple, the lookup can
acquire a reference to the new object and return it while its nulls node
is unhashed.

A PoC that drives reuse with a SYN cookie request socket reproduced the
crash. The new request socket is unhashed and has a NULL rsk_listener.
A lookup that had loaded the address of the old request socket acquired
the reference published by tcp_get_cookie_sock() and returned the new
object. tcp_v4_rcv() then read sk_state through the NULL listener.

The resulting crash was:

  BUG: kernel NULL pointer dereference, address: 0000000000000012
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  ...
  Oops: Oops: 0000 [#1] SMP NOPTI
  CPU: 4 UID: 0 PID: 0 Comm: swapper/4 Not tainted 7.3.0-00591-g0e135910deec #1 PREEMPT(lazy)
  ...
  RIP: 0010:tcp_v4_rcv+0x44e/0x1280
  ...
  R10: 0000000000000001 R11: 00000000024ca8c0 R12: 0000000000000000
  ...
  CR2: 0000000000000012 CR3: 0000000103d50004 CR4: 0000000000770ef0
  ...
  Call Trace:
   <IRQ>
   ? raw_local_deliver+0xae/0x240
   ip_protocol_deliver_rcu+0x2d/0x180
   ip_local_deliver_finish+0x84/0x100
   ip_sublist_rcv_finish+0x5f/0x70
   ip_sublist_rcv+0x12a/0x1d0
   ip_list_rcv+0xfd/0x130
   __netif_receive_skb_list_core+0x223/0x250
   netif_receive_skb_list_internal+0x1d2/0x2c0
   ? try_fill_recv+0x39/0x7b0
   napi_complete_done+0x74/0x170
   virtnet_poll+0x445/0x600
   __napi_poll+0x27/0x1e0
   net_rx_action+0x2fe/0x3b0
   handle_softirqs+0xc2/0x2c0
   __irq_exit_rcu+0x74/0xd0
   common_interrupt+0x7f/0x90
   </IRQ>
  ...
  Kernel panic - not syncing: Fatal exception in interrupt

After taking a reference, check whether the nulls node is unhashed in
both IPv4 and IPv6 established lookups. If it is unhashed or the tuple
no longer matches, drop the reference and restart the lookup.

Lookups racing with the TIME_WAIT hashdance, or with a request being
replaced by its child, now restart and find the new socket instead of
returning the old one.

Request sockets that were never inserted into ehash can have a nonzero
refcount, and a stale lookup can hold a transient reference. Therefore,
when SYN cookie, Fast Open, and request migration paths fail after
setting a nonzero refcount, release the references held by those paths
with reqsk_put() instead of freeing the request directly.

Fixes: 079096f103fa ("tcp/dccp: install syn_recv requests into ehash table")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@kernel.org>
---
 net/ipv4/inet_connection_sock.c | 10 +++++++---
 net/ipv4/inet_hashtables.c      |  5 +++--
 net/ipv4/syncookies.c           |  2 ++
 net/ipv4/tcp_input.c            |  4 +++-
 net/ipv6/inet6_hashtables.c     |  4 +++-
 5 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 6a30f11384547..bd0bad96c93c3 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -1136,10 +1136,14 @@ static void reqsk_timer_handler(struct timer_list *t)
 	 */
 	if (nreq) {
 		__NET_INC_STATS(net, LINUX_MIB_TCPMIGRATEREQFAILURE);
+		/* The timer was not armed, drop its reference here. */
+		reqsk_put(nreq);
 no_ownership:
 		reqsk_migrate_reset(nreq);
 		reqsk_queue_removed(queue, nreq);
-		__reqsk_free(nreq);
+		/* Drop the temporary reference and the one reserved for ehash. */
+		reqsk_put(nreq);
+		reqsk_put(nreq);
 	}
 
 drop:
@@ -1447,7 +1451,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child,
 
 			__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQFAILURE);
 			reqsk_migrate_reset(nreq);
-			__reqsk_free(nreq);
+			reqsk_put(nreq);
 		} else if (inet_csk_reqsk_queue_add(sk, req, child)) {
 			return child;
 		}
@@ -1502,7 +1506,7 @@ void inet_csk_listen_stop(struct sock *sk)
 					__NET_INC_STATS(sock_net(nsk),
 							LINUX_MIB_TCPMIGRATEREQFAILURE);
 					reqsk_migrate_reset(nreq);
-					__reqsk_free(nreq);
+					reqsk_put(nreq);
 				}
 				rcu_read_unlock();
 
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index ba0faa9ae2bb3..58d54965036a2 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -552,8 +552,9 @@ struct sock *__inet_lookup_established(const struct net *net,
 		if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) {
 			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
 				goto out;
-			if (unlikely(!inet_match(net, sk, acookie,
-						 ports, dif, sdif))) {
+			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
+				     !inet_match(net, sk, acookie, ports, dif,
+						 sdif))) {
 				sock_gen_put(sk);
 				goto begin;
 			}
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 73e1297681847..6440305615378 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -218,6 +218,8 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
 
 		bh_unlock_sock(child);
 		sock_put(child);
+		reqsk_put(req);
+		return NULL;
 	}
 	__reqsk_free(req);
 
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 99baf14afdfd8..3583867487d7f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -7777,7 +7777,9 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 		if (!inet_csk_reqsk_queue_add(sk, req, fastopen_sk)) {
 			bh_unlock_sock(fastopen_sk);
 			sock_put(fastopen_sk);
-			goto drop_and_free;
+			/* tcp_disconnect() already dropped one reqsk ref. */
+			reqsk_put(req);
+			goto drop;
 		}
 		READ_ONCE(sk->sk_data_ready)(sk);
 		bh_unlock_sock(fastopen_sk);
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index b111b51d69fc9..aa5fa1dd062f7 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -108,7 +108,9 @@ struct sock *__inet6_lookup_established(const struct net *net,
 		if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
 			goto out;
 
-		if (unlikely(!inet6_match(net, sk, saddr, daddr, ports, dif, sdif))) {
+		if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
+			     !inet6_match(net, sk, saddr, daddr, ports, dif,
+					  sdif))) {
 			sock_gen_put(sk);
 			goto begin;
 		}
-- 
2.43.0

Re: [PATCH net v3 1/2] inet: reject unhashed sockets during established lookups

[netdev-bot+sashiko]

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

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

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] The concern: at this commit, __inet_lookup_established() and
  __inet6_lookup_established() take the reference with a relaxed…

--

inet: reject unhashed sockets during established lookups

This patch adds a hlist_nulls_unhashed_lockless() check after the
reference is taken in __inet_lookup_established() and
__inet6_lookup_established(). A SLAB_TYPESAFE_BY_RCU slot that was reused
for an unhashed socket with the same tuple is then rejected. The SYN
cookie, Fast Open and request migration failure paths now use
reqsk_put(), because requests that were never hashed can hold transient
references from stale lookups.

> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 5abcb0debb270..b0239d203f045 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -552,8 +552,9 @@ struct sock *__inet_lookup_established(const struct net *net,
>  		if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) {
>  			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
>  				goto out;
> -			if (unlikely(!inet_match(net, sk, acookie,
> -						 ports, dif, sdif))) {
> +			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
> +				     !inet_match(net, sk, acookie, ports, dif,
> +						 sdif))) {
>  				sock_gen_put(sk);
>  				goto begin;
>  			}

[Severity: High]
Is the new unhashed check ordered after the refcount increment?

refcount_inc_not_zero() is relaxed. On weakly-ordered CPUs, the loads of
sk->sk_nulls_node.pprev and of the tuple fields in the re-check could
complete before the increment does.

On the writer side, tcp_get_cookie_sock() publishes the refcount of the
never-hashed request (the object reused in the PoC) with a plain store:

net/ipv4/syncookies.c:tcp_get_cookie_sock() {
    ...
	if (child) {
		refcount_set(&req->rsk_refcnt, 1);
    ...
}

tcp_fastopen_create_child() uses the same plain refcount_set() pattern.
So does the cloned nreq in reqsk_timer_handler(),
inet_csk_complete_hashdance() and inet_csk_listen_stop().

According to include/linux/refcount.h, secondary validation under
SLAB_TYPESAFE_BY_RCU needs refcount_inc_not_zero_acquire() paired with
refcount_set_release().

Without that pairing, could a lookup see the new SYN cookie request's
nonzero refcount but still read the old object's non-NULL pprev? It
would then pass both checks and return the unhashed request, with its
NULL rsk_listener, to tcp_v4_rcv(). That is the NULL dereference in the
commit message, here on weakly-ordered architectures.

The same question applies to __inet6_lookup_established().

The next patch in this series, "inet: restore refcount ordering for
established lookups", appears to switch both lookups to
refcount_inc_not_zero_acquire(). It also converts the publishers above to
refcount_set_release(). This patch is tagged for stable. Could the
ordering change be folded into this patch, or could the two patches be
kept together for backports?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024053720.1147022-1-imv4bel%40gmail.com

[PATCH net v3 2/2] inet: restore refcount ordering for established lookups

[Hyunwoo Kim]

Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
of refcount_t"), default builds implemented refcount_inc_not_zero() using
atomic_add_unless(), which provided full memory ordering on success. The
commit removed that ordering on weakly ordered architectures, allowing
validation in established lookups to be reordered before a successful
reference acquisition.

Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
lookups. Use refcount_set_release() when setting refcounts in the SYN
cookie, TCP Fast Open, request migration, and TIME_WAIT paths. Keep the
existing TIME_WAIT smp_wmb(), which separately orders the refcount store
before ehash publication.

Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/ipv4/inet_connection_sock.c | 6 +++---
 net/ipv4/inet_hashtables.c      | 2 +-
 net/ipv4/inet_timewait_sock.c   | 2 +-
 net/ipv4/syncookies.c           | 2 +-
 net/ipv4/tcp_fastopen.c         | 2 +-
 net/ipv6/inet6_hashtables.c     | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index bd0bad96c93c3..837cb91e5b173 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -1056,8 +1056,8 @@ static void reqsk_timer_handler(struct timer_list *t)
 		 * hold another count to prevent use-after-free and
 		 * call reqsk_put() just before return.
 		 */
-		refcount_set(&nreq->rsk_refcnt, 2 + 1);
 		timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED);
+		refcount_set_release(&nreq->rsk_refcnt, 2 + 1);
 		reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req);
 
 		req = nreq;
@@ -1441,7 +1441,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child,
 				goto child_put;
 			}
 
-			refcount_set(&nreq->rsk_refcnt, 1);
+			refcount_set_release(&nreq->rsk_refcnt, 1);
 			if (inet_csk_reqsk_queue_add(sk, nreq, child)) {
 				__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQSUCCESS);
 				reqsk_migrate_reset(req);
@@ -1494,7 +1494,7 @@ void inet_csk_listen_stop(struct sock *sk)
 		if (nsk) {
 			nreq = inet_reqsk_clone(req, nsk);
 			if (nreq) {
-				refcount_set(&nreq->rsk_refcnt, 1);
+				refcount_set_release(&nreq->rsk_refcnt, 1);
 
 				rcu_read_lock();
 				if (inet_csk_reqsk_queue_add(nsk, nreq, child)) {
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 58d54965036a2..7ffa9171d6de1 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -550,7 +550,7 @@ struct sock *__inet_lookup_established(const struct net *net,
 		if (sk->sk_hash != hash)
 			continue;
 		if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) {
-			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+			if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
 				goto out;
 			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
 				     !inet_match(net, sk, acookie, ports, dif,
diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index d4c781a0667fe..88ea3a101849a 100644
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c
@@ -141,7 +141,7 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
 	 * Also note that after this point, we lost our implicit reference
 	 * so we are not allowed to use tw anymore.
 	 */
-	refcount_set(&tw->tw_refcnt, 3);
+	refcount_set_release(&tw->tw_refcnt, 3);
 
 	/* Ensure tw_refcnt has been set before tw is published.
 	 * smp_wmb() provides the necessary memory barrier to enforce this
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 6440305615378..4b2d2713653b7 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -205,7 +205,7 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
 	child = icsk->icsk_af_ops->syn_recv_sock(sk, skb, req, dst,
 						 NULL, &own_req, NULL);
 	if (child) {
-		refcount_set(&req->rsk_refcnt, 1);
+		refcount_set_release(&req->rsk_refcnt, 1);
 		sock_rps_save_rxhash(child, skb);
 
 		if (rsk_drop_req(req)) {
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 471c78be55137..e40477b6b5549 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -364,7 +364,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
 	tcp_reset_xmit_timer(child, ICSK_TIME_RETRANS,
 			     req->timeout, false);
 
-	refcount_set(&req->rsk_refcnt, 2);
+	refcount_set_release(&req->rsk_refcnt, 2);
 
 	sk_mark_napi_id_set(child, skb);
 
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index aa5fa1dd062f7..1eea584d80df8 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -105,7 +105,7 @@ struct sock *__inet6_lookup_established(const struct net *net,
 			continue;
 		if (!inet6_match(net, sk, saddr, daddr, ports, dif, sdif))
 			continue;
-		if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+		if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
 			goto out;
 
 		if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
-- 
2.43.0

Re: [PATCH net v3 2/2] inet: restore refcount ordering for established lookups

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 4

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] inet_twsk_purge() (net/ipv4/inet_timewait_sock.c) was left
  unconverted, so the fix is incomplete.
- [Low] The patch carries 'Cc: stable@vger.kernel.org' and 'Fixes:
  fb041bb7c0a9' (v5.5), but the fix depends on…
- [Low] Commit message accuracy.
- [Low] Several converted sites do not follow refcount_set_release()'s
  kernel-doc contract: 'This function should be called only after new…
- [Low] In reqsk_timer_handler() the patch moves
  timer_setup(&nreq->rsk_timer, ...) ahead of the refcount store, and…

Pre-existing issues:
- [Medium] Same ordering bug in MPTCP token lookups.

--

inet: restore refcount ordering for established lookups

This patch makes the IPv4 and IPv6 established hash lookups use
refcount_inc_not_zero_acquire(). It also converts refcount_set() to
refcount_set_release() in the SYN cookie, TCP Fast Open, request
migration and TIME_WAIT paths. The goal is to bring back ordering that
was lost when fb041bb7c0a9 made refcount_inc_not_zero() relaxed on
success.

> inet: restore refcount ordering for established lookups
>
> Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> of refcount_t"), default builds implemented refcount_inc_not_zero() using
> atomic_add_unless(), which provided full memory ordering on success. The
> commit removed that ordering on weakly ordered architectures, allowing
> validation in established lookups to be reordered before a successful
> reference acquisition.
>
> Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> lookups. Use refcount_set_release() when setting refcounts in the SYN
> cookie, TCP Fast Open, request migration, and TIME_WAIT paths. Keep the
> existing TIME_WAIT smp_wmb(), which separately orders the refcount store
> before ehash publication.

[Severity: Low]
Is "restore" accurate for the refcount_set_release() hunks?

fb041bb7c0a9 only changed the reader side. The !REFCOUNT_FULL
refcount_inc_not_zero() went from atomic_add_unless() to a relaxed
cmpxchg loop. refcount_set() was a plain atomic_set() both before and
after that commit.

So the six conversions add writer-side ordering that never existed. They
don't restore it. The six are in tcp_get_cookie_sock(),
tcp_fastopen_create_child(), reqsk_timer_handler(),
inet_csk_complete_hashdance(), inet_csk_listen_stop() and
inet_twsk_hashdance_schedule().

The changes themselves look justified. A stale reader using acquire
ordering has to see the pprev = NULL from sk_node_init() and the new tuple
fields before the new refcount. Otherwise it could match an unhashed
syncookie, TFO or migrated req.

Could the changelog explain this? It could also say how recycled request
sock slab memory makes these unhashed reqs reachable at all.

sk_clone_lock() and sock_init_data() already have an smp_wmb() before
their refcount_set(), so leaving those two alone looks right.

> Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> Cc: stable@vger.kernel.org

[Severity: Low]
refcount_inc_not_zero_acquire() and refcount_set_release() were added by
7f8ceea0c58039 ("refcount: provide ops for cases when object's memory can
be reused"). That commit is in v6.15 and not in v6.14.

The Fixes: tag points to v5.5. Without that prerequisite, none of the
touched files will build in the 5.10, 5.15, 6.1, 6.6 and 6.12 stable
trees.

Should the stable tag carry a prerequisite annotation, for example:

Cc: stable@vger.kernel.org # needs 7f8ceea0c580 ("refcount: provide ops ...")

Or is a fallback planned for older trees?

> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 01ecd98a2b501..8faef6c38b364 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -1060,8 +1060,8 @@ static void reqsk_timer_handler(struct timer_list *t)
>  		 * hold another count to prevent use-after-free and
>  		 * call reqsk_put() just before return.
>  		 */
> -		refcount_set(&nreq->rsk_refcnt, 2 + 1);
>  		timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED);
> +		refcount_set_release(&nreq->rsk_refcnt, 2 + 1);

[Severity: Low]
This isn't a bug, but timer_setup() now runs before the refcount store,
and the changelog doesn't mention the move.

It follows from the refcount_set_release() kernel-doc, which wants
initialisation done before the release. nreq is still private at this
point. A short note in the commit message would be enough.

>  		reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req);
>  
>  		req = nreq;

[ ... ]

> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index a1d86026aefb0..5c1bff5a6d4d8 100644
> --- a/net/ipv4/inet_timewait_sock.c
> +++ b/net/ipv4/inet_timewait_sock.c
> @@ -143,7 +143,7 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
>  	 * Also note that after this point, we lost our implicit reference
>  	 * so we are not allowed to use tw anymore.
>  	 */
> -	refcount_set(&tw->tw_refcnt, 3);
> +	refcount_set_release(&tw->tw_refcnt, 3);

[Severity: High]
Is the fix complete without also converting inet_twsk_purge() in this
same file?

It walks the same ehash chains locklessly under rcu_read_lock(). It
handles the same SLAB_TYPESAFE_BY_RCU TIME_WAIT and request socks, and it
still uses the relaxed variant:

net/ipv4/inet_timewait_sock.c:inet_twsk_purge() {
    ...
			if (check_net(sock_net(sk)))
				continue;

			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
				continue;

			if (check_net(sock_net(sk))) {
				sock_gen_put(sk);
				goto restart;
			}
    ...
				inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
								  req);
    ...
}

The release store added here only helps readers that pair it with an
acquire. __refcount_add_not_zero() uses atomic_try_cmpxchg_relaxed(). On
arm64 or POWER, the second check_net() and the later req->rsk_listener
load can therefore be satisfied before the CAS.

Can this sequence happen?

CPU0, in tcp_sk_exit_batch()->tcp_twsk_purge()->inet_twsk_purge() for
dying netns D:
  sees socket X in D, and the first check_net() is false

CPU1:
  X is freed
  the slot is reused for a new TIME_WAIT or request sock in live netns L
  the new sock is initialised, gets refcount_set_release(), and is hashed

CPU0:
  refcount_inc_not_zero() succeeds on the new object
  the reordered second check_net() still reads D
  inet_twsk_deschedule_put() or inet_csk_reqsk_queue_drop_and_put() runs

That tears down a TIME_WAIT or pending request sock that belongs to the
live netns L.

req->rsk_listener could also be read stale. It would then be the old
occupant's listener, which may already be released, and
inet_csk_reqsk_queue_drop() would update that listener's accept queue
counters.

An unprivileged user can trigger netns exit through user namespaces. A
remote peer can create TIME_WAIT and request socks in L.

Should inet_twsk_purge() use refcount_inc_not_zero_acquire() too?

[ ... ]

> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 471c78be55137..e40477b6b5549 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -364,7 +364,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
>  	tcp_reset_xmit_timer(child, ICSK_TIME_RETRANS,
>  			     req->timeout, false);
>  
> -	refcount_set(&req->rsk_refcnt, 2);
> +	refcount_set_release(&req->rsk_refcnt, 2);

[Severity: Low]
The kernel-doc for refcount_set_release() says:

 * This function should be called only after new object is fully initialized.
 * After this call the object should be considered visible to other tasks

Later in tcp_fastopen_create_child(), req is still written after the
release store:

	tcp_rsk(req)->rcv_nxt = tp->rcv_nxt;

The other converted sites have the same ordering:

  tcp_get_cookie_sock(), inet_csk_complete_hashdance() and
  inet_csk_listen_stop() do the release before
  inet_csk_reqsk_queue_add(), which writes req->sk and req->dl_next.

  reqsk_timer_handler() does the release before req->num_timeout++, the
  accecn_fail_mode update and mod_timer() on nreq. nreq is then hashed
  by inet_ehash_insert().

For the reqs that are never hashed, hlist_nulls_unhashed_lockless()
rejects stale established lookups, because sk_node_init() runs before the
release. So the practical exposure seems limited to minor fields such as
num_timeout on the migrated nreq.

These stores had no ordering at all before this patch, so nothing
regresses.

Could the release stores move after the last initialising write? If not,
a comment could explain why the later writes are fine.

> diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
> index aa5fa1dd062f7..1eea584d80df8 100644
> --- a/net/ipv6/inet6_hashtables.c
> +++ b/net/ipv6/inet6_hashtables.c
> @@ -105,7 +105,7 @@ struct sock *__inet6_lookup_established(const struct net *net,
>  			continue;
>  		if (!inet6_match(net, sk, saddr, daddr, ports, dif, sdif))
>  			continue;
> -		if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> +		if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
>  			goto out;

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
the MPTCP token lookups seem to have the same pattern.

mptcp_token_get_sock() in net/mptcp/token.c re-validates after a relaxed
increment:

	if (!refcount_inc_not_zero(&sk->sk_refcnt))
		goto not_found;

	if (READ_ONCE(msk->token) != token ||
	    !net_eq(sock_net(sk), net)) {

mptcp_token_iter_next() does the same thing with its net_eq() check.

mptcp_prot sets SLAB_TYPESAFE_BY_RCU for msk objects. For that reason,
mptcp_subflow_reqsk_clone() in net/mptcp/subflow.c already uses
refcount_inc_not_zero_acquire().

On weakly ordered CPUs, can these lookups return a recycled msk from
another connection or netns?

For MP_JOIN, the later HMAC check would fail and cause a spurious reset.
Through the diag iterator, an msk from another netns could be dumped.

MPTCP was merged in v5.6, after fb041bb7c0a9. These sites never had the
fully ordered behaviour, and they are outside what this patch covers.
Would a separate fix for them make sense?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024053720.1147022-1-imv4bel%40gmail.com

Re: [PATCH net v3 2/2] inet: restore refcount ordering for established lookups

[Hyunwoo Kim]

On Sun, Oct 25, 2026 at 05:40:28AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (1) · Low: 4
> 
> 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] inet_twsk_purge() (net/ipv4/inet_timewait_sock.c) was left
>   unconverted, so the fix is incomplete.
> - [Low] The patch carries 'Cc: stable@vger.kernel.org' and 'Fixes:
>   fb041bb7c0a9' (v5.5), but the fix depends on…
> - [Low] Commit message accuracy.
> - [Low] Several converted sites do not follow refcount_set_release()'s
>   kernel-doc contract: 'This function should be called only after new…
> - [Low] In reqsk_timer_handler() the patch moves
>   timer_setup(&nreq->rsk_timer, ...) ahead of the refcount store, and…
> 
> Pre-existing issues:
> - [Medium] Same ordering bug in MPTCP token lookups.
> 
> --
> 
> inet: restore refcount ordering for established lookups
> 
> This patch makes the IPv4 and IPv6 established hash lookups use
> refcount_inc_not_zero_acquire(). It also converts refcount_set() to
> refcount_set_release() in the SYN cookie, TCP Fast Open, request
> migration and TIME_WAIT paths. The goal is to bring back ordering that
> was lost when fb041bb7c0a9 made refcount_inc_not_zero() relaxed on
> success.
> 
> > inet: restore refcount ordering for established lookups
> >
> > Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> > of refcount_t"), default builds implemented refcount_inc_not_zero() using
> > atomic_add_unless(), which provided full memory ordering on success. The
> > commit removed that ordering on weakly ordered architectures, allowing
> > validation in established lookups to be reordered before a successful
> > reference acquisition.
> >
> > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> > lookups. Use refcount_set_release() when setting refcounts in the SYN
> > cookie, TCP Fast Open, request migration, and TIME_WAIT paths. Keep the
> > existing TIME_WAIT smp_wmb(), which separately orders the refcount store
> > before ehash publication.
> 
> [Severity: Low]
> Is "restore" accurate for the refcount_set_release() hunks?
> 
> fb041bb7c0a9 only changed the reader side. The !REFCOUNT_FULL
> refcount_inc_not_zero() went from atomic_add_unless() to a relaxed
> cmpxchg loop. refcount_set() was a plain atomic_set() both before and
> after that commit.
> 
> So the six conversions add writer-side ordering that never existed. They
> don't restore it. The six are in tcp_get_cookie_sock(),
> tcp_fastopen_create_child(), reqsk_timer_handler(),
> inet_csk_complete_hashdance(), inet_csk_listen_stop() and
> inet_twsk_hashdance_schedule().
> 
> The changes themselves look justified. A stale reader using acquire
> ordering has to see the pprev = NULL from sk_node_init() and the new tuple
> fields before the new refcount. Otherwise it could match an unhashed
> syncookie, TFO or migrated req.
> 
> Could the changelog explain this? It could also say how recycled request
> sock slab memory makes these unhashed reqs reachable at all.
> 
> sk_clone_lock() and sock_init_data() already have an smp_wmb() before
> their refcount_set(), so leaving those two alone looks right.
> 
> > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> > Cc: stable@vger.kernel.org
> 
> [Severity: Low]
> refcount_inc_not_zero_acquire() and refcount_set_release() were added by
> 7f8ceea0c58039 ("refcount: provide ops for cases when object's memory can
> be reused"). That commit is in v6.15 and not in v6.14.
> 
> The Fixes: tag points to v5.5. Without that prerequisite, none of the
> touched files will build in the 5.10, 5.15, 6.1, 6.6 and 6.12 stable
> trees.
> 
> Should the stable tag carry a prerequisite annotation, for example:
> 
> Cc: stable@vger.kernel.org # needs 7f8ceea0c580 ("refcount: provide ops ...")
> 
> Or is a fallback planned for older trees?
> 
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index 01ecd98a2b501..8faef6c38b364 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -1060,8 +1060,8 @@ static void reqsk_timer_handler(struct timer_list *t)
> >  		 * hold another count to prevent use-after-free and
> >  		 * call reqsk_put() just before return.
> >  		 */
> > -		refcount_set(&nreq->rsk_refcnt, 2 + 1);
> >  		timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED);
> > +		refcount_set_release(&nreq->rsk_refcnt, 2 + 1);
> 
> [Severity: Low]
> This isn't a bug, but timer_setup() now runs before the refcount store,
> and the changelog doesn't mention the move.
> 
> It follows from the refcount_set_release() kernel-doc, which wants
> initialisation done before the release. nreq is still private at this
> point. A short note in the commit message would be enough.
> 
> >  		reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req);
> >  
> >  		req = nreq;
> 
> [ ... ]
> 
> > diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> > index a1d86026aefb0..5c1bff5a6d4d8 100644
> > --- a/net/ipv4/inet_timewait_sock.c
> > +++ b/net/ipv4/inet_timewait_sock.c
> > @@ -143,7 +143,7 @@ void inet_twsk_hashdance_schedule(struct inet_timewait_sock *tw,
> >  	 * Also note that after this point, we lost our implicit reference
> >  	 * so we are not allowed to use tw anymore.
> >  	 */
> > -	refcount_set(&tw->tw_refcnt, 3);
> > +	refcount_set_release(&tw->tw_refcnt, 3);
> 
> [Severity: High]
> Is the fix complete without also converting inet_twsk_purge() in this
> same file?
> 
> It walks the same ehash chains locklessly under rcu_read_lock(). It
> handles the same SLAB_TYPESAFE_BY_RCU TIME_WAIT and request socks, and it
> still uses the relaxed variant:
> 
> net/ipv4/inet_timewait_sock.c:inet_twsk_purge() {
>     ...
> 			if (check_net(sock_net(sk)))
> 				continue;
> 
> 			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> 				continue;
> 
> 			if (check_net(sock_net(sk))) {
> 				sock_gen_put(sk);
> 				goto restart;
> 			}
>     ...
> 				inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
> 								  req);
>     ...
> }
> 
> The release store added here only helps readers that pair it with an
> acquire. __refcount_add_not_zero() uses atomic_try_cmpxchg_relaxed(). On
> arm64 or POWER, the second check_net() and the later req->rsk_listener
> load can therefore be satisfied before the CAS.
> 
> Can this sequence happen?
> 
> CPU0, in tcp_sk_exit_batch()->tcp_twsk_purge()->inet_twsk_purge() for
> dying netns D:
>   sees socket X in D, and the first check_net() is false
> 
> CPU1:
>   X is freed
>   the slot is reused for a new TIME_WAIT or request sock in live netns L
>   the new sock is initialised, gets refcount_set_release(), and is hashed
> 
> CPU0:
>   refcount_inc_not_zero() succeeds on the new object
>   the reordered second check_net() still reads D
>   inet_twsk_deschedule_put() or inet_csk_reqsk_queue_drop_and_put() runs
> 
> That tears down a TIME_WAIT or pending request sock that belongs to the
> live netns L.
> 
> req->rsk_listener could also be read stale. It would then be the old
> occupant's listener, which may already be released, and
> inet_csk_reqsk_queue_drop() would update that listener's accept queue
> counters.
> 
> An unprivileged user can trigger netns exit through user namespaces. A
> remote peer can create TIME_WAIT and request socks in L.
> 
> Should inet_twsk_purge() use refcount_inc_not_zero_acquire() too?

I will include this change in v4.

> 
> [ ... ]
> 
> > diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> > index 471c78be55137..e40477b6b5549 100644
> > --- a/net/ipv4/tcp_fastopen.c
> > +++ b/net/ipv4/tcp_fastopen.c
> > @@ -364,7 +364,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
> >  	tcp_reset_xmit_timer(child, ICSK_TIME_RETRANS,
> >  			     req->timeout, false);
> >  
> > -	refcount_set(&req->rsk_refcnt, 2);
> > +	refcount_set_release(&req->rsk_refcnt, 2);
> 
> [Severity: Low]
> The kernel-doc for refcount_set_release() says:
> 
>  * This function should be called only after new object is fully initialized.
>  * After this call the object should be considered visible to other tasks
> 
> Later in tcp_fastopen_create_child(), req is still written after the
> release store:
> 
> 	tcp_rsk(req)->rcv_nxt = tp->rcv_nxt;
> 
> The other converted sites have the same ordering:
> 
>   tcp_get_cookie_sock(), inet_csk_complete_hashdance() and
>   inet_csk_listen_stop() do the release before
>   inet_csk_reqsk_queue_add(), which writes req->sk and req->dl_next.
> 
>   reqsk_timer_handler() does the release before req->num_timeout++, the
>   accecn_fail_mode update and mod_timer() on nreq. nreq is then hashed
>   by inet_ehash_insert().
> 
> For the reqs that are never hashed, hlist_nulls_unhashed_lockless()
> rejects stale established lookups, because sk_node_init() runs before the
> release. So the practical exposure seems limited to minor fields such as
> num_timeout on the migrated nreq.
> 
> These stores had no ordering at all before this patch, so nothing
> regresses.
> 
> Could the release stores move after the last initialising write? If not,
> a comment could explain why the later writes are fine.
> 
> > diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
> > index aa5fa1dd062f7..1eea584d80df8 100644
> > --- a/net/ipv6/inet6_hashtables.c
> > +++ b/net/ipv6/inet6_hashtables.c
> > @@ -105,7 +105,7 @@ struct sock *__inet6_lookup_established(const struct net *net,
> >  			continue;
> >  		if (!inet6_match(net, sk, saddr, daddr, ports, dif, sdif))
> >  			continue;
> > -		if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> > +		if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
> >  			goto out;
> 
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch, but
> the MPTCP token lookups seem to have the same pattern.
> 
> mptcp_token_get_sock() in net/mptcp/token.c re-validates after a relaxed
> increment:
> 
> 	if (!refcount_inc_not_zero(&sk->sk_refcnt))
> 		goto not_found;
> 
> 	if (READ_ONCE(msk->token) != token ||
> 	    !net_eq(sock_net(sk), net)) {
> 
> mptcp_token_iter_next() does the same thing with its net_eq() check.
> 
> mptcp_prot sets SLAB_TYPESAFE_BY_RCU for msk objects. For that reason,
> mptcp_subflow_reqsk_clone() in net/mptcp/subflow.c already uses
> refcount_inc_not_zero_acquire().
> 
> On weakly ordered CPUs, can these lookups return a recycled msk from
> another connection or netns?
> 
> For MP_JOIN, the later HMAC check would fail and cause a spurious reset.
> Through the diag iterator, an msk from another netns could be dumped.
> 
> MPTCP was merged in v5.6, after fb041bb7c0a9. These sites never had the
> fully ordered behaviour, and they are outside what this patch covers.
> Would a separate fix for them make sense?

I had already identified the MPTCP issue. To simplify review and 
backporting, I will address it separately in a later patch rather than 
include it in this series.

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024053720.1147022-1-imv4bel%40gmail.com