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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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] The concern: at this commit, __inet_lookup_established() and
  __inet6_lookup_established() take the reference with a relaxed…

--

inet: reject unhashed sockets during established lookups

This patch adds a hlist_nulls_unhashed_lockless() check after the
reference is taken in __inet_lookup_established() and
__inet6_lookup_established(). A SLAB_TYPESAFE_BY_RCU slot that was reused
for an unhashed socket with the same tuple is then rejected. The SYN
cookie, Fast Open and request migration failure paths now use
reqsk_put(), because requests that were never hashed can hold transient
references from stale lookups.

> 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))) {
>  				sock_gen_put(sk);
>  				goto begin;
>  			}

[Severity: High]
Is the new unhashed check ordered after the refcount increment?

refcount_inc_not_zero() is relaxed. On weakly-ordered CPUs, the loads of
sk->sk_nulls_node.pprev and of the tuple fields in the re-check could
complete before the increment does.

On the writer side, tcp_get_cookie_sock() publishes the refcount of the
never-hashed request (the object reused in the PoC) with a plain store:

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

tcp_fastopen_create_child() uses the same plain refcount_set() pattern.
So does the cloned nreq in reqsk_timer_handler(),
inet_csk_complete_hashdance() and inet_csk_listen_stop().

According to include/linux/refcount.h, secondary validation under
SLAB_TYPESAFE_BY_RCU needs refcount_inc_not_zero_acquire() paired with
refcount_set_release().

Without that pairing, could a lookup see the new SYN cookie request's
nonzero refcount but still read the old object's non-NULL pprev? It
would then pass both checks and return the unhashed request, with its
NULL rsk_listener, to tcp_v4_rcv(). That is the NULL dereference in the
commit message, here on weakly-ordered architectures.

The same question applies to __inet6_lookup_established().

The next patch in this series, "inet: restore refcount ordering for
established lookups", appears to switch both lookups to
refcount_inc_not_zero_acquire(). It also converts the publishers above to
refcount_set_release(). This patch is tagged for stable. Could the
ordering change be folded into this patch, or could the two patches be
kept together for backports?

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