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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.