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

[Hyunwoo Kim <imv4bel@gmail.com>]

On Sun, Oct 25, 2026 at 05:40:28AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (1) · Low: 4
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] inet_twsk_purge() (net/ipv4/inet_timewait_sock.c) was left
>   unconverted, so the fix is incomplete.
> - [Low] The patch carries 'Cc: stable@vger.kernel.org' and 'Fixes:
>   fb041bb7c0a9' (v5.5), but the fix depends on…
> - [Low] Commit message accuracy.
> - [Low] Several converted sites do not follow refcount_set_release()'s
>   kernel-doc contract: 'This function should be called only after new…
> - [Low] In reqsk_timer_handler() the patch moves
>   timer_setup(&nreq->rsk_timer, ...) ahead of the refcount store, and…
> 
> Pre-existing issues:
> - [Medium] Same ordering bug in MPTCP token lookups.
> 
> --
> 
> inet: restore refcount ordering for established lookups
> 
> This patch makes the IPv4 and IPv6 established hash lookups use
> refcount_inc_not_zero_acquire(). It also converts refcount_set() to
> refcount_set_release() in the SYN cookie, TCP Fast Open, request
> migration and TIME_WAIT paths. The goal is to bring back ordering that
> was lost when fb041bb7c0a9 made refcount_inc_not_zero() relaxed on
> success.
> 
> > inet: restore refcount ordering for established lookups
> >
> > 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 in established lookups to be reordered before a successful
> > reference acquisition.
> >
> > Use refcount_inc_not_zero_acquire() in the IPv4 and IPv6 established
> > lookups. 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.
> 
> [Severity: Low]
> Is "restore" accurate for the refcount_set_release() hunks?
> 
> fb041bb7c0a9 only changed the reader side. The !REFCOUNT_FULL
> refcount_inc_not_zero() went from atomic_add_unless() to a relaxed
> cmpxchg loop. refcount_set() was a plain atomic_set() both before and
> after that commit.
> 
> So the six conversions add writer-side ordering that never existed. They
> don't restore it. The six are in tcp_get_cookie_sock(),
> tcp_fastopen_create_child(), reqsk_timer_handler(),
> inet_csk_complete_hashdance(), inet_csk_listen_stop() and
> inet_twsk_hashdance_schedule().
> 
> The changes themselves look justified. A stale reader using acquire
> ordering has to see the pprev = NULL from sk_node_init() and the new tuple
> fields before the new refcount. Otherwise it could match an unhashed
> syncookie, TFO or migrated req.
> 
> Could the changelog explain this? It could also say how recycled request
> sock slab memory makes these unhashed reqs reachable at all.
> 
> sk_clone_lock() and sock_init_data() already have an smp_wmb() before
> their refcount_set(), so leaving those two alone looks right.
> 
> > Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> > Cc: stable@vger.kernel.org
> 
> [Severity: Low]
> refcount_inc_not_zero_acquire() and refcount_set_release() were added by
> 7f8ceea0c58039 ("refcount: provide ops for cases when object's memory can
> be reused"). That commit is in v6.15 and not in v6.14.
> 
> The Fixes: tag points to v5.5. Without that prerequisite, none of the
> touched files will build in the 5.10, 5.15, 6.1, 6.6 and 6.12 stable
> trees.
> 
> Should the stable tag carry a prerequisite annotation, for example:
> 
> Cc: stable@vger.kernel.org # needs 7f8ceea0c580 ("refcount: provide ops ...")
> 
> Or is a fallback planned for older trees?
> 
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index 01ecd98a2b501..8faef6c38b364 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -1060,8 +1060,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);
> 
> [Severity: Low]
> This isn't a bug, but timer_setup() now runs before the refcount store,
> and the changelog doesn't mention the move.
> 
> It follows from the refcount_set_release() kernel-doc, which wants
> initialisation done before the release. nreq is still private at this
> point. A short note in the commit message would be enough.
> 
> >  		reqsk_queue_migrated(&inet_csk(nsk)->icsk_accept_queue, req);
> >  
> >  		req = nreq;
> 
> [ ... ]
> 
> > diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> > index a1d86026aefb0..5c1bff5a6d4d8 100644
> > --- a/net/ipv4/inet_timewait_sock.c
> > +++ b/net/ipv4/inet_timewait_sock.c
> > @@ -143,7 +143,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);
> 
> [Severity: High]
> Is the fix complete without also converting inet_twsk_purge() in this
> same file?
> 
> It walks the same ehash chains locklessly under rcu_read_lock(). It
> handles the same SLAB_TYPESAFE_BY_RCU TIME_WAIT and request socks, and it
> still uses the relaxed variant:
> 
> net/ipv4/inet_timewait_sock.c:inet_twsk_purge() {
>     ...
> 			if (check_net(sock_net(sk)))
> 				continue;
> 
> 			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> 				continue;
> 
> 			if (check_net(sock_net(sk))) {
> 				sock_gen_put(sk);
> 				goto restart;
> 			}
>     ...
> 				inet_csk_reqsk_queue_drop_and_put(req->rsk_listener,
> 								  req);
>     ...
> }
> 
> The release store added here only helps readers that pair it with an
> acquire. __refcount_add_not_zero() uses atomic_try_cmpxchg_relaxed(). On
> arm64 or POWER, the second check_net() and the later req->rsk_listener
> load can therefore be satisfied before the CAS.
> 
> Can this sequence happen?
> 
> CPU0, in tcp_sk_exit_batch()->tcp_twsk_purge()->inet_twsk_purge() for
> dying netns D:
>   sees socket X in D, and the first check_net() is false
> 
> CPU1:
>   X is freed
>   the slot is reused for a new TIME_WAIT or request sock in live netns L
>   the new sock is initialised, gets refcount_set_release(), and is hashed
> 
> CPU0:
>   refcount_inc_not_zero() succeeds on the new object
>   the reordered second check_net() still reads D
>   inet_twsk_deschedule_put() or inet_csk_reqsk_queue_drop_and_put() runs
> 
> That tears down a TIME_WAIT or pending request sock that belongs to the
> live netns L.
> 
> req->rsk_listener could also be read stale. It would then be the old
> occupant's listener, which may already be released, and
> inet_csk_reqsk_queue_drop() would update that listener's accept queue
> counters.
> 
> An unprivileged user can trigger netns exit through user namespaces. A
> remote peer can create TIME_WAIT and request socks in L.
> 
> Should inet_twsk_purge() use refcount_inc_not_zero_acquire() too?

I will include this change in v4.

> 
> [ ... ]
> 
> > 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);
> 
> [Severity: Low]
> The kernel-doc for refcount_set_release() says:
> 
>  * This function should be called only after new object is fully initialized.
>  * After this call the object should be considered visible to other tasks
> 
> Later in tcp_fastopen_create_child(), req is still written after the
> release store:
> 
> 	tcp_rsk(req)->rcv_nxt = tp->rcv_nxt;
> 
> The other converted sites have the same ordering:
> 
>   tcp_get_cookie_sock(), inet_csk_complete_hashdance() and
>   inet_csk_listen_stop() do the release before
>   inet_csk_reqsk_queue_add(), which writes req->sk and req->dl_next.
> 
>   reqsk_timer_handler() does the release before req->num_timeout++, the
>   accecn_fail_mode update and mod_timer() on nreq. nreq is then hashed
>   by inet_ehash_insert().
> 
> For the reqs that are never hashed, hlist_nulls_unhashed_lockless()
> rejects stale established lookups, because sk_node_init() runs before the
> release. So the practical exposure seems limited to minor fields such as
> num_timeout on the migrated nreq.
> 
> These stores had no ordering at all before this patch, so nothing
> regresses.
> 
> Could the release stores move after the last initialising write? If not,
> a comment could explain why the later writes are fine.
> 
> > 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;
> 
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch, but
> the MPTCP token lookups seem to have the same pattern.
> 
> mptcp_token_get_sock() in net/mptcp/token.c re-validates after a relaxed
> increment:
> 
> 	if (!refcount_inc_not_zero(&sk->sk_refcnt))
> 		goto not_found;
> 
> 	if (READ_ONCE(msk->token) != token ||
> 	    !net_eq(sock_net(sk), net)) {
> 
> mptcp_token_iter_next() does the same thing with its net_eq() check.
> 
> mptcp_prot sets SLAB_TYPESAFE_BY_RCU for msk objects. For that reason,
> mptcp_subflow_reqsk_clone() in net/mptcp/subflow.c already uses
> refcount_inc_not_zero_acquire().
> 
> On weakly ordered CPUs, can these lookups return a recycled msk from
> another connection or netns?
> 
> For MP_JOIN, the later HMAC check would fail and cause a spurious reset.
> Through the diag iterator, an msk from another netns could be dumped.
> 
> MPTCP was merged in v5.6, after fb041bb7c0a9. These sites never had the
> fully ordered behaviour, and they are outside what this patch covers.
> Would a separate fix for them make sense?

I had already identified the MPTCP issue. To simplify review and 
backporting, I will address it separately in a later patch rather than 
include it in this series.

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024053720.1147022-1-imv4bel%40gmail.com