[PATCH net] inet: reject unhashed sockets during established lookups

[PATCH net] 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

The same stale address pattern also affects full sockets. In an x86
KASAN test with enlarged race windows, a stale lookup allowed an RST to
change a child socket to TCP_CLOSE while it was being initialized and
before ehash insertion. The creation path then inserted the child, and
the server accepted it in TCP_CLOSE. The child was later freed while
still hashed, and a subsequent lookup reported a use-after-free.

Use refcount_inc_not_zero_acquire() when acquiring the reference. After
the reference is acquired, restart the lookup if the nulls node is
unhashed or the tuple no longer matches. This rejects the SYN cookie
request socket and blocks the child socket path observed in the x86
test.

Use release ordering when setting initial nonzero refcounts in the SYN
cookie, TCP Fast Open, request migration, and TIME_WAIT paths. This
makes initialization performed before the refcount update visible to a
reader that acquires a reference. After a nonzero request refcount has
been published, release it with reqsk_put() on failure instead of
freeing the request directly.

Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/ipv4/inet_connection_sock.c | 14 ++++++++------
 net/ipv4/inet_hashtables.c      |  7 ++++---
 net/ipv4/inet_timewait_sock.c   |  2 +-
 net/ipv4/syncookies.c           |  4 +++-
 net/ipv4/tcp_fastopen.c         |  3 +--
 net/ipv4/tcp_input.c            |  3 ++-
 net/ipv6/inet6_hashtables.c     |  5 +++--
 7 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 6a30f11384547e..25b46beed63541 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;
@@ -1136,10 +1136,12 @@ static void reqsk_timer_handler(struct timer_list *t)
 	 */
 	if (nreq) {
 		__NET_INC_STATS(net, LINUX_MIB_TCPMIGRATEREQFAILURE);
+		reqsk_put(nreq);
 no_ownership:
 		reqsk_migrate_reset(nreq);
 		reqsk_queue_removed(queue, nreq);
-		__reqsk_free(nreq);
+		reqsk_put(nreq);
+		reqsk_put(nreq);
 	}
 
 drop:
@@ -1437,7 +1439,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);
@@ -1447,7 +1449,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;
 		}
@@ -1490,7 +1492,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)) {
@@ -1502,7 +1504,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 ba0faa9ae2bb3a..7ffa9171d6de10 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -550,10 +550,11 @@ 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(!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/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index d4c781a0667fe5..88ea3a101849aa 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 73e12976818473..4b2d2713653b75 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)) {
@@ -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_fastopen.c b/net/ipv4/tcp_fastopen.c
index 471c78be55137a..2506f415f410bb 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/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 99baf14afdfd8f..131aa475e35f69 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -7777,7 +7777,8 @@ 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;
+			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 b111b51d69fc9b..7468aca5c1f1ee 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -105,10 +105,11 @@ 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(!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] inet: reject unhashed sockets during established lookups

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

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] inet: reject unhashed sockets during established lookups

[Eric Dumazet]

Le mer. 21 oct. 2026 à 10:38, 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.
>

Thanks for the report.

Rejecting unhashed sockets after taking the reference makes sense:
syncookie, fastopen and migrated request sockets (and children before
inet_ehash_nolisten()) can all have a non zero refcount while never
being in ehash. All allocation paths call sk_node_init(), and all
removals use the _init_rcu() variants, so the pprev test is reliable,
and pprev shares a cache line with skc_refcnt.

Please mention in the changelog that lookups racing with the
TIME_WAIT hashdance, or with a request being replaced by its child,
will now restart and find the new socket, instead of returning the
old one.

> 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
>   ...
>
>
> The same stale address pattern also affects full sockets. In an x86
> KASAN test with enlarged race windows, a stale lookup allowed an RST to
> change a child socket to TCP_CLOSE while it was being initialized and
> before ehash insertion.

I do not see how this can happen. The child is bh-locked from
sk_clone() until tcp_child_process() (or until the bh_unlock_sock()
in tcp_conn_request() for fastopen), and inet_ehash_nolisten() is
called while the lock is held. A stale reader in tcp_v4_rcv() would
spin in bh_lock_sock_nested() until the child is hashed.

Please provide the exact interleaving, or remove this paragraph.
"Enlarged race windows" can easily create schedules that are not
possible on a real kernel.

