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

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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).

- [Medium] The commit message says the reuseport_migrate_sock() refcount
  change prevents a listener that is concurrently removed from the…
- [Low] The comment in inet_reqsk_clone()
  (net/ipv4/inet_connection_sock.c:925) says its sock_put(sk) on the…

--

net: core: take the reuseport migration reference with acquire ordering

This commit changes reuseport_migrate_sock() to take the reference on the
selected listener with refcount_inc_not_zero_acquire() instead of
refcount_inc_not_zero(). The stated goal is to stop callers from observing
a listener in a half-torn-down state when it is removed from the reuseport
group at the same time. The patch carries Fixes: and Cc: stable tags.

[Severity: Medium]
What does the acquire ordering on this increment order against? The commit
message says:

  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.

reuseport_migrate_sock() picks nsk from reuse->socks[] while holding
rcu_read_lock(). Every hashed listener gets SOCK_RCU_FREE in
__inet_hash():

net/ipv4/inet_hashtables.c:__inet_hash() {
    ...
	sock_set_flag(sk, SOCK_RCU_FREE);
    ...
}

So the listener's memory cannot be freed or reused for another socket
inside this RCU section. Also, nothing after the increment re-validates
nsk. There is no sk_state or sk_reuseport_cb re-check before nsk is
returned.

The contract for the helper in include/linux/refcount.h limits it to that
reuse case:

  refcount_{add|inc}_not_zero_acquire() and refcount_set_release() provide
  acquire and release ordering for cases when the memory occupied by the
  object might be reused to store another object. This is important for the
  cases where secondary validation is required to detect such reuse, e.g.
  SLAB_TYPESAFE_BY_RCU. The secondary validation checks have to happen after
  the refcount is taken, hence acquire order is necessary.

Neither condition holds here. Commit 5b5d8f351dd9, cited as precedent, only
changed sites where the socket is validated again after the reference is
taken. __inet_lookup_established() re-runs inet_match() and checks the
unhashed state, and inet_twsk_purge() re-checks check_net(). Its
bpf_sk_assign() change had a separate justification: a BTF socket pointer
obtained without a reference.

A listener that is being closed is torn down while the closer still holds
its own reference. That teardown is not published through a release on
sk_refcnt. So even with the acquire, can't a caller still see the closing
listener?

Callers already handle that state with an explicit check, for example in
inet_csk_reqsk_queue_add():

	spin_lock(&queue->rskq_lock);
	if (unlikely(sk->sk_state != TCP_LISTEN)) {
		inet_child_forget(sk, req, child);

The same pattern also exists elsewhere and is left alone. inet_lookup(),
inet6_lookup(), and nf_tproxy_get_sock_v4()/nf_tproxy_get_sock_v6() all
take a reference on an unreferenced listener with plain
refcount_inc_not_zero() and then use it. Those sites look correct for the
same RCU-stable reason, which suggests this one is correct as well without
the change.

Given that, are the Fixes: fb041bb7c0a9 and Cc: stable tags justified?
Could the commit message name a concrete load that can see stale data here,
or include a reproducer? As written, the patch adds a barrier on weakly
ordered architectures but does not change what callers can observe.

 		nsk = NULL;
 		goto failure;
 	}

[Severity: Low]
This isn't a bug, but the comment on the matching release in
inet_reqsk_clone() still names the old primitive:

net/ipv4/inet_connection_sock.c:inet_reqsk_clone() {
    ...
	if (!nreq) {
		__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMIGRATEREQFAILURE);

		/* paired with refcount_inc_not_zero() in reuseport_migrate_sock() */
		sock_put(sk);
		return NULL;
	}
    ...
}

Should this comment be updated if the switch to
refcount_inc_not_zero_acquire() is kept?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_28A2BD1631FE531992E91753BA6419FFC506%40qq.com