From: Kuniyuki Iwashima <kuniyu@google.com>
To: Hyunwoo Kim <imv4bel@gmail.com>
Cc: edumazet@kernel.org, ncardwell@google.com, davem@davemloft.net,
kuba@kernel.org, pabeni@redhat.com, horms@kernel.org,
martin.lau@linux.dev, john.fastabend@gmail.com,
weiwan@google.com, sd@queasysnail.net, jiayuan.chen@linux.dev,
netdev@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks
Date: Wed, 28 Oct 2026 02:13:15 +0000 [thread overview]
Message-ID: <auFUvRnF2uDgZix8@google.com> (raw)
In-Reply-To: <20261026003804.61923-3-imv4bel@gmail.com>
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
>
next prev parent reply other threads:[~2026-10-28 2:13 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-26 0:37 [PATCH net v4 0/2] inet: fix lockless ehash walk races Hyunwoo Kim
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 [this message]
2026-10-28 3:29 ` Hyunwoo Kim
2026-10-28 20:58 ` Kuniyuki Iwashima
2026-10-26 0:42 ` [PATCH net v4 0/2] inet: fix lockless ehash walk races netdev-bot+sinfo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=auFUvRnF2uDgZix8@google.com \
--to=kuniyu@google.com \
--cc=davem@davemloft.net \
--cc=edumazet@kernel.org \
--cc=horms@kernel.org \
--cc=imv4bel@gmail.com \
--cc=jiayuan.chen@linux.dev \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=martin.lau@linux.dev \
--cc=ncardwell@google.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sd@queasysnail.net \
--cc=stable@vger.kernel.org \
--cc=weiwan@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox