Re: [PATCH net v2 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] At this commit, the new post-reference checks
  hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) and…

--

inet: reject unhashed sockets during established lookups

This commit adds an 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 of a socket now restarts instead of returning
an unhashed object. The SYN cookie, Fast Open and request migration
failure paths now drop references with reqsk_put() instead of calling
__reqsk_free() directly.

> 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]
On weakly ordered architectures, are the new pprev load and the repeated
tuple check guaranteed to happen after the refcount increment?

refcount_inc_not_zero() is relaxed here. At this commit, the code that
publishes a nonzero refcount on never-hashed request sockets also uses
plain stores:

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

net/ipv4/tcp_fastopen.c:tcp_fastopen_create_child() {
    ...
	refcount_set(&req->rsk_refcnt, 2);
    ...
}

The inet_reqsk_clone() users do the same. They are reqsk_timer_handler()
(refcount_set(&nreq->rsk_refcnt, 2 + 1)), inet_csk_complete_hashdance()
and inet_csk_listen_stop() (refcount_set(&nreq->rsk_refcnt, 1)).

On arm64 or POWER, the hlist_nulls_unhashed_lockless() and inet_match()
loads could be satisfied before the cmpxchg in refcount_inc_not_zero().
If so, the lookup could still return the recycled, unhashed SYN cookie
reqsk with rsk_listener == NULL. That is the tcp_v4_rcv() crash described
in the commit message. The same applies to __inet6_lookup_established(),
which also uses a relaxed refcount_inc_not_zero() before the new check.

This ordering is older than this patch. It is fully fixed by the next
patch in the series, "inet: restore refcount ordering for established
lookups". That patch switches both lookups to
refcount_inc_not_zero_acquire(), and switches tcp_get_cookie_sock(),
tcp_fastopen_create_child() (moving the set after initialization),
reqsk_timer_handler(), inet_csk_complete_hashdance(),
inet_csk_listen_stop() and sk_clone() to refcount_set_release().

Both patches carry Cc: stable. Should they be marked so that they are
backported together, so that this one is not picked up alone?

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

[ ... ]

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