Re: [PATCH v2 net] udp: restore hlist_nulls for primary and secondary hash tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] udp: tail insertion in udp_lib_get_port() defeats hlist_nulls restart check
--

commit f98bb6c6e4827227f9083bb5452ae79a7eeaa175
Author: Eric Dumazet <edumazet@kernel.org>

udp: restore hlist_nulls for primary and secondary hash tables

This commit restores hlist_nulls for the primary (udptable->hash) and
secondary (udptable->hash2) UDP socket hash tables so lockless RCU readers
can detect when a socket is concurrently unhashed or moved to another
bucket and restart the lookup.

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 173b37b2b8bb0..6140244735b55 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
[ ... ]
> @@ -932,7 +932,11 @@ static inline void __sk_nulls_add_node_tail_rcu(struct sock *sk, struct hlist_nu
>  static inline void sk_nulls_add_node_rcu(struct sock *sk, struct hlist_nulls_head *list)
>  {
>  	sock_hold(sk);
> -	__sk_nulls_add_node_rcu(sk, list);
> +	if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
> +	    sk->sk_family == AF_INET6)
> +		__sk_nulls_add_node_tail_rcu(sk, list);
> +	else
> +		__sk_nulls_add_node_rcu(sk, list);
>  }
>  
>  static inline void __sk_del_bind_node(struct sock *sk)
[ ... ]
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 303d0102519ac..7d3790550241f 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
[ ... ]
> @@ -324,7 +327,7 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
>  
>  		sock_set_flag(sk, SOCK_RCU_FREE);
>  
> -		sk_add_node_rcu(sk, &hslot->head);
> +		sk_nulls_add_node_rcu(sk, &hslot->head);
>  		hslot->count++;
>  		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>  
> @@ -332,11 +335,11 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
>  		spin_lock(&hslot2->lock);
>  		if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
>  		    sk->sk_family == AF_INET6)
> -			hlist_add_tail_rcu(&udp_sk(sk)->udp_portaddr_node,
> -					   &hslot2->head);
> +			hlist_nulls_add_tail_rcu(&udp_sk(sk)->udp_portaddr_node,
> +						 &hslot2->head);
>  		else
> -			hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> -					   &hslot2->head);
> +			hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> +						 &hslot2->head);
>  		hslot2->count++;
>  		spin_unlock(&hslot2->lock);
>  	}

[Severity: Medium]
Can the tail insertions in sk_nulls_add_node_rcu() and udp_lib_get_port()
still defeat the get_nulls_value() restart check when a live AF_INET6
SO_REUSEPORT socket is unhashed and re-hashed?

In udp_lib_rehash(), head insertion is used unconditionally with the comment:

	/* Note: head insertion is required here, so
	 * that a lockless reader parked on this socket
	 * restarts from the head of the new chain
	 * instead of hitting a nulls marker early.
	 */
	hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
				 &nhslot2->head);

However, a live AF_INET6 SO_REUSEPORT socket bound without SOCK_BINDPORT_LOCK
(for example via autobind in connect() or sendmsg(), or bind() with port 0,
or when BPF_CGROUP_RUN_PROG_INET6_POST_BIND fails in __inet6_bind()) can also
be unhashed without an RCU grace period via __udp_disconnect() ->
udp_lib_unhash(), and then immediately re-inserted via bind() or connect() ->
udp_lib_get_port().

If bucket S contains:

	head -> sk1 -> sk2 -> NULLS_MARKER(S)

and a lockless reader in udp4_lib_lookup1(), udp4_lib_lookup2(),
udp6_lib_lookup1(), udp6_lib_lookup2(), or __udp4_lib_mcast_demux_lookup() is
parked on sk1 while sk1 is unhashed and re-inserted into the same bucket S
(or if a reader in bucket S jumps via a moved socket into another bucket and
lands on a socket that is concurrently unhashed and tail-inserted into bucket
S), __sk_nulls_add_node_tail_rcu() and hlist_nulls_add_tail_rcu() place sk1
at the end of bucket S:

	head -> sk2 -> sk1 -> NULLS_MARKER(S)

and overwrite sk1's next pointer with NULLS_MARKER(S). When the reader
resumes and advances past sk1, it immediately hits NULLS_MARKER(S) with
get_nulls_value(node) == S, so the restart check passes and the lookup
terminates early without ever visiting sk2.

Also, in the primary hash table (udptable->hash), udp4_lib_lookup1() and
udp6_lib_lookup1() always score every socket in hslot->head and do not use
inet_lookup_reuseport() or inet6_lookup_reuseport(), so tail insertion in
sk_nulls_add_node_rcu() does not appear to help reuseport selection on
hslot->head.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030122045.3035709-1-edumazet@kernel.org?part=1