Re: [PATCH net v2 2/2] inet: restore refcount ordering for established lookups

[Hyunwoo Kim <imv4bel@gmail.com>]

On Fri, Oct 23, 2026 at 06:33:53PM +0200, Eric Dumazet wrote:
> Le ven. 23 oct. 2026 à 07:26, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
> >
> > When commit 3ab5aee7fe84 ("net: Convert TCP & DCCP hash tables to use
> > RCU / hlist_nulls") was written, atomic_inc_not_zero() provided full
> > memory ordering. Even after commit 41c6d650f653 ("net: convert
> > sock.sk_refcnt from atomic_t to refcount_t"), default builds used
> > atomic_add_unless() and retained that ordering.
> >
> > Commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations
> > of refcount_t") consolidated the refcount_t implementations in v5.5.
> > On weakly ordered architectures, this removed the ordering required
> > by validation after a successful reference acquisition.
> >
> > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> > lookups. Use refcount_set_release() when setting initial refcounts in
> > sk_clone(), SYN cookie, and request migration paths. In the TCP Fast
> > Open path, move the refcount update after the required initialization
> > and use release ordering.
> >
> > The TIME_WAIT path already has an smp_wmb() between refcount_set() and
> > hash publication, so leave it unchanged.
> 
> This is not accurate. The smp_wmb() in inet_twsk_hashdance_schedule()
> orders the tw_refcnt store against the ehash publication, not the tw
> identity stores done in inet_twsk_alloc() against the tw_refcnt store.
> 
> The latter is what a lookup holding a stale pointer needs. Today we get it
> more or less by accident from the bhash spin_unlock() followed by
> spin_lock() of the ehash lock.
> 
> Please either reword this paragraph, or simply use
> refcount_set_release(&tw->tw_refcnt, 3) there; it is free under the lock
> and makes the pairing explicit.

OK, I'll switch to refcount_set_release().

> 
> 
> >
> > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> 
> refcount_inc_not_zero_acquire() and refcount_set_release() were only
> added in 7f8ceea0c580 ("refcount: provide ops for cases when object's
> memory can be reused"), in v6.15.
> 
> 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > ---
> >  net/core/sock.c                 | 3 +--
> >  net/ipv4/inet_connection_sock.c | 6 +++---
> >  net/ipv4/inet_hashtables.c      | 2 +-
> >  net/ipv4/syncookies.c           | 2 +-
> >  net/ipv4/tcp_fastopen.c         | 3 +--
> >  net/ipv6/inet6_hashtables.c     | 2 +-
> >  6 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 3c25d66ba31a..f3ef9601ab73 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -2622,8 +2622,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
> >         /* Before updating sk_refcnt, we must commit prior changes to memory
> >          * (Documentation/RCU/rculist_nulls.rst for details)
> >          */
> > -       smp_wmb();
> > -       refcount_set(&newsk->sk_refcnt, 2);
> > +       refcount_set_release(&newsk->sk_refcnt, 2);
> 
> This part is not a fix: the writer side only needs store->store ordering,
> which smp_wmb() already provides.
> 
> I do not mind the conversion, but then reqsk_queue_hash_req() (which has
> the same smp_wmb() + refcount_set() pattern) should be converted as well
> for consistency. Otherwise please drop this hunk to keep the stable
> backport minimal.
> 
> 
> >
> >         sk_set_socket(newsk, NULL);
> >         sk_tx_queue_clear(newsk);
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index cff02f4c5ac8..2d8e54d3a31b 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 58d54965036a..7ffa9171d6de 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/syncookies.c b/net/ipv4/syncookies.c
> > index 644030561537..4b2d2713653b 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 471c78be5513..2506f415f410 100644
> > --- a/net/ipv4/tcp_fastopen.c
> > +++ b/net/ipv4/tcp_fastopen.c
> > @@ -364,8 +364,6 @@ 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);
> > -
> >         sk_mark_napi_id_set(child, skb);
> >
> >         /* Now finish processing the fastopen child socket. */
> > @@ -378,6 +376,7 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
> >         tcp_rsk(req)->rcv_nxt = tp->rcv_nxt;
> >         tp->rcv_wup = tp->rcv_nxt;
> >         tp->rcv_mwnd_seq = tp->rcv_wup + tp->rcv_wnd;
> > +       refcount_set_release(&req->rsk_refcnt, 2);
> >         /* tcp_conn_request() is sending the SYNACK,
> 
> Moving this is harmless, but not needed. The lookup never looks at
> rcv_nxt, and this req is never hashed, so after patch 1/2 a stale lookup
> will reject it anyway. A plain conversion in place would be enough.
> 
> Let's keep code churn minimal, to lower risk of merge conflicts.

I'll submit v3 with all unnecessary changes removed. Thanks for the review.


Best regards,
Hyunwoo Kim