From: Hyunwoo Kim <imv4bel@gmail.com>
To: Kuniyuki Iwashima <kuniyu@google.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,
imv4bel@gmail.com
Subject: Re: [PATCH net v4 2/2] inet: restore refcount ordering for lockless ehash walks
Date: Wed, 28 Oct 2026 12:29:10 +0900 [thread overview]
Message-ID: <auFsBkQInOabvUEd@v4bel> (raw)
In-Reply-To: <auFUvRnF2uDgZix8@google.com>
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
> >
next prev parent reply other threads:[~2026-10-28 3:29 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
2026-10-28 3:29 ` Hyunwoo Kim [this message]
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=auFsBkQInOabvUEd@v4bel \
--to=imv4bel@gmail.com \
--cc=davem@davemloft.net \
--cc=edumazet@kernel.org \
--cc=horms@kernel.org \
--cc=jiayuan.chen@linux.dev \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=kuniyu@google.com \
--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