Do not add bugs in the instrumented kernel just to pretend things.



> The creation path then inserted the child, and
> the server accepted it in TCP_CLOSE. The child was later freed while
> still hashed, and a subsequent lookup reported a use-after-free.
>
> Use refcount_inc_not_zero_acquire() when acquiring the reference.

This is a separate issue, with a different Fixes: tag.

When 3ab5aee7fe84 was written, sk_refcnt was an atomic_t and
atomic_inc_not_zero() was fully ordered. Even after the conversion to
refcount_t (41c6d650f653), default builds used atomic_add_unless().
The ordering was only lost on weakly ordered arches in v5.5, with
commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
of refcount_t").

Please split this into a series:

1) Unhashed check in the lookups, and the __reqsk_free() -> reqsk_put()
   conversions (these are needed because a stale reader can hold a
   transient reference on a request that was never hashed).
   Fixes: 079096f103fa ("tcp/dccp: install syn_recv requests into ehash table")

2) The acquire/release conversions.

If you convert writers to refcount_set_release(), please be
consistent: sk_clone() still uses refcount_set(&newsk->sk_refcnt, 2),
and this is precisely the full socket case you mention.
On the other hand, inet_twsk_hashdance_schedule() already has an
smp_wmb() right after the refcount_set(), so changing it is not
needed.


> After the reference is acquired, restart the lookup if the nulls node is
> unhashed or the tuple no longer matches. This rejects the SYN cookie
> request socket and blocks the child socket path observed in the x86
> test.
>
> Use release ordering when setting initial nonzero refcounts in the SYN
> cookie, TCP Fast Open, request migration, and TIME_WAIT paths. This
> makes initialization performed before the refcount update visible to a
> reader that acquires a reference. After a nonzero request refcount has
> been published, release it with reqsk_put() on failure instead of
> freeing the request directly.
>
> Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>  net/ipv4/inet_connection_sock.c | 14 ++++++++------
>  net/ipv4/inet_hashtables.c      |  7 ++++---
>  net/ipv4/inet_timewait_sock.c   |  2 +-
>  net/ipv4/syncookies.c           |  4 +++-
>  net/ipv4/tcp_fastopen.c         |  3 +--
>  net/ipv4/tcp_input.c            |  3 ++-
>  net/ipv6/inet6_hashtables.c     |  5 +++--
>  7 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 6a30f11384547e..25b46beed63541 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;
> @@ -1136,10 +1136,12 @@ static void reqsk_timer_handler(struct timer_list *t)
>          */
>         if (nreq) {
>                 __NET_INC_STATS(net, LINUX_MIB_TCPMIGRATEREQFAILURE);
> +               reqsk_put(nreq);
>  no_ownership:
>                 reqsk_migrate_reset(nreq);
>                 reqsk_queue_removed(queue, nreq);
> -               __reqsk_free(nreq);
> +               reqsk_put(nreq);
> +               reqsk_put(nreq);

The accounting looks right on both paths, but three reqsk_put() split
around a label are hard to follow. Please add a comment explaining
which references are released on each path.

>         }
>
>  drop:
> @@ -1437,7 +1439,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);
> @@ -1447,7 +1449,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;
>                 }
> @@ -1490,7 +1492,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)) {
> @@ -1502,7 +1504,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 ba0faa9ae2bb3a..7ffa9171d6de10 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -550,10 +550,11 @@ 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(!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/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index d4c781a0667fe5..88ea3a101849aa 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 73e12976818473..4b2d2713653b75 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)) {
> @@ -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_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 471c78be55137a..2506f415f410bb 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/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 99baf14afdfd8f..131aa475e35f69 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -7777,7 +7777,8 @@ 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;
> +                       reqsk_put(req);
> +                       goto drop;

This leaks the request.

tcp_fastopen_create_child() set rsk_refcnt to 2, and
inet_child_forget() cleared tp->fastopen_rsk without releasing its
reference. A single reqsk_put() leaves the refcount at 1, leaking the
request, its rsk_listener reference, and therefore the netns.

You need to drop both references here.

>                 }
>                 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 b111b51d69fc9b..7468aca5c1f1ee 100644
> --- a/net/ipv6/inet6_hashtables.c
> +++ b/net/ipv6/inet6_hashtables.c
> @@ -105,10 +105,11 @@ 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(!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] inet: reject unhashed sockets during established lookups

[Hyunwoo Kim]

On Wed, Oct 21, 2026 at 11:04:58AM +0200, Eric Dumazet wrote:
> Le mer. 21 oct. 2026 à 10:38, 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.
> >
> 
> Thanks for the report.
> 
> Rejecting unhashed sockets after taking the reference makes sense:
> syncookie, fastopen and migrated request sockets (and children before
> inet_ehash_nolisten()) can all have a non zero refcount while never
> being in ehash. All allocation paths call sk_node_init(), and all
> removals use the _init_rcu() variants, so the pprev test is reliable,
> and pprev shares a cache line with skc_refcnt.
> 
> Please mention in the changelog that lookups racing with the
> TIME_WAIT hashdance, or with a request being replaced by its child,
> will now restart and find the new socket, instead of returning the
> old one.
> 
> > 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
> >   ...
> >
> >
> > The same stale address pattern also affects full sockets. In an x86
> > KASAN test with enlarged race windows, a stale lookup allowed an RST to
> > change a child socket to TCP_CLOSE while it was being initialized and
> > before ehash insertion.
> 
> I do not see how this can happen. The child is bh-locked from
> sk_clone() until tcp_child_process() (or until the bh_unlock_sock()
> in tcp_conn_request() for fastopen), and inet_ehash_nolisten() is
> called while the lock is held. A stale reader in tcp_v4_rcv() would
> spin in bh_lock_sock_nested() until the child is hashed.

inet_ehash_nolisten() is called with the child lock held. A child created
by sk_clone_lock() briefly inherits TCP_LISTEN from the listener, and its
tuple is set before the transition to TCP_SYN_RECV.

[1] If a stale lookup returns the child during this window, the
TCP_LISTEN test in tcp_v4_rcv() succeeds, so bh_lock_sock_nested() is
skipped. If the child transitions to TCP_SYN_RECV immediately
afterward, the reader can continue into tcp_v4_do_rcv() without taking
the lock.

The child lock therefore does not serialize this path with
inet_ehash_nolisten().

> 
> Please provide the exact interleaving, or remove this paragraph.

The interleaving is a little involved, but the race is as follows.
Should I include this diagram in the changelog, or simply remove the
reference from the changelog?

Personally, I think it would be better to simply remove the paragraph.

           stale RST RX                 server task                 cookie ACK RX


                              listener = socket(AF_INET, SOCK_STREAM, 0)
                              bind(listener, 0.0.0.0:8080)
                              listen(listener, 1)
                              ...
                              A = accept(listener)
                              recv(A)

tcp_v4_rcv(RST) // processes a fragmented RST with the expected sequence number from the peer
  __inet_lookup_established()
    inet_match(A)

                              setsockopt(A, SO_LINGER, {1, 0})
                              close(A)
                                inet_release(A)
                                  tcp_close(A)
                                    __tcp_close(A)
                                      tcp_disconnect(A)
                                        tcp_set_state(A, TCP_CLOSE)
                                          inet_unhash(A)
                                    ...
                                    sock_put(A)
                                      sk_free(A)
                                        kmem_cache_free(TCP, A)

                                                          tcp_v4_rcv(ACK) // processes another SYN cookie ACK for the same tuple from the peer
                                                            tcp_v4_do_rcv(listener)
                                                              tcp_v4_cookie_check()
                                                                cookie_v4_check()
                                                                  tcp_get_cookie_sock()
                                                                    tcp_v4_syn_recv_sock()
                                                                      tcp_create_openreq_child()
                                                                        inet_csk_clone_lock()
                                                                          sk_clone_lock() // creates B locked and in TCP_LISTEN
                                                                          set B tuple // sets B's addresses and ports to match A

    refcount_inc_not_zero(&B->sk_refcnt) // increments B's refcount, not A's
    inet_match(B)
    return B

    ======================== RACE 1 ========================

tcp_v4_rcv(RST) // [1] skips bh_lock_sock_nested(B) when B is observed in TCP_LISTEN

                                                          inet_sk_set_state(B, TCP_SYN_RECV) // changes state after stale RX has skipped the locking path
                                                          WRITE_ONCE(B->rcv_nxt, seq) // sets the value required for the exact sequence check

  tcp_v4_do_rcv(B, RST) // stale RX processes the RST without waiting for B's lock
    tcp_rcv_state_process()
      tcp_validate_incoming()
        tcp_reset()
          tcp_done_with_error()
            tcp_done()
              tcp_set_state(B, TCP_CLOSE)
                inet_unhash(B) // does nothing because B has not been hashed yet

    ======================== RACE 2 ========================

                                                          inet_ehash_nolisten(B) // B remains locked, but stale RX has already bypassed the lock and moved B to TCP_CLOSE
                                                          inet_csk_reqsk_queue_add(listener, req, B)
                                                          tcp_child_process(listener, B, ACK)
                                                            tcp_rcv_state_process(B, ACK)
                                                            bh_unlock_sock(B)
                                                          sock_put(B)

  sock_put(B)

                              B = accept(listener)
                                inet_accept()
                                  inet_csk_accept()
                                    reqsk_queue_remove()
                                  __inet_accept()
                                    sock_graft(B)

                              send(B)

                              close(B)
                                inet_release(B)
                                  tcp_close(B)
                                    __tcp_close(B) // B is already TCP_CLOSE, so inet_unhash() is not called
                                      sock_hold(B)
                                      sock_orphan(B)
                                      inet_csk_destroy_sock(B)
                                        WARN_ON(!sk_unhashed(B))
                                        sock_put(B)
                                    sock_put(B)
                                      sk_free(B)
                                        __sk_free(B)
                                          sk_destruct(B)
                                            __sk_destruct(B)
                                              sk_prot_free(B)
                                                kmem_cache_free(TCP, B) // B is freed while its node remains in the ehash

                                                          net_rx_action() // processes a subsequent ACK for the same tuple from the peer
                                                            ...
                                                            ip_rcv_finish_core()
                                                              tcp_v4_early_demux()
                                                                __inet_lookup_established()
                                                                  if (B->sk_hash != hash) // use-after-fre

> "Enlarged race windows" can easily create schedules that are not
> possible on a real kernel.
> 
> Do not add bugs in the instrumented kernel just to pretend things.

The instrumented kernel did not otherwise change the control flow. I
only added "mdelay()" calls at those points to observe the race
deterministically. The scenario is involved, but I believe the same
interleaving can occur with enough attempts and concurrent processing.

In any case, this "Enlarged race windows" discussion does not belong 
in the changelog. I will remove it.

> 
> 
> 
> > The creation path then inserted the child, and
> > the server accepted it in TCP_CLOSE. The child was later freed while
> > still hashed, and a subsequent lookup reported a use-after-free.
> >
> > Use refcount_inc_not_zero_acquire() when acquiring the reference.
> 
> This is a separate issue, with a different Fixes: tag.
> 
> When 3ab5aee7fe84 was written, sk_refcnt was an atomic_t and
> atomic_inc_not_zero() was fully ordered. Even after the conversion to
> refcount_t (41c6d650f653), default builds used atomic_add_unless().
> The ordering was only lost on weakly ordered arches in v5.5, with
> commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> of refcount_t").
> 
> Please split this into a series:
> 
> 1) Unhashed check in the lookups, and the __reqsk_free() -> reqsk_put()
>    conversions (these are needed because a stale reader can hold a
>    transient reference on a request that was never hashed).
>    Fixes: 079096f103fa ("tcp/dccp: install syn_recv requests into ehash table")
> 
> 2) The acquire/release conversions.

Understood.

> 
> If you convert writers to refcount_set_release(), please be
> consistent: sk_clone() still uses refcount_set(&newsk->sk_refcnt, 2),
> and this is precisely the full socket case you mention.
> On the other hand, inet_twsk_hashdance_schedule() already has an
> smp_wmb() right after the refcount_set(), so changing it is not
> needed.
> 
> 
> > After the reference is acquired, restart the lookup if the nulls node is
> > unhashed or the tuple no longer matches. This rejects the SYN cookie
> > request socket and blocks the child socket path observed in the x86
> > test.
> >
> > Use release ordering when setting initial nonzero refcounts in the SYN
> > cookie, TCP Fast Open, request migration, and TIME_WAIT paths. This
> > makes initialization performed before the refcount update visible to a
> > reader that acquires a reference. After a nonzero request refcount has
> > been published, release it with reqsk_put() on failure instead of
> > freeing the request directly.
> >
> > Fixes: 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use RCU / hlist_nulls")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > ---
> >  net/ipv4/inet_connection_sock.c | 14 ++++++++------
> >  net/ipv4/inet_hashtables.c      |  7 ++++---
> >  net/ipv4/inet_timewait_sock.c   |  2 +-
> >  net/ipv4/syncookies.c           |  4 +++-
> >  net/ipv4/tcp_fastopen.c         |  3 +--
> >  net/ipv4/tcp_input.c            |  3 ++-
> >  net/ipv6/inet6_hashtables.c     |  5 +++--
> >  7 files changed, 22 insertions(+), 16 deletions(-)
> >
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index 6a30f11384547e..25b46beed63541 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;
> > @@ -1136,10 +1136,12 @@ static void reqsk_timer_handler(struct timer_list *t)
> >          */
> >         if (nreq) {
> >                 __NET_INC_STATS(net, LINUX_MIB_TCPMIGRATEREQFAILURE);
> > +               reqsk_put(nreq);
> >  no_ownership:
> >                 reqsk_migrate_reset(nreq);
> >                 reqsk_queue_removed(queue, nreq);
> > -               __reqsk_free(nreq);
> > +               reqsk_put(nreq);
> > +               reqsk_put(nreq);
> 
> The accounting looks right on both paths, but three reqsk_put() split
> around a label are hard to follow. Please add a comment explaining
> which references are released on each path.
> 
> >         }
> >
> >  drop:
> > @@ -1437,7 +1439,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);
> > @@ -1447,7 +1449,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;
> >                 }
> > @@ -1490,7 +1492,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)) {
> > @@ -1502,7 +1504,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 ba0faa9ae2bb3a..7ffa9171d6de10 100644
> > --- a/net/ipv4/inet_hashtables.c
> > +++ b/net/ipv4/inet_hashtables.c
> > @@ -550,10 +550,11 @@ 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(!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/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> > index d4c781a0667fe5..88ea3a101849aa 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 73e12976818473..4b2d2713653b75 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)) {
> > @@ -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_fastopen.c b/net/ipv4/tcp_fastopen.c
> > index 471c78be55137a..2506f415f410bb 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/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 99baf14afdfd8f..131aa475e35f69 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -7777,7 +7777,8 @@ 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;
> > +                       reqsk_put(req);
> > +                       goto drop;
> 
> This leaks the request.
> 
> tcp_fastopen_create_child() set rsk_refcnt to 2, and
> inet_child_forget() cleared tp->fastopen_rsk without releasing its
> reference. A single reqsk_put() leaves the refcount at 1, leaking the
> request, its rsk_listener reference, and therefore the netns.
> 
> You need to drop both references here.

I was a little confused by this part, so I checked the path more
carefully.

req->sk is set only on the success path of
inet_csk_reqsk_queue_add(). It remains NULL on the failure path, so
tcp_disconnect() drops one reference through reqsk_fastopen_remove().

tcp_conn_request()
  inet_csk_reqsk_queue_add(sk, req, fastopen_sk) // fails
    inet_child_forget(sk, req, fastopen_sk)
      sk->sk_prot->disconnect(fastopen_sk, O_NONBLOCK)
        tcp_disconnect(fastopen_sk, O_NONBLOCK)
          reqsk_fastopen_remove(fastopen_sk, req, false)
            reqsk_put(req) // 2 -> 1 when no transient lookup reference is held

  reqsk_put(req)           // 1 -> 0 when no transient lookup reference is held

A transient reference held by a lookup is released separately in the
lookup path. The caller therefore needs only one reqsk_put(). Calling it
twice would drop one reference too many and could cause a UAF.

I therefore think the caller should keep a single reqsk_put(), as in v1,
and add a short comment noting that reqsk_fastopen_remove() has already
dropped one reference.

> 
> >                 }
> >                 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 b111b51d69fc9b..7468aca5c1f1ee 100644
> > --- a/net/ipv6/inet6_hashtables.c
> > +++ b/net/ipv6/inet6_hashtables.c
> > @@ -105,10 +105,11 @@ 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(!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
> >

Before sending v2, I would like to make sure I understood your comments
correctly. I plan to make the following changes. Does this look right?

PATCH 1:

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;
 		}

