Re: [PATCH net v5 1/2] inet: reject unhashed sockets during established lookups

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In this commit by itself, the new
  `hlist_nulls_unhashed_lockless(&sk->sk_nulls_node)` check in…

--

inet: reject unhashed sockets during established lookups

This commit adds a hlist_nulls_unhashed_lockless() check after the
reference is taken in __inet_lookup_established() and
__inet6_lookup_established(). A lookup that races with SLAB_TYPESAFE_BY_RCU
reuse can no longer return a request socket that was never hashed. It also
changes the SYN cookie, Fast Open and request migration failure paths to
drop references with reqsk_put() instead of calling __reqsk_free().

> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 5abcb0debb270..b0239d203f045 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -552,8 +552,9 @@ struct sock *__inet_lookup_established(const struct net *net,
>  		if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) {
>  			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
>  				goto out;
> -			if (unlikely(!inet_match(net, sk, acookie,
> -						 ports, dif, sdif))) {
> +			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
> +				     !inet_match(net, sk, acookie, ports, dif,
> +						 sdif))) {

[Severity: Low]
If this commit is applied by itself, is the new unhashed check ordered
after the refcount increment?

refcount_inc_not_zero() here is a relaxed cmpxchg. The writers that make
never-hashed request sockets referenceable also use a plain refcount_set():

net/ipv4/syncookies.c:tcp_get_cookie_sock() {
    ...
	if (child) {
		refcount_set(&req->rsk_refcnt, 1);
    ...
}

The same applies to tcp_fastopen_create_child(), and to the migration
clones in reqsk_timer_handler(), inet_csk_complete_hashdance() and
inet_csk_listen_stop().

On weakly ordered CPUs, can the pprev load in
hlist_nulls_unhashed_lockless() and the inet_match() reload be satisfied
before the increment? They could then see the old, hashed incarnation,
while the increment succeeds on the new, unhashed SYN cookie request whose
rsk_listener is NULL. That is the tcp_v4_rcv() NULL dereference this
commit describes. __inet6_lookup_established() has the same pattern.

The next patch in the series, "net: restore refcount ordering for lockless
socket access", resolves this. It switches both lookups to
refcount_inc_not_zero_acquire(), and switches tcp_get_cookie_sock(),
tcp_fastopen_create_child() and the three migration writers to
refcount_set_release().

Both patches are tagged Cc: stable. Should the two be backported together,
so stable trees don't pick up this one without the ordering fix?

>  				sock_gen_put(sk);
>  				goto begin;
>  			}

[ ... ]

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