[PATCH net v2 0/2] inet: fix established lookup races

[PATCH net v2 0/2] inet: fix established lookup races

[Hyunwoo Kim]

Fix two issues in lockless established socket lookups. Reject unhashed
sockets after taking a reference, and restore the acquire/release
ordering around refcount publication and acquisition.

Changes in v2:
- 1/2: split the unhashed checks and __reqsk_free() to reqsk_put()
  conversions from the ordering changes, set the Fixes tag to commit
  079096f103fa ("tcp/dccp: install syn_recv requests into ehash table"),
  mention the TIME_WAIT hashdance and request replacement cases, clarify
  request reference ownership, and drop the full socket race discussion
  (Eric)
- 2/2: new, split out the acquire/release changes, set the Fixes tag to
  commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations of
  refcount_t"), cover sk_clone(), and leave the already ordered TIME_WAIT
  path unchanged (Eric)
- v1: https://lore.kernel.org/r/ath58Egc-oI6p3Jl@v4bel

Hyunwoo Kim (2):
  inet: reject unhashed sockets during established lookups
  inet: restore refcount ordering for established lookups

 net/core/sock.c                 |  3 +--
 net/ipv4/inet_connection_sock.c | 16 ++++++++++------
 net/ipv4/inet_hashtables.c      |  7 ++++---
 net/ipv4/syncookies.c           |  4 +++-
 net/ipv4/tcp_fastopen.c         |  3 +--
 net/ipv4/tcp_input.c            |  4 +++-
 net/ipv6/inet6_hashtables.c     |  6 ++++--
 7 files changed, 26 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH net v2 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>
---
 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 6a30f1138454..cff02f4c5ac8 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 ehash and temporary references. */
+		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 ba0faa9ae2bb..58d54965036a 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 73e129768184..644030561537 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 99baf14afdfd..3583867487d7 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 b111b51d69fc..aa5fa1dd062f 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 v2 1/2] inet: reject unhashed sockets during established lookups

[Eric Dumazet]

Le ven. 23 oct. 2026 à 07:26, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
>
> 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>
> ---
>  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 6a30f1138454..cff02f4c5ac8 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 ehash and temporary references. */

Lets change this comment to :
/* Drop the temporary reference and the one reserved for ehash. */

Reviewed-by: Eric Dumazet <edumazet@kernel.org>

> +               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 ba0faa9ae2bb..58d54965036a 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 73e129768184..644030561537 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 99baf14afdfd..3583867487d7 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 b111b51d69fc..aa5fa1dd062f 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 v2 1/2] inet: reject unhashed sockets during established lookups

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] At this commit, the new post-reference checks
  hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) and…

--

inet: reject unhashed sockets during established lookups

This commit adds an hlist_nulls_unhashed_lockless() check after the
reference is taken in __inet_lookup_established() and
__inet6_lookup_established(). A lookup that races with
SLAB_TYPESAFE_BY_RCU reuse of a socket now restarts instead of returning
an unhashed object. The SYN cookie, Fast Open and request migration
failure paths now drop references with reqsk_put() instead of calling
__reqsk_free() directly.

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

[Severity: Low]
On weakly ordered architectures, are the new pprev load and the repeated
tuple check guaranteed to happen after the refcount increment?

refcount_inc_not_zero() is relaxed here. At this commit, the code that
publishes a nonzero refcount on never-hashed request sockets also uses
plain stores:

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

net/ipv4/tcp_fastopen.c:tcp_fastopen_create_child() {
    ...
	refcount_set(&req->rsk_refcnt, 2);
    ...
}

The inet_reqsk_clone() users do the same. They are reqsk_timer_handler()
(refcount_set(&nreq->rsk_refcnt, 2 + 1)), inet_csk_complete_hashdance()
and inet_csk_listen_stop() (refcount_set(&nreq->rsk_refcnt, 1)).

On arm64 or POWER, the hlist_nulls_unhashed_lockless() and inet_match()
loads could be satisfied before the cmpxchg in refcount_inc_not_zero().
If so, the lookup could still return the recycled, unhashed SYN cookie
reqsk with rsk_listener == NULL. That is the tcp_v4_rcv() crash described
in the commit message. The same applies to __inet6_lookup_established(),
which also uses a relaxed refcount_inc_not_zero() before the new check.

This ordering is older than this patch. It is fully fixed by the next
patch in the series, "inet: restore refcount ordering for established
lookups". That patch switches both lookups to
refcount_inc_not_zero_acquire(), and switches tcp_get_cookie_sock(),
tcp_fastopen_create_child() (moving the set after initialization),
reqsk_timer_handler(), inet_csk_complete_hashdance(),
inet_csk_listen_stop() and sk_clone() to refcount_set_release().

Both patches carry Cc: stable. Should they be marked so that they are
backported together, so that this one is not picked up alone?

>  				sock_gen_put(sk);
>  				goto begin;
>  			}

[ ... ]

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

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

[Hyunwoo Kim]

When commit 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use
RCU / hlist_nulls") was written, atomic_inc_not_zero() provided full
memory ordering. Even after commit 41c6d650f653 ("net: convert
sock.sk_refcnt from atomic_t to refcount_t"), default builds used
atomic_add_unless() and retained that ordering.

Commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
of refcount_t") consolidated the refcount_t implementations in v5.5.
On weakly ordered architectures, this removed the ordering required
by validation after a successful reference acquisition.

Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
lookups. Use refcount_set_release() when setting initial refcounts in
sk_clone(), SYN cookie, and request migration paths. In the TCP Fast
Open path, move the refcount update after the required initialization
and use release ordering.

The TIME_WAIT path already has an smp_wmb() between refcount_set() and
hash publication, so leave it unchanged.

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

diff --git a/net/core/sock.c b/net/core/sock.c
index 3c25d66ba31a..f3ef9601ab73 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2622,8 +2622,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
 	/* Before updating sk_refcnt, we must commit prior changes to memory
 	 * (Documentation/RCU/rculist_nulls.rst for details)
 	 */
-	smp_wmb();
-	refcount_set(&newsk->sk_refcnt, 2);
+	refcount_set_release(&newsk->sk_refcnt, 2);
 
 	sk_set_socket(newsk, NULL);
 	sk_tx_queue_clear(newsk);
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index cff02f4c5ac8..2d8e54d3a31b 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 58d54965036a..7ffa9171d6de 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/syncookies.c b/net/ipv4/syncookies.c
index 644030561537..4b2d2713653b 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 471c78be5513..2506f415f410 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -364,8 +364,6 @@ 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);
-
 	sk_mark_napi_id_set(child, skb);
 
 	/* Now finish processing the fastopen child socket. */
@@ -378,6 +376,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
 	tcp_rsk(req)->rcv_nxt = tp->rcv_nxt;
 	tp->rcv_wup = tp->rcv_nxt;
 	tp->rcv_mwnd_seq = tp->rcv_wup + tp->rcv_wnd;
+	refcount_set_release(&req->rsk_refcnt, 2);
 	/* tcp_conn_request() is sending the SYNACK,
 	 * and queues the child into listener accept queue.
 	 */
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index aa5fa1dd062f..1eea584d80df 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 v2 2/2] inet: restore refcount ordering for established lookups

[Eric Dumazet]

Le ven. 23 oct. 2026 à 07:26, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
>
> When commit 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use
> RCU / hlist_nulls") was written, atomic_inc_not_zero() provided full
> memory ordering. Even after commit 41c6d650f653 ("net: convert
> sock.sk_refcnt from atomic_t to refcount_t"), default builds used
> atomic_add_unless() and retained that ordering.
>
> Commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> of refcount_t") consolidated the refcount_t implementations in v5.5.
> On weakly ordered architectures, this removed the ordering required
> by validation after a successful reference acquisition.
>
> Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> lookups. Use refcount_set_release() when setting initial refcounts in
> sk_clone(), SYN cookie, and request migration paths. In the TCP Fast
> Open path, move the refcount update after the required initialization
> and use release ordering.
>
> The TIME_WAIT path already has an smp_wmb() between refcount_set() and
> hash publication, so leave it unchanged.

This is not accurate. The smp_wmb() in inet_twsk_hashdance_schedule()
orders the tw_refcnt store against the ehash publication, not the tw
identity stores done in inet_twsk_alloc() against the tw_refcnt store.

The latter is what a lookup holding a stale pointer needs. Today we get it
more or less by accident from the bhash spin_unlock() followed by
spin_lock() of the ehash lock.

Please either reword this paragraph, or simply use
refcount_set_release(&tw->tw_refcnt, 3) there; it is free under the lock
and makes the pairing explicit.


>
> Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")

refcount_inc_not_zero_acquire() and refcount_set_release() were only
added in 7f8ceea0c580 ("refcount: provide ops for cases when object's
memory can be reused"), in v6.15.


> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>  net/core/sock.c                 | 3 +--
>  net/ipv4/inet_connection_sock.c | 6 +++---
>  net/ipv4/inet_hashtables.c      | 2 +-
>  net/ipv4/syncookies.c           | 2 +-
>  net/ipv4/tcp_fastopen.c         | 3 +--
>  net/ipv6/inet6_hashtables.c     | 2 +-
>  6 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 3c25d66ba31a..f3ef9601ab73 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2622,8 +2622,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>         /* Before updating sk_refcnt, we must commit prior changes to memory
>          * (Documentation/RCU/rculist_nulls.rst for details)
>          */
> -       smp_wmb();
> -       refcount_set(&newsk->sk_refcnt, 2);
> +       refcount_set_release(&newsk->sk_refcnt, 2);

This part is not a fix: the writer side only needs store->store ordering,
which smp_wmb() already provides.

I do not mind the conversion, but then reqsk_queue_hash_req() (which has
the same smp_wmb() + refcount_set() pattern) should be converted as well
for consistency. Otherwise please drop this hunk to keep the stable
backport minimal.


>
>         sk_set_socket(newsk, NULL);
>         sk_tx_queue_clear(newsk);
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index cff02f4c5ac8..2d8e54d3a31b 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 58d54965036a..7ffa9171d6de 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/syncookies.c b/net/ipv4/syncookies.c
> index 644030561537..4b2d2713653b 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 471c78be5513..2506f415f410 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -364,8 +364,6 @@ 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);
> -
>         sk_mark_napi_id_set(child, skb);
>
>         /* Now finish processing the fastopen child socket. */
> @@ -378,6 +376,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
>         tcp_rsk(req)->rcv_nxt = tp->rcv_nxt;
>         tp->rcv_wup = tp->rcv_nxt;
>         tp->rcv_mwnd_seq = tp->rcv_wup + tp->rcv_wnd;
> +       refcount_set_release(&req->rsk_refcnt, 2);
>         /* tcp_conn_request() is sending the SYNACK,

Moving this is harmless, but not needed. The lookup never looks at
rcv_nxt, and this req is never hashed, so after patch 1/2 a stale lookup
will reject it anyway. A plain conversion in place would be enough.

Let's keep code churn minimal, to lower risk of merge conflicts.

>          * and queues the child into listener accept queue.
>          */
> diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
> index aa5fa1dd062f..1eea584d80df 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 v2 2/2] inet: restore refcount ordering for established lookups

[Hyunwoo Kim]

