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

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

[Hyunwoo Kim]

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

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

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

The resulting crash was:

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

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

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

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

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

Re: [PATCH net v5 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] In this commit by itself, the new
  `hlist_nulls_unhashed_lockless(&sk->sk_nulls_node)` check in…

--

inet: reject unhashed sockets during established lookups

This commit adds a 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 can no longer return a request socket that was never hashed. It also
changes the SYN cookie, Fast Open and request migration failure paths to
drop references with reqsk_put() instead of calling __reqsk_free().

> 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]
If this commit is applied by itself, is the new unhashed check ordered
after the refcount increment?

refcount_inc_not_zero() here is a relaxed cmpxchg. The writers that make
never-hashed request sockets referenceable also use a plain refcount_set():

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

The same applies to tcp_fastopen_create_child(), and to the migration
clones in reqsk_timer_handler(), inet_csk_complete_hashdance() and
inet_csk_listen_stop().

On weakly ordered CPUs, can the pprev load in
hlist_nulls_unhashed_lockless() and the inet_match() reload be satisfied
before the increment? They could then see the old, hashed incarnation,
while the increment succeeds on the new, unhashed SYN cookie request whose
rsk_listener is NULL. That is the tcp_v4_rcv() NULL dereference this
commit describes. __inet6_lookup_established() has the same pattern.

The next patch in the series, "net: restore refcount ordering for lockless
socket access", resolves this. It switches both lookups to
refcount_inc_not_zero_acquire(), and switches tcp_get_cookie_sock(),
tcp_fastopen_create_child() and the three migration writers to
refcount_set_release().

Both patches are tagged Cc: stable. Should the two be backported together,
so stable trees don't pick up this one without the ordering fix?

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

[ ... ]

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

[PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Hyunwoo Kim]

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

Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
lookups and in inet_twsk_purge(), where the socket is validated again
after taking a reference.

bpf_sk_assign() can receive a socket pointer obtained through BTF
without a reference. Use acquire ordering when it takes its own reference
for a refcounted socket.

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

Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
Fixes: 27e5203bd9c5 ("bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/core/filter.c               | 2 +-
 net/ipv4/inet_connection_sock.c | 6 +++---
 net/ipv4/inet_hashtables.c      | 2 +-
 net/ipv4/inet_timewait_sock.c   | 4 ++--
 net/ipv4/syncookies.c           | 2 +-
 net/ipv4/tcp_fastopen.c         | 2 +-
 net/ipv6/inet6_hashtables.c     | 2 +-
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 70dc621672f2e..6d87194c377df 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7989,7 +7989,7 @@ BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
 	if (sk_unhashed(sk))
 		return -EOPNOTSUPP;
 	if (sk_is_refcounted(sk) &&
-	    unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+	    unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
 		return -ENOENT;
 
 	skb_orphan(skb);
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index bd0bad96c93c3..837cb91e5b173 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -1056,8 +1056,8 @@ static void reqsk_timer_handler(struct timer_list *t)
 		 * hold another count to prevent use-after-free and
 		 * call reqsk_put() just before return.
 		 */
-		refcount_set(&nreq->rsk_refcnt, 2 + 1);
 		timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED);
+		refcount_set_release(&nreq->rsk_refcnt, 2 + 1);
 		reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req);
 
 		req = nreq;
@@ -1441,7 +1441,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child,
 				goto child_put;
 			}
 
-			refcount_set(&nreq->rsk_refcnt, 1);
+			refcount_set_release(&nreq->rsk_refcnt, 1);
 			if (inet_csk_reqsk_queue_add(sk, nreq, child)) {
 				__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQSUCCESS);
 				reqsk_migrate_reset(req);
@@ -1494,7 +1494,7 @@ void inet_csk_listen_stop(struct sock *sk)
 		if (nsk) {
 			nreq = inet_reqsk_clone(req, nsk);
 			if (nreq) {
-				refcount_set(&nreq->rsk_refcnt, 1);
+				refcount_set_release(&nreq->rsk_refcnt, 1);
 
 				rcu_read_lock();
 				if (inet_csk_reqsk_queue_add(nsk, nreq, child)) {
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 58d54965036a2..7ffa9171d6de1 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -550,7 +550,7 @@ struct sock *__inet_lookup_established(const struct net *net,
 		if (sk->sk_hash != hash)
 			continue;
 		if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) {
-			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+			if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
 				goto out;
 			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
 				     !inet_match(net, sk, acookie, ports, dif,
diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
index d4c781a0667fe..74ff40d42e56b 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
@@ -328,7 +328,7 @@ void inet_twsk_purge(struct inet_hashinfo *hashinfo)
 			if (check_net(sock_net(sk)))
 				continue;
 
-			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+			if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
 				continue;
 
 			if (check_net(sock_net(sk))) {
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 6440305615378..4b2d2713653b7 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -205,7 +205,7 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
 	child = icsk->icsk_af_ops->syn_recv_sock(sk, skb, req, dst,
 						 NULL, &own_req, NULL);
 	if (child) {
-		refcount_set(&req->rsk_refcnt, 1);
+		refcount_set_release(&req->rsk_refcnt, 1);
 		sock_rps_save_rxhash(child, skb);
 
 		if (rsk_drop_req(req)) {
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 471c78be55137..e40477b6b5549 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -364,7 +364,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
 	tcp_reset_xmit_timer(child, ICSK_TIME_RETRANS,
 			     req->timeout, false);
 
-	refcount_set(&req->rsk_refcnt, 2);
+	refcount_set_release(&req->rsk_refcnt, 2);
 
 	sk_mark_napi_id_set(child, skb);
 
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index aa5fa1dd062f7..1eea584d80df8 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -105,7 +105,7 @@ struct sock *__inet6_lookup_established(const struct net *net,
 			continue;
 		if (!inet6_match(net, sk, saddr, daddr, ports, dif, sdif))
 			continue;
-		if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+		if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
 			goto out;
 
 		if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
-- 
2.43.0

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Kuniyuki Iwashima]

On Fri, 30 Oct 2026 06:42:17 +0900, Hyunwoo Kim wrote:
> Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> of refcount_t"), default builds implemented refcount_inc_not_zero() using
> atomic_add_unless(), which provided full memory ordering on success. The
> commit removed that ordering on weakly ordered architectures, allowing
> accesses after a successful reference acquisition to be reordered before
> it.
> 
> Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> lookups and in inet_twsk_purge(), where the socket is validated again
> after taking a reference.
> 
> bpf_sk_assign() can receive a socket pointer obtained through BTF
> without a reference. Use acquire ordering when it takes its own reference
> for a refcounted socket.
> 
> Use refcount_set_release() when setting refcounts in the SYN cookie,
> TCP Fast Open, request migration, and TIME_WAIT paths. Keep the existing
> TIME_WAIT smp_wmb(), which separately orders the refcount store before
> ehash publication.
> 
> Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> Fixes: 27e5203bd9c5 ("bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Thanks !

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[netdev-bot+sashiko]

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

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

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

- [High] The bpf_sk_assign() change in net/core/filter.c does not fix the
  problem that its Fixes: 27e5203bd9c5 tag points at.
- [High] The patch is titled "restore refcount ordering for lockless
  socket access" and converts lockless lookups "where the socket is…

--

net: restore refcount ordering for lockless socket access

This patch switches the IPv4/IPv6 established lookups, inet_twsk_purge()
and bpf_sk_assign() to refcount_inc_not_zero_acquire(). It also uses
refcount_set_release() when setting refcounts in the SYN cookie, TCP Fast
Open, request migration and TIME_WAIT paths.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 09c60a58c82ab..d832d3332c6f9 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8022,7 +8022,7 @@ BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
>  	if (sk_unhashed(sk))
>  		return -EOPNOTSUPP;
>  	if (sk_is_refcounted(sk) &&
> -	    unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> +	    unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
>  		return -ENOENT;
>  
>  	skb_orphan(skb);

[Severity: High]
Does acquire ordering here fix the unreferenced BTF pointer case that the
Fixes: 27e5203bd9c5 tag refers to?

Acquire only orders the accesses that come after the increment. In
bpf_sk_assign(), every check on the socket runs before it:

    if (unlikely(dev_net(skb->dev) != sock_net(sk)))
        return -ENETUNREACH;
    if (sk_unhashed(sk))
        return -EOPNOTSUPP;
    if (sk_is_refcounted(sk) &&
        unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
        return -ENOENT;

    skb_orphan(skb);
    skb->sk = sk;
    skb->destructor = sock_pfree;

After the increment there are only stores. Nothing is checked again.

The verifier's btf_id_sock_common_types accepts plain, untrusted
PTR_TO_BTF_ID. tcp_prot and tcpv6_prot use SLAB_TYPESAFE_BY_RCU, and
sk_destruct() frees sockets without SOCK_RCU_FREE right away through
__sk_destruct().

Given that, can this sequence happen?

  - socket A passes the netns, hashed and sk_is_refcounted() checks
  - A is freed and its slot is reused as socket B (maybe in another
    netns, or not hashed yet), and B's refcount is set
  - refcount_inc_not_zero_acquire() succeeds on B
  - B is attached to the skb without being checked

inet_steal_sock()/skb_steal_sock() then deliver to the prefetched socket
without checking the netns. That would get around the -ENETUNREACH check.

sock_pfree() and skb_steal_sock() also call sk_is_refcounted() again
later. If the reused object's result differs, could the reference taken
here leak?

This race doesn't depend on memory ordering, and it existed even when the
increment was fully ordered. The established lookups changed by this
patch re-check with inet_match()/inet6_match() and
hlist_nulls_unhashed_lockless() after the increment, then call
sock_gen_put() and retry. bpf_sk_assign() has no such step.

Would it work better to redo the netns, hashed and refcounted checks after
taking the reference and call sock_gen_put() on a mismatch? Rejecting
unreferenced pointers here would be another option.

> 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;
>  			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
>  				     !inet_match(net, sk, acookie, ports, dif,

[Severity: High]
The commit message says the change targets lookups "where the socket is
validated again after taking a reference". Should the MPTCP token lookups
be converted too? They follow the same pattern but still use plain
refcount_inc_not_zero():

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)) {
			sock_put(sk);
			goto again;
		}
    ...
}

mptcp_token_iter_next() does the same thing: it increments and then checks
net_eq() again.

mptcp_prot uses SLAB_TYPESAFE_BY_RCU, and mptcp_sk_clone_init() clears
SOCK_RCU_FREE on established msks:

    sock_reset_flag(nsk, SOCK_RCU_FREE);

So an msk can be freed and its slot reused while an RCU reader still holds
the address.

On weakly ordered CPUs, can the token and sk_net loads in the re-check run
before the increment? If so, they could see the old object's values, and
the lookup would return a referenced msk for a different connection or
netns.

Callers include the userspace PM netlink operations that look up by
token, MP_JOIN handling in subflow.c and syncookies.c, and the diag and PM
iterators. HMAC validation would probably reject most of the MP_JOIN
cases.

mptcp_subflow_reqsk_clone() already uses refcount_inc_not_zero_acquire()
on the same msk objects.

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

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Hyunwoo Kim]

On Mon, Nov 02, 2026 at 10:06:21PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The bpf_sk_assign() change in net/core/filter.c does not fix the
>   problem that its Fixes: 27e5203bd9c5 tag points at.
> - [High] The patch is titled "restore refcount ordering for lockless
>   socket access" and converts lockless lookups "where the socket is…
> 
> --
> 
> net: restore refcount ordering for lockless socket access
> 
> This patch switches the IPv4/IPv6 established lookups, inet_twsk_purge()
> and bpf_sk_assign() to refcount_inc_not_zero_acquire(). It also uses
> refcount_set_release() when setting refcounts in the SYN cookie, TCP Fast
> Open, request migration and TIME_WAIT paths.
> 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 09c60a58c82ab..d832d3332c6f9 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -8022,7 +8022,7 @@ BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
> >  	if (sk_unhashed(sk))
> >  		return -EOPNOTSUPP;
> >  	if (sk_is_refcounted(sk) &&
> > -	    unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> > +	    unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
> >  		return -ENOENT;
> >  
> >  	skb_orphan(skb);
> 
> [Severity: High]
> Does acquire ordering here fix the unreferenced BTF pointer case that the
> Fixes: 27e5203bd9c5 tag refers to?
> 
> Acquire only orders the accesses that come after the increment. In
> bpf_sk_assign(), every check on the socket runs before it:
> 
>     if (unlikely(dev_net(skb->dev) != sock_net(sk)))
>         return -ENETUNREACH;
>     if (sk_unhashed(sk))
>         return -EOPNOTSUPP;
>     if (sk_is_refcounted(sk) &&
>         unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
>         return -ENOENT;
> 
>     skb_orphan(skb);
>     skb->sk = sk;
>     skb->destructor = sock_pfree;
> 
> After the increment there are only stores. Nothing is checked again.
> 
> The verifier's btf_id_sock_common_types accepts plain, untrusted
> PTR_TO_BTF_ID. tcp_prot and tcpv6_prot use SLAB_TYPESAFE_BY_RCU, and
> sk_destruct() frees sockets without SOCK_RCU_FREE right away through
> __sk_destruct().
> 
> Given that, can this sequence happen?
> 
>   - socket A passes the netns, hashed and sk_is_refcounted() checks
>   - A is freed and its slot is reused as socket B (maybe in another
>     netns, or not hashed yet), and B's refcount is set
>   - refcount_inc_not_zero_acquire() succeeds on B
>   - B is attached to the skb without being checked
> 
> inet_steal_sock()/skb_steal_sock() then deliver to the prefetched socket
> without checking the netns. That would get around the -ENETUNREACH check.
> 
> sock_pfree() and skb_steal_sock() also call sk_is_refcounted() again
> later. If the reused object's result differs, could the reference taken
> here leak?
> 
> This race doesn't depend on memory ordering, and it existed even when the
> increment was fully ordered. The established lookups changed by this
> patch re-check with inet_match()/inet6_match() and
> hlist_nulls_unhashed_lockless() after the increment, then call
> sock_gen_put() and retry. bpf_sk_assign() has no such step.
> 
> Would it work better to redo the netns, hashed and refcounted checks after
> taking the reference and call sock_gen_put() on a mismatch? Rejecting
> unreferenced pointers here would be another option.

This is beyond the ordering fix and would complicate the patch. I'll 
address it in a follow-up patch.

> 
> > 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;
> >  			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
> >  				     !inet_match(net, sk, acookie, ports, dif,
> 
> [Severity: High]
> The commit message says the change targets lookups "where the socket is
> validated again after taking a reference". Should the MPTCP token lookups
> be converted too? They follow the same pattern but still use plain
> refcount_inc_not_zero():
> 
> 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)) {
> 			sock_put(sk);
> 			goto again;
> 		}
>     ...
> }
> 
> mptcp_token_iter_next() does the same thing: it increments and then checks
> net_eq() again.
> 
> mptcp_prot uses SLAB_TYPESAFE_BY_RCU, and mptcp_sk_clone_init() clears
> SOCK_RCU_FREE on established msks:
> 
>     sock_reset_flag(nsk, SOCK_RCU_FREE);
> 
> So an msk can be freed and its slot reused while an RCU reader still holds
> the address.
> 
> On weakly ordered CPUs, can the token and sk_net loads in the re-check run
> before the increment? If so, they could see the old object's values, and
> the lookup would return a referenced msk for a different connection or
> netns.
> 
> Callers include the userspace PM netlink operations that look up by
> token, MP_JOIN handling in subflow.c and syncookies.c, and the diag and PM
> iterators. HMAC validation would probably reject most of the MP_JOIN
> cases.
> 
> mptcp_subflow_reqsk_clone() already uses refcount_inc_not_zero_acquire()
> on the same msk objects.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029214219.1035176-1-imv4bel%40gmail.com

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Paolo Abeni]

On 11/2/26 23:06, netdev-bot+sashiko@kernel.org wrote:
>> 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;
>>   			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
>>   				     !inet_match(net, sk, acookie, ports, dif,
> 
> [Severity: High]
> The commit message says the change targets lookups "where the socket is
> validated again after taking a reference". Should the MPTCP token lookups
> be converted too? They follow the same pattern but still use plain
> refcount_inc_not_zero():
> 
> 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)) {
> 			sock_put(sk);
> 			goto again;
> 		}
>      ...
> }
> 
> mptcp_token_iter_next() does the same thing: it increments and then checks
> net_eq() again.
> 
> mptcp_prot uses SLAB_TYPESAFE_BY_RCU, and mptcp_sk_clone_init() clears
> SOCK_RCU_FREE on established msks:
> 
>      sock_reset_flag(nsk, SOCK_RCU_FREE);
> 
> So an msk can be freed and its slot reused while an RCU reader still holds
> the address.
> 
> On weakly ordered CPUs, can the token and sk_net loads in the re-check run
> before the increment? If so, they could see the old object's values, and
> the lookup would return a referenced msk for a different connection or
> netns.
> 
> Callers include the userspace PM netlink operations that look up by
> token, MP_JOIN handling in subflow.c and syncookies.c, and the diag and PM
> iterators. HMAC validation would probably reject most of the MP_JOIN
> cases.
> 
> mptcp_subflow_reqsk_clone() already uses refcount_inc_not_zero_acquire()
> on the same msk objects.
Even this part is better handled with a separate patch.

/P

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Eric Dumazet]

Le jeu. 29 oct. 2026 à 22:42, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
>
> Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> of refcount_t"), default builds implemented refcount_inc_not_zero() using
> atomic_add_unless(), which provided full memory ordering on success. The
> commit removed that ordering on weakly ordered architectures, allowing
> accesses after a successful reference acquisition to be reordered before
> it.
>
> Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> lookups and in inet_twsk_purge(), where the socket is validated again
> after taking a reference.
>
> bpf_sk_assign() can receive a socket pointer obtained through BTF
> without a reference. Use acquire ordering when it takes its own reference
> for a refcounted socket.
>
> Use refcount_set_release() when setting refcounts in the SYN cookie,
> TCP Fast Open, request migration, and TIME_WAIT paths. Keep the existing
> TIME_WAIT smp_wmb(), which separately orders the refcount store before
> ehash publication.
>
> Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> Fixes: 27e5203bd9c5 ("bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---

Lack of links to prior versions is rather annoying.

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Hyunwoo Kim]

On Tue, Nov 03, 2026 at 03:49:40PM +0100, Eric Dumazet wrote:
> Le jeu. 29 oct. 2026 à 22:42, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
> >
> > Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> > of refcount_t"), default builds implemented refcount_inc_not_zero() using
> > atomic_add_unless(), which provided full memory ordering on success. The
> > commit removed that ordering on weakly ordered architectures, allowing
> > accesses after a successful reference acquisition to be reordered before
> > it.
> >
> > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> > lookups and in inet_twsk_purge(), where the socket is validated again
> > after taking a reference.
> >
> > bpf_sk_assign() can receive a socket pointer obtained through BTF
> > without a reference. Use acquire ordering when it takes its own reference
> > for a refcounted socket.
> >
> > Use refcount_set_release() when setting refcounts in the SYN cookie,
> > TCP Fast Open, request migration, and TIME_WAIT paths. Keep the existing
> > TIME_WAIT smp_wmb(), which separately orders the refcount store before
> > ehash publication.
> >
> > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> > Fixes: 27e5203bd9c5 ("bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > ---
> 
> Lack of links to prior versions is rather annoying.

Is the cover letter alone insufficient?

Should the changelog be in each patch?

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Paolo Abeni]

On 11/4/26 02:24, Hyunwoo Kim wrote:
> On Tue, Nov 03, 2026 at 03:49:40PM +0100, Eric Dumazet wrote:
>> Le jeu. 29 oct. 2026 à 22:42, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
>>>
>>> Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
>>> of refcount_t"), default builds implemented refcount_inc_not_zero() using
>>> atomic_add_unless(), which provided full memory ordering on success. The
>>> commit removed that ordering on weakly ordered architectures, allowing
>>> accesses after a successful reference acquisition to be reordered before
>>> it.
>>>
>>> Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
>>> lookups and in inet_twsk_purge(), where the socket is validated again
>>> after taking a reference.
>>>
>>> bpf_sk_assign() can receive a socket pointer obtained through BTF
>>> without a reference. Use acquire ordering when it takes its own reference
>>> for a refcounted socket.
>>>
>>> Use refcount_set_release() when setting refcounts in the SYN cookie,
>>> TCP Fast Open, request migration, and TIME_WAIT paths. Keep the existing
>>> TIME_WAIT smp_wmb(), which separately orders the refcount store before
>>> ehash publication.
>>>
>>> Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
>>> Fixes: 27e5203bd9c5 ("bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
>>> ---
>>
>> Lack of links to prior versions is rather annoying.
> 
> Is the cover letter alone insufficient?
> 
> Should the changelog be in each patch?

Ideally the cover letter should include a very quick/short overview of the
changes, and a per patch changelog should include a more detailed list
(no entry added when the patch is unmodified).

Thanks,

/P

Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Hyunwoo Kim]

On Wed, Nov 04, 2026 at 08:59:50AM +0100, Paolo Abeni wrote:
> On 11/4/26 02:24, Hyunwoo Kim wrote:
> > On Tue, Nov 03, 2026 at 03:49:40PM +0100, Eric Dumazet wrote:
> > > Le jeu. 29 oct. 2026 à 22:42, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
> > > > 
> > > > Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> > > > of refcount_t"), default builds implemented refcount_inc_not_zero() using
> > > > atomic_add_unless(), which provided full memory ordering on success. The
> > > > commit removed that ordering on weakly ordered architectures, allowing
> > > > accesses after a successful reference acquisition to be reordered before
> > > > it.
> > > > 
> > > > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> > > > lookups and in inet_twsk_purge(), where the socket is validated again
> > > > after taking a reference.
> > > > 
> > > > bpf_sk_assign() can receive a socket pointer obtained through BTF
> > > > without a reference. Use acquire ordering when it takes its own reference
> > > > for a refcounted socket.
> > > > 
> > > > Use refcount_set_release() when setting refcounts in the SYN cookie,
> > > > TCP Fast Open, request migration, and TIME_WAIT paths. Keep the existing
> > > > TIME_WAIT smp_wmb(), which separately orders the refcount store before
> > > > ehash publication.
> > > > 
> > > > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> > > > Fixes: 27e5203bd9c5 ("bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > > > ---
> > > 
> > > Lack of links to prior versions is rather annoying.
> > 
> > Is the cover letter alone insufficient?
> > 
> > Should the changelog be in each patch?
> 
> Ideally the cover letter should include a very quick/short overview of the
> changes, and a per patch changelog should include a more detailed list
> (no entry added when the patch is unmodified).

Understood. I'll do that from now on.


Best regards,
Hyunwoo Kim