Re: [PATCH net 2/2] net: core: take the reuseport migration reference with acquire ordering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] net: invalid acquire rationale and Fixes tag in
  reuseport_migrate_sock()
--

commit 7417b1ab930b0d5e78b0d25b1f59ad89404b15b1
Author: Hang Nan <2122295973@qq.com>

> net: core: take the reuseport migration reference with acquire ordering
>
> reuseport_migrate_sock() returns a listener with a reference held, and
> every caller immediately reads that listener: tcp_v4_rcv() and
> tcp_v6_rcv() pass it to tcp_filter() and tcp_check_req(), and the
> request-socket paths clone its state.  A listener removed from the
> reuseport group concurrently with that lookup must not be observed in a
> half-torn-down state after the reference was successfully taken.
>
> refcount_inc_not_zero() lost its acquire ordering in the refcount_t
> consolidation (fb041bb7c0a9), so these subsequent reads are not ordered
> against the reference acquisition.  Use
> refcount_inc_not_zero_acquire(), as was done for the established-socket
> lookups, inet_twsk_purge() and bpf_sk_assign() in 5b5d8f351dd9.
>
> No behaviour change on strongly ordered architectures.
>
> Fixes: fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hang Nan <2122295973@qq.com>
> ---
> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> index b9e5648fa4c51..56230aa55a3e2 100644
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c
> @@ -667,7 +667,8 @@ struct sock *reuseport_migrate_sock(struct sock *sk,
>  	if (!nsk)
>  		nsk = reuseport_select_sock_by_hash(reuse, hash, socks);
>
> -	if (IS_ERR_OR_NULL(nsk) || unlikely(!refcount_inc_not_zero(&nsk->sk_refcnt))) {
> +	if (IS_ERR_OR_NULL(nsk) ||
> +	    unlikely(!refcount_inc_not_zero_acquire(&nsk->sk_refcnt))) {
>  		nsk = NULL;
>  		goto failure;
>  	}

[Severity: Low]
Does acquire ordering on nsk->sk_refcnt actually prevent callers of
reuseport_migrate_sock() from observing a closing listener?

In __tcp_close(), listener teardown (tcp_set_state(sk, TCP_CLOSE) and
inet_csk_listen_stop()) runs while sk->sk_refcnt is still non-zero, before
tcp_close() drops the final reference with sock_put(sk).  An acquire barrier
on sk_refcnt only orders the caller's subsequent loads after the increment;
it does not synchronize with __tcp_close() and cannot prevent observing a
half-torn-down listener, which is instead handled under rskq_lock in
inet_csk_reqsk_queue_add().

Unlike the SLAB_TYPESAFE_BY_RCU lookups updated in 5b5d8f351dd9 ("net:
restore refcount ordering for lockless socket access"),
reuseport_migrate_sock() does no post-increment re-validation of nsk, and
sk_destruct() always defers destruction via call_rcu() when SOCK_RCU_FREE or
sk_reuseport_cb is set.  Also, reuseport_migrate_sock() was introduced in
1cd62c21572c ("tcp: Add reuseport_migrate_sock() to select a new listener."),
after fb041bb7c0a9 ("locking/refcount: Consolidate implementations of
refcount_t").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_28A2BD1631FE531992E91753BA6419FFC506@qq.com?part=2