* [PATCH net v4 1/2] inet: reject unhashed sockets during established lookups [not found] <20261026003804.61923-1-imv4bel@gmail.com> @ 2026-10-26 0:37 ` Hyunwoo Kim 2026-10-28 1:47 ` Kuniyuki Iwashima 2026-10-26 0:37 ` [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks Hyunwoo Kim 1 sibling, 1 reply; 6+ messages in thread From: Hyunwoo Kim @ 2026-10-26 0:37 UTC (permalink / raw) To: edumazet, ncardwell, kuniyu, davem, kuba, pabeni, horms Cc: Hyunwoo Kim, martin.lau, john.fastabend, weiwan, sd, jiayuan.chen, netdev, stable TCP established ehash lookups are lockless. The socket caches involved use SLAB_TYPESAFE_BY_RCU, so memory can be reused before an RCU reader that retained its old address has finished. After a tuple match, __inet_lookup_established() and __inet6_lookup_established() acquire a reference and repeat the tuple check. They do not check whether the new object is hashed. If the address is reused for a socket with the same tuple, the lookup can acquire a reference to the new object and return it while its nulls node is unhashed. A PoC that drives reuse with a SYN cookie request socket reproduced the crash. The new request socket is unhashed and has a NULL rsk_listener. A lookup that had loaded the address of the old request socket acquired the reference published by tcp_get_cookie_sock() and returned the new object. tcp_v4_rcv() then read sk_state through the NULL listener. The resulting crash was: BUG: kernel NULL pointer dereference, address: 0000000000000012 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page ... Oops: Oops: 0000 [#1] SMP NOPTI CPU: 4 UID: 0 PID: 0 Comm: swapper/4 Not tainted 7.3.0-00591-g0e135910deec #1 PREEMPT(lazy) ... RIP: 0010:tcp_v4_rcv+0x44e/0x1280 ... R10: 0000000000000001 R11: 00000000024ca8c0 R12: 0000000000000000 ... CR2: 0000000000000012 CR3: 0000000103d50004 CR4: 0000000000770ef0 ... Call Trace: <IRQ> ? raw_local_deliver+0xae/0x240 ip_protocol_deliver_rcu+0x2d/0x180 ip_local_deliver_finish+0x84/0x100 ip_sublist_rcv_finish+0x5f/0x70 ip_sublist_rcv+0x12a/0x1d0 ip_list_rcv+0xfd/0x130 __netif_receive_skb_list_core+0x223/0x250 netif_receive_skb_list_internal+0x1d2/0x2c0 ? try_fill_recv+0x39/0x7b0 napi_complete_done+0x74/0x170 virtnet_poll+0x445/0x600 __napi_poll+0x27/0x1e0 net_rx_action+0x2fe/0x3b0 handle_softirqs+0xc2/0x2c0 __irq_exit_rcu+0x74/0xd0 common_interrupt+0x7f/0x90 </IRQ> ... Kernel panic - not syncing: Fatal exception in interrupt After taking a reference, check whether the nulls node is unhashed in both IPv4 and IPv6 established lookups. If it is unhashed or the tuple no longer matches, drop the reference and restart the lookup. Lookups racing with the TIME_WAIT hashdance, or with a request being replaced by its child, now restart and find the new socket instead of returning the old one. Request sockets that were never inserted into ehash can have a nonzero refcount, and a stale lookup can hold a transient reference. Therefore, when SYN cookie, Fast Open, and request migration paths fail after setting a nonzero refcount, release the references held by those paths with reqsk_put() instead of freeing the request directly. Fixes: 079096f103fa ("tcp/dccp: install syn_recv requests into ehash table") Cc: stable@vger.kernel.org Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> Reviewed-by: Eric Dumazet <edumazet@kernel.org> --- net/ipv4/inet_connection_sock.c | 10 +++++++--- net/ipv4/inet_hashtables.c | 5 +++-- net/ipv4/syncookies.c | 2 ++ net/ipv4/tcp_input.c | 4 +++- net/ipv6/inet6_hashtables.c | 4 +++- 5 files changed, 18 insertions(+), 7 deletions(-) diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index 6a30f11384547..bd0bad96c93c3 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -1136,10 +1136,14 @@ static void reqsk_timer_handler(struct timer_list *t) */ if (nreq) { __NET_INC_STATS(net, LINUX_MIB_TCPMIGRATEREQFAILURE); + /* The timer was not armed, drop its reference here. */ + reqsk_put(nreq); no_ownership: reqsk_migrate_reset(nreq); reqsk_queue_removed(queue, nreq); - __reqsk_free(nreq); + /* Drop the temporary reference and the one reserved for ehash. */ + reqsk_put(nreq); + reqsk_put(nreq); } drop: @@ -1447,7 +1451,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child, __NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQFAILURE); reqsk_migrate_reset(nreq); - __reqsk_free(nreq); + reqsk_put(nreq); } else if (inet_csk_reqsk_queue_add(sk, req, child)) { return child; } @@ -1502,7 +1506,7 @@ void inet_csk_listen_stop(struct sock *sk) __NET_INC_STATS(sock_net(nsk), LINUX_MIB_TCPMIGRATEREQFAILURE); reqsk_migrate_reset(nreq); - __reqsk_free(nreq); + reqsk_put(nreq); } rcu_read_unlock(); diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c index ba0faa9ae2bb3..58d54965036a2 100644 --- a/net/ipv4/inet_hashtables.c +++ b/net/ipv4/inet_hashtables.c @@ -552,8 +552,9 @@ struct sock *__inet_lookup_established(const struct net *net, if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) { if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt))) goto out; - if (unlikely(!inet_match(net, sk, acookie, - ports, dif, sdif))) { + if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) || + !inet_match(net, sk, acookie, ports, dif, + sdif))) { sock_gen_put(sk); goto begin; } diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c index 73e1297681847..6440305615378 100644 --- a/net/ipv4/syncookies.c +++ b/net/ipv4/syncookies.c @@ -218,6 +218,8 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb, bh_unlock_sock(child); sock_put(child); + reqsk_put(req); + return NULL; } __reqsk_free(req); diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 99baf14afdfd8..3583867487d7f 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -7777,7 +7777,9 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops, if (!inet_csk_reqsk_queue_add(sk, req, fastopen_sk)) { bh_unlock_sock(fastopen_sk); sock_put(fastopen_sk); - goto drop_and_free; + /* tcp_disconnect() already dropped one reqsk ref. */ + reqsk_put(req); + goto drop; } READ_ONCE(sk->sk_data_ready)(sk); bh_unlock_sock(fastopen_sk); diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c index b111b51d69fc9..aa5fa1dd062f7 100644 --- a/net/ipv6/inet6_hashtables.c +++ b/net/ipv6/inet6_hashtables.c @@ -108,7 +108,9 @@ struct sock *__inet6_lookup_established(const struct net *net, if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt))) goto out; - if (unlikely(!inet6_match(net, sk, saddr, daddr, ports, dif, sdif))) { + if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) || + !inet6_match(net, sk, saddr, daddr, ports, dif, + sdif))) { sock_gen_put(sk); goto begin; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v4 1/2] inet: reject unhashed sockets during established lookups 2026-10-26 0:37 ` [PATCH net v4 1/2] inet: reject unhashed sockets during established lookups Hyunwoo Kim @ 2026-10-28 1:47 ` Kuniyuki Iwashima 0 siblings, 0 replies; 6+ messages in thread From: Kuniyuki Iwashima @ 2026-10-28 1:47 UTC (permalink / raw) To: Hyunwoo Kim Cc: edumazet, ncardwell, davem, kuba, pabeni, horms, martin.lau, john.fastabend, weiwan, sd, jiayuan.chen, netdev, stable On Mon, 26 Oct 2026 09:37:44 +0900, Hyunwoo Kim wrote: > 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks [not found] <20261026003804.61923-1-imv4bel@gmail.com> 2026-10-26 0:37 ` [PATCH net v4 1/2] inet: reject unhashed sockets during established lookups Hyunwoo Kim @ 2026-10-26 0:37 ` Hyunwoo Kim 2026-10-28 2:13 ` Kuniyuki Iwashima 1 sibling, 1 reply; 6+ messages in thread From: Hyunwoo Kim @ 2026-10-26 0:37 UTC (permalink / raw) To: edumazet, ncardwell, kuniyu, davem, kuba, pabeni, horms Cc: Hyunwoo Kim, martin.lau, john.fastabend, weiwan, sd, jiayuan.chen, netdev, stable Before commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t"), default builds implemented refcount_inc_not_zero() using atomic_add_unless(), which provided full memory ordering on success. The commit removed that ordering on weakly ordered architectures, allowing validation after a successful reference acquisition in lockless ehash walks 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 network namespace is validated again after taking a reference. Use refcount_set_release() when setting refcounts in the SYN cookie, TCP Fast Open, request migration, and TIME_WAIT paths. Keep the existing TIME_WAIT smp_wmb(), which separately orders the refcount store before ehash publication. Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t") Cc: stable@vger.kernel.org Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> --- net/ipv4/inet_connection_sock.c | 6 +++--- net/ipv4/inet_hashtables.c | 2 +- net/ipv4/inet_timewait_sock.c | 4 ++-- net/ipv4/syncookies.c | 2 +- net/ipv4/tcp_fastopen.c | 2 +- net/ipv6/inet6_hashtables.c | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index bd0bad96c93c3..837cb91e5b173 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -1056,8 +1056,8 @@ static void reqsk_timer_handler(struct timer_list *t) * hold another count to prevent use-after-free and * call reqsk_put() just before return. */ - refcount_set(&nreq->rsk_refcnt, 2 + 1); timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED); + refcount_set_release(&nreq->rsk_refcnt, 2 + 1); reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req); req = nreq; @@ -1441,7 +1441,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child, goto child_put; } - refcount_set(&nreq->rsk_refcnt, 1); + refcount_set_release(&nreq->rsk_refcnt, 1); if (inet_csk_reqsk_queue_add(sk, nreq, child)) { __NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQSUCCESS); reqsk_migrate_reset(req); @@ -1494,7 +1494,7 @@ void inet_csk_listen_stop(struct sock *sk) if (nsk) { nreq = inet_reqsk_clone(req, nsk); if (nreq) { - refcount_set(&nreq->rsk_refcnt, 1); + refcount_set_release(&nreq->rsk_refcnt, 1); rcu_read_lock(); if (inet_csk_reqsk_queue_add(nsk, nreq, child)) { diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c index 58d54965036a2..7ffa9171d6de1 100644 --- a/net/ipv4/inet_hashtables.c +++ b/net/ipv4/inet_hashtables.c @@ -550,7 +550,7 @@ struct sock *__inet_lookup_established(const struct net *net, if (sk->sk_hash != hash) continue; if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) { - if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt))) + if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt))) goto out; if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) || !inet_match(net, sk, acookie, ports, dif, diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c index d4c781a0667fe..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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks 2026-10-26 0:37 ` [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks Hyunwoo Kim @ 2026-10-28 2:13 ` Kuniyuki Iwashima 2026-10-28 3:29 ` Hyunwoo Kim 0 siblings, 1 reply; 6+ messages in thread From: Kuniyuki Iwashima @ 2026-10-28 2:13 UTC (permalink / raw) To: Hyunwoo Kim Cc: edumazet, ncardwell, davem, kuba, pabeni, horms, martin.lau, john.fastabend, weiwan, sd, jiayuan.chen, netdev, stable On Mon, 26 Oct 2026 09:37:45 +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 > validation after a successful reference acquisition in lockless ehash > walks to be reordered before it. > > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established Does bpf_sk_assign() need it too ? If the socket is taken from lookup helper, it's safe, but what happens if the pointer is taken from BTF (27e5203bd9c5), e.g. tp->inet_conn.icsk_accept_queue.rskq_accept_head ? > lookups and in inet_twsk_purge(), where the network namespace is validated > again after taking a reference. Use refcount_set_release() when setting > refcounts in the SYN cookie, TCP Fast Open, request migration, and > TIME_WAIT paths. Keep the existing TIME_WAIT smp_wmb(), which separately > orders the refcount store before ehash publication. > > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t") > Cc: stable@vger.kernel.org > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> > --- > net/ipv4/inet_connection_sock.c | 6 +++--- > net/ipv4/inet_hashtables.c | 2 +- > net/ipv4/inet_timewait_sock.c | 4 ++-- > net/ipv4/syncookies.c | 2 +- > net/ipv4/tcp_fastopen.c | 2 +- > net/ipv6/inet6_hashtables.c | 2 +- > 6 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > index bd0bad96c93c3..837cb91e5b173 100644 > --- a/net/ipv4/inet_connection_sock.c > +++ b/net/ipv4/inet_connection_sock.c > @@ -1056,8 +1056,8 @@ static void reqsk_timer_handler(struct timer_list *t) > * hold another count to prevent use-after-free and > * call reqsk_put() just before return. > */ > - refcount_set(&nreq->rsk_refcnt, 2 + 1); > timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED); > + refcount_set_release(&nreq->rsk_refcnt, 2 + 1); > reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req); > > req = nreq; > @@ -1441,7 +1441,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child, > goto child_put; > } > > - refcount_set(&nreq->rsk_refcnt, 1); > + refcount_set_release(&nreq->rsk_refcnt, 1); > if (inet_csk_reqsk_queue_add(sk, nreq, child)) { > __NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQSUCCESS); > reqsk_migrate_reset(req); > @@ -1494,7 +1494,7 @@ void inet_csk_listen_stop(struct sock *sk) > if (nsk) { > nreq = inet_reqsk_clone(req, nsk); > if (nreq) { > - refcount_set(&nreq->rsk_refcnt, 1); > + refcount_set_release(&nreq->rsk_refcnt, 1); > > rcu_read_lock(); > if (inet_csk_reqsk_queue_add(nsk, nreq, child)) { > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c > index 58d54965036a2..7ffa9171d6de1 100644 > --- a/net/ipv4/inet_hashtables.c > +++ b/net/ipv4/inet_hashtables.c > @@ -550,7 +550,7 @@ struct sock *__inet_lookup_established(const struct net *net, > if (sk->sk_hash != hash) > continue; > if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) { > - if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt))) > + if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt))) > goto out; > if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) || > !inet_match(net, sk, acookie, ports, dif, > diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c > index d4c781a0667fe..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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks 2026-10-28 2:13 ` Kuniyuki Iwashima @ 2026-10-28 3:29 ` Hyunwoo Kim 2026-10-28 20:58 ` Kuniyuki Iwashima 0 siblings, 1 reply; 6+ messages in thread From: Hyunwoo Kim @ 2026-10-28 3:29 UTC (permalink / raw) To: Kuniyuki Iwashima Cc: edumazet, ncardwell, davem, kuba, pabeni, horms, martin.lau, john.fastabend, weiwan, sd, jiayuan.chen, netdev, stable, imv4bel On Wed, Oct 28, 2026 at 02:13:15AM +0000, Kuniyuki Iwashima wrote: > On Mon, 26 Oct 2026 09:37:45 +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 > > validation after a successful reference acquisition in lockless ehash > > walks to be reordered before it. > > > > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established > > Does bpf_sk_assign() need it too ? > > If the socket is taken from lookup helper, it's safe, but what > happens if the pointer is taken from BTF (27e5203bd9c5), e.g. > tp->inet_conn.icsk_accept_queue.rskq_accept_head ? Yeah the same ordering issue remains when the pointer is obtained from BTF without a reference. Fixes: 27e5203bd9c5. However, I think this is better handled in a separate follow-up patch. What do you think? > > > > lookups and in inet_twsk_purge(), where the network namespace is validated > > again after taking a reference. Use refcount_set_release() when setting > > refcounts in the SYN cookie, TCP Fast Open, request migration, and > > TIME_WAIT paths. Keep the existing TIME_WAIT smp_wmb(), which separately > > orders the refcount store before ehash publication. > > > > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t") > > Cc: stable@vger.kernel.org > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> > > --- > > net/ipv4/inet_connection_sock.c | 6 +++--- > > net/ipv4/inet_hashtables.c | 2 +- > > net/ipv4/inet_timewait_sock.c | 4 ++-- > > net/ipv4/syncookies.c | 2 +- > > net/ipv4/tcp_fastopen.c | 2 +- > > net/ipv6/inet6_hashtables.c | 2 +- > > 6 files changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > > index bd0bad96c93c3..837cb91e5b173 100644 > > --- a/net/ipv4/inet_connection_sock.c > > +++ b/net/ipv4/inet_connection_sock.c > > @@ -1056,8 +1056,8 @@ static void reqsk_timer_handler(struct timer_list *t) > > * hold another count to prevent use-after-free and > > * call reqsk_put() just before return. > > */ > > - refcount_set(&nreq->rsk_refcnt, 2 + 1); > > timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED); > > + refcount_set_release(&nreq->rsk_refcnt, 2 + 1); > > reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req); > > > > req = nreq; > > @@ -1441,7 +1441,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child, > > goto child_put; > > } > > > > - refcount_set(&nreq->rsk_refcnt, 1); > > + refcount_set_release(&nreq->rsk_refcnt, 1); > > if (inet_csk_reqsk_queue_add(sk, nreq, child)) { > > __NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQSUCCESS); > > reqsk_migrate_reset(req); > > @@ -1494,7 +1494,7 @@ void inet_csk_listen_stop(struct sock *sk) > > if (nsk) { > > nreq = inet_reqsk_clone(req, nsk); > > if (nreq) { > > - refcount_set(&nreq->rsk_refcnt, 1); > > + refcount_set_release(&nreq->rsk_refcnt, 1); > > > > rcu_read_lock(); > > if (inet_csk_reqsk_queue_add(nsk, nreq, child)) { > > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c > > index 58d54965036a2..7ffa9171d6de1 100644 > > --- a/net/ipv4/inet_hashtables.c > > +++ b/net/ipv4/inet_hashtables.c > > @@ -550,7 +550,7 @@ struct sock *__inet_lookup_established(const struct net *net, > > if (sk->sk_hash != hash) > > continue; > > if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) { > > - if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt))) > > + if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt))) > > goto out; > > if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) || > > !inet_match(net, sk, acookie, ports, dif, > > diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c > > index d4c781a0667fe..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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks 2026-10-28 3:29 ` Hyunwoo Kim @ 2026-10-28 20:58 ` Kuniyuki Iwashima 0 siblings, 0 replies; 6+ messages in thread From: Kuniyuki Iwashima @ 2026-10-28 20:58 UTC (permalink / raw) To: Hyunwoo Kim Cc: edumazet, ncardwell, davem, kuba, pabeni, horms, martin.lau, john.fastabend, weiwan, sd, jiayuan.chen, netdev, stable On Wed, 28 Oct 2026 12:29:10 +0900, Hyunwoo Kim wrote: > On Wed, Oct 28, 2026 at 02:13:15AM +0000, Kuniyuki Iwashima wrote: > > On Mon, 26 Oct 2026 09:37:45 +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 > > > validation after a successful reference acquisition in lockless ehash > > > walks to be reordered before it. > > > > > > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established > > > > Does bpf_sk_assign() need it too ? > > > > If the socket is taken from lookup helper, it's safe, but what > > happens if the pointer is taken from BTF (27e5203bd9c5), e.g. > > tp->inet_conn.icsk_accept_queue.rskq_accept_head ? > > Yeah the same ordering issue remains when the pointer is obtained from > BTF without a reference. Fixes: 27e5203bd9c5. > > However, I think this is better handled in a separate follow-up patch. > What do you think? I think it can be just squashed given 27e5203bd9c5 exist in the oldest LTS 5.10. > > > > > > > > lookups and in inet_twsk_purge(), where the network namespace is validated > > > again after taking a reference. Use refcount_set_release() when setting > > > refcounts in the SYN cookie, TCP Fast Open, request migration, and > > > TIME_WAIT paths. Keep the existing TIME_WAIT smp_wmb(), which separately > > > orders the refcount store before ehash publication. > > > > > > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> > > > --- > > > net/ipv4/inet_connection_sock.c | 6 +++--- > > > net/ipv4/inet_hashtables.c | 2 +- > > > net/ipv4/inet_timewait_sock.c | 4 ++-- > > > net/ipv4/syncookies.c | 2 +- > > > net/ipv4/tcp_fastopen.c | 2 +- > > > net/ipv6/inet6_hashtables.c | 2 +- > > > 6 files changed, 9 insertions(+), 9 deletions(-) > > > > > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > > > index bd0bad96c93c3..837cb91e5b173 100644 > > > --- a/net/ipv4/inet_connection_sock.c > > > +++ b/net/ipv4/inet_connection_sock.c > > > @@ -1056,8 +1056,8 @@ static void reqsk_timer_handler(struct timer_list *t) > > > * hold another count to prevent use-after-free and > > > * call reqsk_put() just before return. > > > */ > > > - refcount_set(&nreq->rsk_refcnt, 2 + 1); > > > timer_setup(&nreq->rsk_timer, reqsk_timer_handler, TIMER_PINNED); > > > + refcount_set_release(&nreq->rsk_refcnt, 2 + 1); > > > reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req); > > > > > > req = nreq; > > > @@ -1441,7 +1441,7 @@ struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child, > > > goto child_put; > > > } > > > > > > - refcount_set(&nreq->rsk_refcnt, 1); > > > + refcount_set_release(&nreq->rsk_refcnt, 1); > > > if (inet_csk_reqsk_queue_add(sk, nreq, child)) { > > > __NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQSUCCESS); > > > reqsk_migrate_reset(req); > > > @@ -1494,7 +1494,7 @@ void inet_csk_listen_stop(struct sock *sk) > > > if (nsk) { > > > nreq = inet_reqsk_clone(req, nsk); > > > if (nreq) { > > > - refcount_set(&nreq->rsk_refcnt, 1); > > > + refcount_set_release(&nreq->rsk_refcnt, 1); > > > > > > rcu_read_lock(); > > > if (inet_csk_reqsk_queue_add(nsk, nreq, child)) { > > > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c > > > index 58d54965036a2..7ffa9171d6de1 100644 > > > --- a/net/ipv4/inet_hashtables.c > > > +++ b/net/ipv4/inet_hashtables.c > > > @@ -550,7 +550,7 @@ struct sock *__inet_lookup_established(const struct net *net, > > > if (sk->sk_hash != hash) > > > continue; > > > if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) { > > > - if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt))) > > > + if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt))) > > > goto out; > > > if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) || > > > !inet_match(net, sk, acookie, ports, dif, > > > diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c > > > index d4c781a0667fe..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 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-10-28 20:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20261026003804.61923-1-imv4bel@gmail.com>
2026-10-26 0:37 ` [PATCH net v4 1/2] inet: reject unhashed sockets during established lookups Hyunwoo Kim
2026-10-28 1:47 ` Kuniyuki Iwashima
2026-10-26 0:37 ` [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks Hyunwoo Kim
2026-10-28 2:13 ` Kuniyuki Iwashima
2026-10-28 3:29 ` Hyunwoo Kim
2026-10-28 20:58 ` Kuniyuki Iwashima
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox