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 20:58:50 +0000 [thread overview]
Message-ID: <auJhzo-hCUxII1ce@google.com> (raw)
In-Reply-To: <auFsBkQInOabvUEd@v4bel>
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
> > >
next prev parent reply other threads:[~2026-10-28 20:58 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
2026-10-28 20:58 ` Kuniyuki Iwashima [this message]
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=auJhzo-hCUxII1ce@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