PATCH 2:

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


Best regards,
Hyunwoo Kim

Re: [PATCH net] 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: 0 · Medium: 1 (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).

- [Medium] The new reader-side check does not order later loads of socket
  fields.

Pre-existing issues:
- [Medium] When a tuple matches but refcount_inc_not_zero_acquire()
  fails, __inet_lookup_established()…

--

inet: reject unhashed sockets during established lookups

This patch changes __inet_lookup_established() and
__inet6_lookup_established() to take the reference with
refcount_inc_not_zero_acquire(). After taking it, the lookup now restarts
if the nulls node is unhashed. It also switches the initial nonzero
refcount stores in the SYN cookie, TFO, request migration and TIME_WAIT
paths to refcount_set_release(), and frees published requests with
reqsk_put().

[ ... ]

> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index ba0faa9ae2bb3..7ffa9171d6de1 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -550,10 +550,11 @@ 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 is a pre-existing issue and was not introduced by this patch. When
the tuple matches but the refcount increment fails, the lookup still
does goto out and returns NULL instead of restarting.
Documentation/RCU/rculist_nulls.rst restarts the lookup in this case.

Can this miss a live socket that is in the same chain? Here is one window.
In reqsk_timer_handler(), inet_reqsk_clone() does:

    memcpy(nreq_sk, req_sk,
           offsetof(struct sock, sk_dontcopy_begin));

This copies skc_hash and the tuple from oreq, but not skc_refcnt. Until
reqsk_timer_handler() reaches this line:

    refcount_set_release(&nreq->rsk_refcnt, 2 + 1);

nreq carries oreq's hash and tuple with a refcount of 0 left over from its
previous life. During that time oreq is still hashed in the same chain.

A reader whose stale pointer lands on nreq passes the hash and tuple
checks and fails the increment. It then returns NULL without reaching
oreq.

tcp_v4_rcv() then falls back to the listener. For an ACK,
tcp_rcv_state_process() in TCP_LISTEN returns 1, so a RST is sent for a
valid handshake. A similar window exists for a replaced req whose
refcount has already dropped to 0.

A restart has a cost of its own. reqsk_queue_hash_req() hashes a req
before it sets the refcount, so a restarting reader would have to spin
past that window. The window is kept short with preempt_disable_nested().

The same goto out exists in __inet6_lookup_established().

> -			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: Medium]
On weakly ordered CPUs such as arm64 and POWER, does this check order the
caller's later loads of socket fields?

hlist_nulls_unhashed_lockless() is only:

    return !READ_ONCE(h->pprev);

Branching on it creates a control dependency. That orders later stores,
but not later loads.

For full child sockets, the acquire in refcount_inc_not_zero_acquire()
does not seem to pair with a release either. sk_clone() publishes the
refcount with a plain store:

    smp_wmb();
    refcount_set(&newsk->sk_refcnt, 2);

After that store, inet_csk_clone_lock() writes the child's tuple, clears
icsk_accept_queue, and changes sk_state from the TCP_LISTEN value copied
from the parent to TCP_SYN_RECV. tcp_v4_syn_recv_sock() hashes the child
last.

A stale reader could then see a non-NULL pprev and the new tuple, but
still load the parent's TCP_LISTEN when tcp_v4_rcv() reads sk_state:

    if (sk->sk_state == TCP_LISTEN) {
        ret = tcp_v4_do_rcv(sk, skb);
        goto put_and_return;
    }

That path skips bh_lock_sock_nested(). The packet is processed on the
child without its lock while the creating CPU still holds that lock and
is finishing setup. The result is either a RST from listener handling or
unlocked SYN_RECV processing that races with the creator.

The commit message says:

    This rejects the SYN cookie
    request socket and blocks the child socket path observed in the x86
    test.

Does this hold only under x86 TSO? Is an smp_rmb() (or an acquire load of
pprev) needed after the hashed check, with matching ordering on the
writer side?

The same check in __inet6_lookup_established() has the same ordering
question.

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/ath58Egc-oI6p3Jl%40v4bel