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

[netdev-bot+sashiko@kernel.org]

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

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

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: premature refcount_set_release() in sk_clone() and
  reqsk_timer_handler()
- [Medium] The patch converts only __inet_lookup_established() and
  __inet6_lookup_established() to refcount_inc_not_zero_acquire().

--

inet: restore refcount ordering for established lookups

This patch converts the IPv4 and IPv6 established hash lookups to
refcount_inc_not_zero_acquire(). It also converts the code that sets
initial refcounts in sk_clone(), SYN cookies, request migration and TCP
Fast Open to refcount_set_release(). The goal is to restore the ordering
that weakly ordered architectures lost after the refcount_t
consolidation in v5.5.

> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index b0239d203f045..860145d810ed4 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;

[Severity: Medium]
This converts __inet_lookup_established() and
__inet6_lookup_established(). Other lockless readers of
SLAB_TYPESAFE_BY_RCU sockets take a reference and then revalidate the
object in the same way. Are those readers still using the relaxed
refcount_inc_not_zero()?

inet_twsk_purge() in net/ipv4/inet_timewait_sock.c walks the same ehash
chains and handles TIME_WAIT and NEW_SYN_RECV sockets:

net/ipv4/inet_timewait_sock.c:inet_twsk_purge() {
    ...
			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
				continue;

			if (check_net(sock_net(sk))) {
				sock_gen_put(sk);
				goto restart;
			}
    ...
}

The twsk and request sock slabs are created with prot->slab_flags. For
tcp_prot and tcpv6_prot, that is SLAB_TYPESAFE_BY_RCU. reqsk_timer_handler()
inserts nreq into ehash, and this patch changes it to set nreq's refcount
with refcount_set_release(). inet_twsk_purge() has no matching acquire.

On a weakly ordered CPU, the check_net() load can be satisfied before the
increment. In that case, could inet_twsk_purge() call
inet_twsk_deschedule_put() or inet_csk_reqsk_queue_drop_and_put() on a
recycled object that belongs to a live netns?

It also reads req->rsk_listener after rcu_read_unlock(), with no ordering
against the reference acquisition.

MPTCP has the same pattern on mptcp_sock. mptcp_prot also uses
SLAB_TYPESAFE_BY_RCU in net/mptcp/protocol.c:

net/mptcp/token.c:mptcp_token_get_sock() {
    ...
		if (!refcount_inc_not_zero(&sk->sk_refcnt))
			goto not_found;

		if (READ_ONCE(msk->token) != token ||
		    !net_eq(sock_net(sk), net)) {
    ...
}

net/mptcp/token.c:mptcp_token_iter_next() {
    ...
			if (!refcount_inc_not_zero(&sk->sk_refcnt))
				continue;

			if (!net_eq(sock_net(sk), net)) {
    ...
}

net/mptcp/subflow.c already uses refcount_inc_not_zero_acquire() for the
msk.

__refcount_add_not_zero() uses atomic_try_cmpxchg_relaxed(). The
refcount.h documentation says to use the acquire variant when the
increment is followed by validation on SLAB_TYPESAFE_BY_RCU objects.

Could mptcp_token_get_sock() return the wrong msk for a remote MP_JOIN
SYN? Could mptcp_token_iter_next() return an msk from another netns to
the PM or diag iterators? For the MP_JOIN case, the HMAC check would
most likely reject a wrong msk.

The commit message describes this class of bug as removing "the ordering
required by validation after a successful reference acquisition". Should
these readers also move to refcount_inc_not_zero_acquire(), either in
this patch or in a follow-up?

>  			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
>  				     !inet_match(net, sk, acookie, ports, dif,

[ ... ]

> diff --git a/net/core/sock.c b/net/core/sock.c
> --- 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);
>  
>  	sk_set_socket(newsk, NULL);
>  	sk_tx_queue_clear(newsk);

[Severity: High]
Is this release placed late enough to cover the fields that the
established lookup revalidates?

sk_clone() publishes sk_refcnt here, but several stores still follow
it before the socket is linked into ehash. These include
sk_set_socket(newsk, NULL), sk_tx_queue_clear(), sk_rx_queue_clear()
and the sk_wq reset right below. They also include the tuple fields
(sk_daddr, sk_rcv_saddr, inet_dport, inet_num, sk_v6_daddr, ...) and
inet_sk_set_state(newsk, TCP_SYN_RECV) in inet_csk_clone_lock(). The
child setup in tcp_create_openreq_child() and tcp_v4_syn_recv_sock()
or tcp_v6_syn_recv_sock() comes after that.

Only then does inet_ehash_insert() link the socket. The pprev store in
__sk_nulls_add_node_rcu() or sk_nulls_replace_node_init_rcu() is a
plain WRITE_ONCE() and does not touch sk_refcnt.

Consider a reader in __inet_lookup_established() or
__inet6_lookup_established() that picked up the object pointer during
the object's previous life in the SLAB_TYPESAFE_BY_RCU cache. That
reader never traverses the newly published chain link:

  refcount_inc_not_zero_acquire() succeeds and pairs with the
  release in sk_clone()
  hlist_nulls_unhashed_lockless() reads pprev with READ_ONCE()
  inet_match() / inet6_match() read the tuple with relaxed loads

The acquire only orders the reader against stores made before the
release. The pprev and tuple values are written after the release,
so the reader can observe them with no ordering at all.

On a weakly ordered CPU, the lookup could then pass both checks and
return the socket while still seeing values that sock_copy() took
from the listener. Examples are sk_state == TCP_LISTEN, the
listener's sk_socket and sk_wq, or TCP fields that are not yet
initialized. Wouldn't tcp_v4_rcv() or tcp_v6_rcv() then process the
packet against a half-initialized socket?

reqsk_timer_handler() has the same problem. Moving the release after
timer_setup() helps, but several stores still run between
refcount_set_release(&nreq->rsk_refcnt, 2 + 1) and
inet_ehash_insert(req_to_sk(nreq), req_to_sk(oreq), NULL). These are
tcp_rtx_synack(), the accecn_fail_mode update, nreq->num_timeout++
and mod_timer(&nreq->rsk_timer, ...). A stale reader that sees nreq
hashed has no ordering against any of them.

The release cannot simply be moved past mod_timer() either. Once the
timer is armed, it can already drop references.

By contrast, the TIME_WAIT path that the commit message calls
correct does its smp_wmb() and refcount_set() after
inet_twsk_add_node_rcu(), if I read inet_twsk_hashdance_schedule()
correctly. There the refcount only becomes non-zero after the hash
linkage and every earlier store. Documentation/RCU/rculist_nulls.rst
likewise writes the key before the release on the refcount.

This gap already existed with the old smp_wmb() and refcount_set().
This patch sets out to restore the ordering that the lookup
revalidation depends on, though, so it looks incomplete.

How would you like to close the gap? One option is to make sk_refcnt
visible only after the socket is fully initialized and hashed.
Another is to pair an acquire load of pprev in the lookup with a
release store on the insert side. I'm not sure either option fits the
sk_clone() callers and the request timer's refcount requirements.

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