On Fri, Oct 23, 2026 at 06:33:53PM +0200, Eric Dumazet wrote:
> Le ven. 23 oct. 2026 à 07:26, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
> >
> > When commit 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use
> > RCU / hlist_nulls") was written, atomic_inc_not_zero() provided full
> > memory ordering. Even after commit 41c6d650f653 ("net: convert
> > sock.sk_refcnt from atomic_t to refcount_t"), default builds used
> > atomic_add_unless() and retained that ordering.
> >
> > Commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> > of refcount_t") consolidated the refcount_t implementations in v5.5.
> > On weakly ordered architectures, this removed the ordering required
> > by validation after a successful reference acquisition.
> >
> > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> > lookups. Use refcount_set_release() when setting initial refcounts in
> > sk_clone(), SYN cookie, and request migration paths. In the TCP Fast
> > Open path, move the refcount update after the required initialization
> > and use release ordering.
> >
> > The TIME_WAIT path already has an smp_wmb() between refcount_set() and
> > hash publication, so leave it unchanged.
> 
> This is not accurate. The smp_wmb() in inet_twsk_hashdance_schedule()
> orders the tw_refcnt store against the ehash publication, not the tw
> identity stores done in inet_twsk_alloc() against the tw_refcnt store.
> 
> The latter is what a lookup holding a stale pointer needs. Today we get it
> more or less by accident from the bhash spin_unlock() followed by
> spin_lock() of the ehash lock.
> 
> Please either reword this paragraph, or simply use
> refcount_set_release(&tw->tw_refcnt, 3) there; it is free under the lock
> and makes the pairing explicit.

OK, I'll switch to refcount_set_release().

> 
> 
> >
> > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> 
> refcount_inc_not_zero_acquire() and refcount_set_release() were only
> added in 7f8ceea0c580 ("refcount: provide ops for cases when object's
> memory can be reused"), in v6.15.
> 
> 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > ---
> >  net/core/sock.c                 | 3 +--
> >  net/ipv4/inet_connection_sock.c | 6 +++---
> >  net/ipv4/inet_hashtables.c      | 2 +-
> >  net/ipv4/syncookies.c           | 2 +-
> >  net/ipv4/tcp_fastopen.c         | 3 +--
> >  net/ipv6/inet6_hashtables.c     | 2 +-
> >  6 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 3c25d66ba31a..f3ef9601ab73 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -2622,8 +2622,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
> >         /* Before updating sk_refcnt, we must commit prior changes to memory
> >          * (Documentation/RCU/rculist_nulls.rst for details)
> >          */
> > -       smp_wmb();
> > -       refcount_set(&newsk->sk_refcnt, 2);
> > +       refcount_set_release(&newsk->sk_refcnt, 2);
> 
> This part is not a fix: the writer side only needs store->store ordering,
> which smp_wmb() already provides.
> 
> I do not mind the conversion, but then reqsk_queue_hash_req() (which has
> the same smp_wmb() + refcount_set() pattern) should be converted as well
> for consistency. Otherwise please drop this hunk to keep the stable
> backport minimal.
> 
> 
> >
> >         sk_set_socket(newsk, NULL);
> >         sk_tx_queue_clear(newsk);
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index cff02f4c5ac8..2d8e54d3a31b 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 58d54965036a..7ffa9171d6de 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/syncookies.c b/net/ipv4/syncookies.c
> > index 644030561537..4b2d2713653b 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 471c78be5513..2506f415f410 100644
> > --- a/net/ipv4/tcp_fastopen.c
> > +++ b/net/ipv4/tcp_fastopen.c
> > @@ -364,8 +364,6 @@ 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);
> > -
> >         sk_mark_napi_id_set(child, skb);
> >
> >         /* Now finish processing the fastopen child socket. */
> > @@ -378,6 +376,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
> >         tcp_rsk(req)->rcv_nxt = tp->rcv_nxt;
> >         tp->rcv_wup = tp->rcv_nxt;
> >         tp->rcv_mwnd_seq = tp->rcv_wup + tp->rcv_wnd;
> > +       refcount_set_release(&req->rsk_refcnt, 2);
> >         /* tcp_conn_request() is sending the SYNACK,
> 
> Moving this is harmless, but not needed. The lookup never looks at
> rcv_nxt, and this req is never hashed, so after patch 1/2 a stale lookup
> will reject it anyway. A plain conversion in place would be enough.
> 
> Let's keep code churn minimal, to lower risk of merge conflicts.

I'll submit v3 with all unnecessary changes removed. Thanks for the review.


Best regards,
Hyunwoo Kim

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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] inet: premature refcount_set_release() in sk_clone() and
  reqsk_timer_handler()
- [Medium] The patch converts only __inet_lookup_established() and
  __inet6_lookup_established() to refcount_inc_not_zero_acquire().

--

inet: restore refcount ordering for established lookups

This patch converts the IPv4 and IPv6 established hash lookups to
refcount_inc_not_zero_acquire(). It also converts the code that sets
initial refcounts in sk_clone(), SYN cookies, request migration and TCP
Fast Open to refcount_set_release(). The goal is to restore the ordering
that weakly ordered architectures lost after the refcount_t
consolidation in v5.5.

> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index b0239d203f045..860145d810ed4 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;

[Severity: Medium]
This converts __inet_lookup_established() and
__inet6_lookup_established(). Other lockless readers of
SLAB_TYPESAFE_BY_RCU sockets take a reference and then revalidate the
object in the same way. Are those readers still using the relaxed
refcount_inc_not_zero()?

inet_twsk_purge() in net/ipv4/inet_timewait_sock.c walks the same ehash
chains and handles TIME_WAIT and NEW_SYN_RECV sockets:

net/ipv4/inet_timewait_sock.c:inet_twsk_purge() {
    ...
			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
				continue;

			if (check_net(sock_net(sk))) {
				sock_gen_put(sk);
				goto restart;
			}
    ...
}

The twsk and request sock slabs are created with prot->slab_flags. For
tcp_prot and tcpv6_prot, that is SLAB_TYPESAFE_BY_RCU. reqsk_timer_handler()
inserts nreq into ehash, and this patch changes it to set nreq's refcount
with refcount_set_release(). inet_twsk_purge() has no matching acquire.

On a weakly ordered CPU, the check_net() load can be satisfied before the
increment. In that case, could inet_twsk_purge() call
inet_twsk_deschedule_put() or inet_csk_reqsk_queue_drop_and_put() on a
recycled object that belongs to a live netns?

It also reads req->rsk_listener after rcu_read_unlock(), with no ordering
against the reference acquisition.

MPTCP has the same pattern on mptcp_sock. mptcp_prot also uses
SLAB_TYPESAFE_BY_RCU in net/mptcp/protocol.c:

net/mptcp/token.c:mptcp_token_get_sock() {
    ...
		if (!refcount_inc_not_zero(&sk->sk_refcnt))
			goto not_found;

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

net/mptcp/token.c:mptcp_token_iter_next() {
    ...
			if (!refcount_inc_not_zero(&sk->sk_refcnt))
				continue;

			if (!net_eq(sock_net(sk), net)) {
    ...
}

net/mptcp/subflow.c already uses refcount_inc_not_zero_acquire() for the
msk.

__refcount_add_not_zero() uses atomic_try_cmpxchg_relaxed(). The
refcount.h documentation says to use the acquire variant when the
increment is followed by validation on SLAB_TYPESAFE_BY_RCU objects.

Could mptcp_token_get_sock() return the wrong msk for a remote MP_JOIN
SYN? Could mptcp_token_iter_next() return an msk from another netns to
the PM or diag iterators? For the MP_JOIN case, the HMAC check would
most likely reject a wrong msk.

The commit message describes this class of bug as removing "the ordering
required by validation after a successful reference acquisition". Should
these readers also move to refcount_inc_not_zero_acquire(), either in
this patch or in a follow-up?

>  			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
>  				     !inet_match(net, sk, acookie, ports, dif,

[ ... ]

> diff --git a/net/core/sock.c b/net/core/sock.c
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2622,8 +2622,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>  	/* Before updating sk_refcnt, we must commit prior changes to memory
>  	 * (Documentation/RCU/rculist_nulls.rst for details)
>  	 */
> -	smp_wmb();
> -	refcount_set(&newsk->sk_refcnt, 2);
> +	refcount_set_release(&newsk->sk_refcnt, 2);
>  
>  	sk_set_socket(newsk, NULL);
>  	sk_tx_queue_clear(newsk);

[Severity: High]
Is this release placed late enough to cover the fields that the
established lookup revalidates?

sk_clone() publishes sk_refcnt here, but several stores still follow
it before the socket is linked into ehash. These include
sk_set_socket(newsk, NULL), sk_tx_queue_clear(), sk_rx_queue_clear()
and the sk_wq reset right below. They also include the tuple fields
(sk_daddr, sk_rcv_saddr, inet_dport, inet_num, sk_v6_daddr, ...) and
inet_sk_set_state(newsk, TCP_SYN_RECV) in inet_csk_clone_lock(). The
child setup in tcp_create_openreq_child() and tcp_v4_syn_recv_sock()
or tcp_v6_syn_recv_sock() comes after that.

Only then does inet_ehash_insert() link the socket. The pprev store in
__sk_nulls_add_node_rcu() or sk_nulls_replace_node_init_rcu() is a
plain WRITE_ONCE() and does not touch sk_refcnt.

Consider a reader in __inet_lookup_established() or
__inet6_lookup_established() that picked up the object pointer during
the object's previous life in the SLAB_TYPESAFE_BY_RCU cache. That
reader never traverses the newly published chain link:

  refcount_inc_not_zero_acquire() succeeds and pairs with the
  release in sk_clone()
  hlist_nulls_unhashed_lockless() reads pprev with READ_ONCE()
  inet_match() / inet6_match() read the tuple with relaxed loads

The acquire only orders the reader against stores made before the
release. The pprev and tuple values are written after the release,
so the reader can observe them with no ordering at all.

On a weakly ordered CPU, the lookup could then pass both checks and
return the socket while still seeing values that sock_copy() took
from the listener. Examples are sk_state == TCP_LISTEN, the
listener's sk_socket and sk_wq, or TCP fields that are not yet
initialized. Wouldn't tcp_v4_rcv() or tcp_v6_rcv() then process the
packet against a half-initialized socket?

reqsk_timer_handler() has the same problem. Moving the release after
timer_setup() helps, but several stores still run between
refcount_set_release(&nreq->rsk_refcnt, 2 + 1) and
inet_ehash_insert(req_to_sk(nreq), req_to_sk(oreq), NULL). These are
tcp_rtx_synack(), the accecn_fail_mode update, nreq->num_timeout++
and mod_timer(&nreq->rsk_timer, ...). A stale reader that sees nreq
hashed has no ordering against any of them.

The release cannot simply be moved past mod_timer() either. Once the
timer is armed, it can already drop references.

By contrast, the TIME_WAIT path that the commit message calls
correct does its smp_wmb() and refcount_set() after
inet_twsk_add_node_rcu(), if I read inet_twsk_hashdance_schedule()
correctly. There the refcount only becomes non-zero after the hash
linkage and every earlier store. Documentation/RCU/rculist_nulls.rst
likewise writes the key before the release on the refcount.

This gap already existed with the old smp_wmb() and refcount_set().
This patch sets out to restore the ordering that the lookup
revalidation depends on, though, so it looks incomplete.

How would you like to close the gap? One option is to make sk_refcnt
visible only after the socket is fully initialized and hashed.
Another is to pair an acquire load of pprev in the lookup with a
release store on the insert side. I'm not sure either option fits the
sk_clone() callers and the request timer's refcount requirements.

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

Re: [PATCH net v2 0/2] inet: fix established lookup races

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

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error 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.