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

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

[Eric Dumazet]

Commit ca065d0cf80f ("udp: no longer use SLAB_DESTROY_BY_RCU") switched
UDP sockets to SOCK_RCU_FREE and converted udptable->hash and
udptable->hash2 from hlist_nulls_head to hlist_head.

While SOCK_RCU_FREE guarantees that a socket is not freed before an RCU
grace period elapses, a live UDP socket can still be unhashed or moved
to a different hash bucket without an RCU grace period:

1. udp_lib_rehash() moves sk->skc_portaddr_node from hslot2 to a
   different nhslot2 when inet_rcv_saddr changes (for instance via
   connect() or disconnect() on an wildcard-bound socket).
2. __udp_disconnect() calls udp_lib_unhash() when an implicitly bound
   port was used, and a subsequent connect() or bind() can immediately
   re-insert sk->sk_nulls_node and sk->skc_portaddr_node into different
   hash and hash2 buckets.

Because hlist_add_head_rcu() overwrites node->next with the new bucket
chain without waiting for an RCU grace period, a concurrent lockless
reader in udp4_lib_lookup1/2() or udp6_lib_lookup1/2() traversing the
old bucket can silently jump to the new bucket chain, terminate early,
and miss a matching socket that was located later in the original bucket.

Restore hlist_nulls for udptable->hash and udptable->hash2 while keeping
SOCK_RCU_FREE (so RCU lookups remain refcount-free), and restart the
bucket traversal if get_nulls_value(node) does not match the expected
bucket index.

Fixes: ca065d0cf80f ("udp: no longer use SLAB_DESTROY_BY_RCU")
Assisted-by: LLM
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
---
 include/linux/udp.h |  11 +++--
 include/net/sock.h  |  23 +++++----
 include/net/udp.h   |   5 +-
 net/ipv4/udp.c      | 112 ++++++++++++++++++++++++++++++--------------
 net/ipv4/udp_diag.c |   5 +-
 net/ipv6/udp.c      |  40 ++++++++++++----
 6 files changed, 131 insertions(+), 65 deletions(-)

diff --git a/include/linux/udp.h b/include/linux/udp.h
index 998906ec3b32add4652bde3ee9c28e8b2b98ce51..a1990350fd22f3418724c0e06d4b7405779f7c2b 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -243,14 +243,15 @@ static inline void udp_allow_gso(struct sock *sk)
 	udp_set_bit(ACCEPT_FRAGLIST, sk);
 }
 
-#define udp_portaddr_for_each_entry(__sk, list) \
-	hlist_for_each_entry(__sk, list, __sk_common.skc_portaddr_node)
+#define udp_portaddr_for_each_entry(__sk, node, list) \
+	hlist_nulls_for_each_entry(__sk, node, list, __sk_common.skc_portaddr_node)
 
 #define udp_portaddr_for_each_entry_from(__sk) \
-	hlist_for_each_entry_from(__sk, __sk_common.skc_portaddr_node)
+	for (; __sk; __sk = hlist_nulls_entry_safe((__sk)->__sk_common.skc_portaddr_node.next, \
+						   typeof(*(__sk)), __sk_common.skc_portaddr_node))
 
-#define udp_portaddr_for_each_entry_rcu(__sk, list) \
-	hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
+#define udp_portaddr_for_each_entry_rcu(__sk, node, list) \
+	hlist_nulls_for_each_entry_rcu(__sk, node, list, __sk_common.skc_portaddr_node)
 
 #if !IS_ENABLED(CONFIG_BASE_SMALL)
 #define udp_lrpa_for_each_entry_rcu(__up, node, list) \
diff --git a/include/net/sock.h b/include/net/sock.h
index 60ea55dc18854a9759f5df618cc8c904d2323e95..78626aba52dca6ab9fcae60f5bc277089f143633 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -184,7 +184,7 @@ struct sock_common {
 	int			skc_bound_dev_if;
 	union {
 		struct hlist_node	skc_bind_node;
-		struct hlist_node	skc_portaddr_node;
+		struct hlist_nulls_node	skc_portaddr_node;
 	};
 	struct proto		*skc_prot;
 	possible_net_t		skc_net;
@@ -930,7 +930,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)
@@ -965,18 +969,19 @@ static inline void sk_add_bind_node(struct sock *sk,
 	hlist_for_each_entry_safe(__sk, tmp, list, sk_bind_node)
 
 /**
- * sk_for_each_entry_offset_rcu - iterate over a list at a given struct offset
+ * sk_nulls_for_each_entry_offset_rcu - iterate over a list at a given struct offset
  * @tpos:	the type * to use as a loop cursor.
- * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @pos:	the &struct hlist_nulls_node to use as a loop cursor.
  * @head:	the head for your list.
- * @offset:	offset of hlist_node within the struct.
+ * @offset:	offset of hlist_nulls_node within the struct.
  *
  */
-#define sk_for_each_entry_offset_rcu(tpos, pos, head, offset)		       \
-	for (pos = rcu_dereference(hlist_first_rcu(head));		       \
-	     pos != NULL &&						       \
+#define sk_nulls_for_each_entry_offset_rcu(tpos, pos, head, offset)	       \
+	for (({ barrier(); }),						       \
+	     pos = rcu_dereference_raw(hlist_nulls_first_rcu(head));	       \
+	     (!is_a_nulls(pos)) &&					       \
 		({ tpos = (typeof(*tpos) *)((void *)pos - offset); 1;});       \
-	     pos = rcu_dereference(hlist_next_rcu(pos)))
+	     pos = rcu_dereference_raw(hlist_nulls_next_rcu(pos)))
 
 static inline struct user_namespace *sk_user_ns(const struct sock *sk)
 {
diff --git a/include/net/udp.h b/include/net/udp.h
index 1fee17274745f0b52837b7eb2498dbc423a450fd..1bba33479341e07f2dde9acc19b0b09cafa3ef29 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -56,10 +56,7 @@ struct udp_skb_cb {
  */
 struct udp_hslot {
 	union {
-		struct hlist_head	head;
-		/* hash4 uses hlist_nulls to avoid moving wrongly onto another
-		 * hlist, because rehash() can happen with lookup().
-		 */
+		struct hlist_nulls_head	head;
 		struct hlist_nulls_head	nulls_head;
 	};
 	int			count;
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index b090bd1f59e86cd22edd9622b17b5679346ea24d..309220bf2fba9e7ccf48676d312924a6881bc34f 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -136,10 +136,11 @@ static int udp_lib_lport_inuse(struct net *net, __u16 num,
 			       unsigned long *bitmap,
 			       struct sock *sk, unsigned int log)
 {
+	struct hlist_nulls_node *node;
 	kuid_t uid = sk_uid(sk);
 	struct sock *sk2;
 
-	sk_for_each(sk2, &hslot->head) {
+	sk_nulls_for_each(sk2, node, &hslot->head) {
 		if (net_eq(sock_net(sk2), net) &&
 		    sk2 != sk &&
 		    (bitmap || udp_sk(sk2)->udp_port_hash == num) &&
@@ -171,12 +172,13 @@ static int udp_lib_lport_inuse2(struct net *net, __u16 num,
 				struct udp_hslot *hslot2,
 				struct sock *sk)
 {
+	struct hlist_nulls_node *node;
 	kuid_t uid = sk_uid(sk);
 	struct sock *sk2;
 	int res = 0;
 
 	spin_lock(&hslot2->lock);
-	udp_portaddr_for_each_entry(sk2, &hslot2->head) {
+	udp_portaddr_for_each_entry(sk2, node, &hslot2->head) {
 		if (net_eq(sock_net(sk2), net) &&
 		    sk2 != sk &&
 		    (udp_sk(sk2)->udp_port_hash == num) &&
@@ -201,10 +203,11 @@ static int udp_lib_lport_inuse2(struct net *net, __u16 num,
 static int udp_reuseport_add_sock(struct sock *sk, struct udp_hslot *hslot)
 {
 	struct net *net = sock_net(sk);
+	struct hlist_nulls_node *node;
 	kuid_t uid = sk_uid(sk);
 	struct sock *sk2;
 
-	sk_for_each(sk2, &hslot->head) {
+	sk_nulls_for_each(sk2, node, &hslot->head) {
 		if (net_eq(sock_net(sk2), net) &&
 		    sk2 != sk &&
 		    sk2->sk_family == sk->sk_family &&
@@ -323,7 +326,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);
 
@@ -331,11 +334,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);
 	}
@@ -440,10 +443,14 @@ static struct sock *udp4_lib_lookup1(const struct net *net,
 {
 	unsigned int slot = udp_hashfn(net, hnum, udptable->mask);
 	struct udp_hslot *hslot = &udptable->hash[slot];
-	struct sock *sk, *result = NULL;
-	int score, badness = 0;
+	struct hlist_nulls_node *node;
+	struct sock *sk, *result;
+	int score, badness;
 
-	sk_for_each_rcu(sk, &hslot->head) {
+begin:
+	result = NULL;
+	badness = 0;
+	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
 		score = compute_score(sk, net,
 				      saddr, sport, daddr, hnum, dif, sdif);
 		if (score > badness) {
@@ -451,6 +458,13 @@ static struct sock *udp4_lib_lookup1(const struct net *net,
 			badness = score;
 		}
 	}
+	/*
+	 * if the nulls value we got at the end of this lookup is
+	 * not the expected one, we must restart lookup.
+	 * We probably met an item that was moved to another chain.
+	 */
+	if (unlikely(get_nulls_value(node) != slot))
+		goto begin;
 
 	return result;
 }
@@ -463,13 +477,16 @@ static struct sock *udp4_lib_lookup2(const struct net *net,
 				     struct udp_hslot *hslot2,
 				     struct sk_buff *skb)
 {
+	unsigned int slot2 = UDP_HSLOT_MAIN(hslot2) - net->ipv4.udp_table->hash2;
+	struct hlist_nulls_node *node;
 	struct sock *sk, *result;
 	int score, badness;
 	bool need_rescore;
 
+begin:
 	result = NULL;
 	badness = 0;
-	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
+	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
 		need_rescore = false;
 rescore:
 		score = compute_score(need_rescore ? result : sk, net, saddr,
@@ -510,6 +527,13 @@ static struct sock *udp4_lib_lookup2(const struct net *net,
 			goto rescore;
 		}
 	}
+	/*
+	 * if the nulls value we got at the end of this lookup is
+	 * not the expected one, we must restart lookup.
+	 * We probably met an item that was moved to another chain.
+	 */
+	if (unlikely(get_nulls_value(node) != slot2))
+		goto begin;
 	return result;
 }
 
@@ -562,7 +586,7 @@ static struct sock *udp4_lib_lookup4(const struct net *net,
 	 * expected one, we must restart lookup. We probably met an item that
 	 * was moved to another chain due to rehash.
 	 */
-	if (get_nulls_value(node) != slot)
+	if (unlikely(get_nulls_value(node) != slot))
 		goto begin;
 
 	return NULL;
@@ -2251,13 +2275,13 @@ void udp_lib_unhash(struct sock *sk)
 		spin_lock_bh(&hslot->lock);
 		if (rcu_access_pointer(sk->sk_reuseport_cb))
 			reuseport_detach_sock(sk);
-		if (sk_del_node_init_rcu(sk)) {
+		if (sk_nulls_del_node_init_rcu(sk)) {
 			hslot->count--;
 			inet_sk(sk)->inet_num = 0;
 			sock_prot_inuse_add(net, sk->sk_prot, -1);
 
 			spin_lock(&hslot2->lock);
-			hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
+			hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
 			hslot2->count--;
 			spin_unlock(&hslot2->lock);
 
@@ -2291,13 +2315,18 @@ void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4)
 
 			if (hslot2 != nhslot2) {
 				spin_lock(&hslot2->lock);
-				hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
+				hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
 				hslot2->count--;
 				spin_unlock(&hslot2->lock);
 
 				spin_lock(&nhslot2->lock);
-				hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
-							 &nhslot2->head);
+				if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
+				    sk->sk_family == AF_INET6)
+					hlist_nulls_add_tail_rcu(&udp_sk(sk)->udp_portaddr_node,
+								 &nhslot2->head);
+				else
+					hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
+								 &nhslot2->head);
 				nhslot2->count++;
 				spin_unlock(&nhslot2->lock);
 			}
@@ -2513,9 +2542,9 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	unsigned int hash2, hash2_any, offset;
 	unsigned short hnum = ntohs(uh->dest);
 	struct sock *sk, *first = NULL;
+	struct hlist_nulls_node *node;
 	int dif = skb->dev->ifindex;
 	int sdif = inet_sdif(skb);
-	struct hlist_node *node;
 	struct udp_hslot *hslot;
 	struct sk_buff *nskb;
 	bool use_hash2;
@@ -2525,7 +2554,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	hash2 = 0;
 	hslot = udp_hashslot(udptable, net, hnum);
 	use_hash2 = hslot->count > 10;
-	offset = offsetof(typeof(*sk), sk_node);
+	offset = offsetof(typeof(*sk), sk_nulls_node);
 
 	if (use_hash2) {
 		hash2_any = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
@@ -2536,7 +2565,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
 	}
 
-	sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
+	sk_nulls_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
 		if (!__udp_is_mcast_sock(net, sk, uh->dest, daddr,
 					 uh->source, saddr, dif, sdif, hnum))
 			continue;
@@ -2749,6 +2778,7 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
 {
 	struct udp_table *udptable = net->ipv4.udp_table;
 	unsigned short hnum = ntohs(loc_port);
+	struct hlist_nulls_node *node;
 	struct sock *sk, *result;
 	struct udp_hslot *hslot;
 	unsigned int slot;
@@ -2760,8 +2790,9 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
 	if (hslot->count > 10)
 		return NULL;
 
+begin:
 	result = NULL;
-	sk_for_each_rcu(sk, &hslot->head) {
+	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
 		if (__udp_is_mcast_sock(net, sk, loc_port, loc_addr,
 					rmt_port, rmt_addr, dif, sdif, hnum)) {
 			if (result)
@@ -2769,6 +2800,13 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
 			result = sk;
 		}
 	}
+	/*
+	 * if the nulls value we got at the end of this lookup is
+	 * not the expected one, we must restart lookup.
+	 * We probably met an item that was moved to another chain.
+	 */
+	if (unlikely(get_nulls_value(node) != slot))
+		goto begin;
 
 	return result;
 }
@@ -2785,6 +2823,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
 	struct udp_table *udptable = net->ipv4.udp_table;
 	INET_ADDR_COOKIE(acookie, rmt_addr, loc_addr);
 	unsigned short hnum = ntohs(loc_port);
+	struct hlist_nulls_node *node;
 	struct udp_hslot *hslot2;
 	unsigned int hash2;
 	__portpair ports;
@@ -2794,7 +2833,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
 	hslot2 = udp_hashslot2(udptable, hash2);
 	ports = INET_COMBINED_PORTS(rmt_port, hnum);
 
-	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
+	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
 		if (inet_match(net, sk, acookie, ports, dif, sdif))
 			return sk;
 		/* Only check first socket in chain */
@@ -3228,6 +3267,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start)
 {
 	struct udp_iter_state *state = seq->private;
 	struct net *net = seq_file_net(seq);
+	struct hlist_nulls_node *node;
 	struct udp_table *udptable;
 	struct sock *sk;
 
@@ -3237,11 +3277,11 @@ static struct sock *udp_get_first(struct seq_file *seq, int start)
 	     ++state->bucket) {
 		struct udp_hslot *hslot = &udptable->hash[state->bucket];
 
-		if (hlist_empty(&hslot->head))
+		if (hlist_nulls_empty(&hslot->head))
 			continue;
 
 		spin_lock_bh(&hslot->lock);
-		sk_for_each(sk, &hslot->head) {
+		sk_nulls_for_each(sk, node, &hslot->head) {
 			if (seq_sk_match(seq, sk))
 				goto found;
 		}
@@ -3259,7 +3299,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk)
 	struct udp_table *udptable;
 
 	do {
-		sk = sk_next(sk);
+		sk = sk_nulls_next(sk);
 	} while (sk && !seq_sk_match(seq, sk));
 
 	if (!sk) {
@@ -3431,12 +3471,12 @@ static struct sock *bpf_iter_udp_batch(struct seq_file *seq)
 	for (; state->bucket <= udptable->mask; state->bucket++) {
 		struct udp_hslot *hslot2 = &udptable->hash2[state->bucket].hslot;
 
-		if (hlist_empty(&hslot2->head))
+		if (hlist_nulls_empty(&hslot2->head))
 			goto next_bucket;
 
 		spin_lock_bh(&hslot2->lock);
-		sk = hlist_entry_safe(hslot2->head.first, struct sock,
-				      __sk_common.skc_portaddr_node);
+		sk = hlist_nulls_entry_safe(hslot2->head.first, struct sock,
+					    __sk_common.skc_portaddr_node);
 		/* Resume from the first (in iteration order) unseen socket from
 		 * the last batch that still exists in resume_bucket. Most of
 		 * the time this will just be where the last iteration left off
@@ -3488,9 +3528,9 @@ static struct sock *bpf_iter_udp_batch(struct seq_file *seq)
 
 			/* Pick up where we left off. */
 			sk = iter->batch[iter->end_sk - 1].sk;
-			sk = hlist_entry_safe(sk->__sk_common.skc_portaddr_node.next,
-					      struct sock,
-					      __sk_common.skc_portaddr_node);
+			sk = hlist_nulls_entry_safe(sk->__sk_common.skc_portaddr_node.next,
+						    struct sock,
+						    __sk_common.skc_portaddr_node);
 			batch_sks = iter->end_sk;
 			goto fill_batch;
 		}
@@ -3717,12 +3757,12 @@ static void __init udp_table_init(struct udp_table *table, const char *name)
 
 	table->hash2 = (void *)(table->hash + (table->mask + 1));
 	for (i = 0; i <= table->mask; i++) {
-		INIT_HLIST_HEAD(&table->hash[i].head);
+		INIT_HLIST_NULLS_HEAD(&table->hash[i].head, i);
 		table->hash[i].count = 0;
 		spin_lock_init(&table->hash[i].lock);
 	}
 	for (i = 0; i <= table->mask; i++) {
-		INIT_HLIST_HEAD(&table->hash2[i].hslot.head);
+		INIT_HLIST_NULLS_HEAD(&table->hash2[i].hslot.head, i);
 		table->hash2[i].hslot.count = 0;
 		spin_lock_init(&table->hash2[i].hslot.lock);
 	}
@@ -3771,11 +3811,11 @@ static struct udp_table __net_init *udp_pernet_table_alloc(unsigned int hash_ent
 	udptable->log = ilog2(hash_entries);
 
 	for (i = 0; i < hash_entries; i++) {
-		INIT_HLIST_HEAD(&udptable->hash[i].head);
+		INIT_HLIST_NULLS_HEAD(&udptable->hash[i].head, i);
 		udptable->hash[i].count = 0;
 		spin_lock_init(&udptable->hash[i].lock);
 
-		INIT_HLIST_HEAD(&udptable->hash2[i].hslot.head);
+		INIT_HLIST_NULLS_HEAD(&udptable->hash2[i].hslot.head, i);
 		udptable->hash2[i].hslot.count = 0;
 		spin_lock_init(&udptable->hash2[i].hslot.lock);
 	}
diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
index f4b24e628cf8ded821d0c1887dd6ca5b83c4c8e2..5e0b4e07d9c1d80a727647273c9793ca11626827 100644
--- a/net/ipv4/udp_diag.c
+++ b/net/ipv4/udp_diag.c
@@ -100,15 +100,16 @@ static void udp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
 
 	for (slot = s_slot; slot <= table->mask; s_num = 0, slot++) {
 		struct udp_hslot *hslot = &table->hash[slot];
+		struct hlist_nulls_node *node;
 		struct sock *sk;
 
 		num = 0;
 
-		if (hlist_empty(&hslot->head))
+		if (hlist_nulls_empty(&hslot->head))
 			continue;
 
 		spin_lock_bh(&hslot->lock);
-		sk_for_each(sk, &hslot->head) {
+		sk_nulls_for_each(sk, node, &hslot->head) {
 			struct inet_sock *inet = inet_sk(sk);
 
 			if (!net_eq(sock_net(sk), net))
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 93478d1ad5769c6058567ff4deb433b0656b8129..f14132d4006718ce1e065986b734fb098bae4d9a 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -201,10 +201,14 @@ static struct sock *udp6_lib_lookup1(const struct net *net,
 {
 	unsigned int slot = udp_hashfn(net, hnum, udptable->mask);
 	struct udp_hslot *hslot = &udptable->hash[slot];
-	struct sock *sk, *result = NULL;
-	int score, badness = 0;
+	struct hlist_nulls_node *node;
+	struct sock *sk, *result;
+	int score, badness;
 
-	sk_for_each_rcu(sk, &hslot->head) {
+begin:
+	result = NULL;
+	badness = 0;
+	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
 		score = compute_score(sk, net,
 				      saddr, sport, daddr, hnum, dif, sdif);
 		if (score > badness) {
@@ -212,6 +216,13 @@ static struct sock *udp6_lib_lookup1(const struct net *net,
 			badness = score;
 		}
 	}
+	/*
+	 * if the nulls value we got at the end of this lookup is
+	 * not the expected one, we must restart lookup.
+	 * We probably met an item that was moved to another chain.
+	 */
+	if (unlikely(get_nulls_value(node) != slot))
+		goto begin;
 
 	return result;
 }
@@ -223,13 +234,16 @@ static struct sock *udp6_lib_lookup2(const struct net *net,
 		int dif, int sdif, struct udp_hslot *hslot2,
 		struct sk_buff *skb)
 {
+	unsigned int slot2 = UDP_HSLOT_MAIN(hslot2) - net->ipv4.udp_table->hash2;
+	struct hlist_nulls_node *node;
 	struct sock *sk, *result;
 	int score, badness;
 	bool need_rescore;
 
+begin:
 	result = NULL;
 	badness = -1;
-	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
+	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
 		need_rescore = false;
 rescore:
 		score = compute_score(need_rescore ? result : sk, net, saddr,
@@ -270,6 +284,13 @@ static struct sock *udp6_lib_lookup2(const struct net *net,
 			goto rescore;
 		}
 	}
+	/*
+	 * if the nulls value we got at the end of this lookup is
+	 * not the expected one, we must restart lookup.
+	 * We probably met an item that was moved to another chain.
+	 */
+	if (unlikely(get_nulls_value(node) != slot2))
+		goto begin;
 	return result;
 }
 
@@ -315,7 +336,7 @@ static struct sock *udp6_lib_lookup4(const struct net *net,
 	 * expected one, we must restart lookup. We probably met an item that
 	 * was moved to another chain due to rehash.
 	 */
-	if (get_nulls_value(node) != slot)
+	if (unlikely(get_nulls_value(node) != slot))
 		goto begin;
 
 	return NULL;
@@ -956,9 +977,9 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	unsigned int hash2, hash2_any, offset;
 	unsigned short hnum = ntohs(uh->dest);
 	struct sock *sk, *first = NULL;
+	struct hlist_nulls_node *node;
 	int sdif = inet6_sdif(skb);
 	int dif = inet6_iif(skb);
-	struct hlist_node *node;
 	struct udp_hslot *hslot;
 	struct sk_buff *nskb;
 	bool use_hash2;
@@ -968,7 +989,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	hash2 = 0;
 	hslot = udp_hashslot(udptable, net, hnum);
 	use_hash2 = hslot->count > 10;
-	offset = offsetof(typeof(*sk), sk_node);
+	offset = offsetof(typeof(*sk), sk_nulls_node);
 
 	if (use_hash2) {
 		hash2_any = ipv6_portaddr_hash(net, &in6addr_any, hnum) &
@@ -979,7 +1000,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
 	}
 
-	sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
+	sk_nulls_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
 		if (!__udp_v6_is_mcast_sock(net, sk, uh->dest, daddr,
 					    uh->source, saddr, dif, sdif,
 					    hnum))
@@ -1205,6 +1226,7 @@ static struct sock *__udp6_lib_demux_lookup(struct net *net,
 {
 	struct udp_table *udptable = net->ipv4.udp_table;
 	unsigned short hnum = ntohs(loc_port);
+	struct hlist_nulls_node *node;
 	struct udp_hslot *hslot2;
 	unsigned int hash2;
 	__portpair ports;
@@ -1214,7 +1236,7 @@ static struct sock *__udp6_lib_demux_lookup(struct net *net,
 	hslot2 = udp_hashslot2(udptable, hash2);
 	ports = INET_COMBINED_PORTS(rmt_port, hnum);
 
-	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
+	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
 		if (sk->sk_state == TCP_ESTABLISHED &&
 		    inet6_match(net, sk, rmt_addr, loc_addr, ports, dif, sdif))
 			return sk;
-- 
2.55.0.1082.g2b9226bbc0-goog

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

[David Laight]

On Tue, 22 Sep 2026 09:57:47 +0000
Eric Dumazet <edumazet@google.com> wrote:

> Commit ca065d0cf80f ("udp: no longer use SLAB_DESTROY_BY_RCU") switched
> UDP sockets to SOCK_RCU_FREE and converted udptable->hash and
> udptable->hash2 from hlist_nulls_head to hlist_head.
> 
> While SOCK_RCU_FREE guarantees that a socket is not freed before an RCU
> grace period elapses, a live UDP socket can still be unhashed or moved
> to a different hash bucket without an RCU grace period:
> 
> 1. udp_lib_rehash() moves sk->skc_portaddr_node from hslot2 to a
>    different nhslot2 when inet_rcv_saddr changes (for instance via
>    connect() or disconnect() on an wildcard-bound socket).
> 2. __udp_disconnect() calls udp_lib_unhash() when an implicitly bound
>    port was used, and a subsequent connect() or bind() can immediately
>    re-insert sk->sk_nulls_node and sk->skc_portaddr_node into different
>    hash and hash2 buckets.
> 
> Because hlist_add_head_rcu() overwrites node->next with the new bucket
> chain without waiting for an RCU grace period, a concurrent lockless
> reader in udp4_lib_lookup1/2() or udp6_lib_lookup1/2() traversing the
> old bucket can silently jump to the new bucket chain, terminate early,
> and miss a matching socket that was located later in the original bucket.
> 
> Restore hlist_nulls for udptable->hash and udptable->hash2 while keeping
> SOCK_RCU_FREE (so RCU lookups remain refcount-free), and restart the
> bucket traversal if get_nulls_value(node) does not match the expected
> bucket index.

Thanks - I've reported it before.
This is hit by a real application and causes unexpected ICMP port unreachable
messages on localhost.
I'm not sure why that application hits it, it does use a lot of udp sockets
but I don't believe any are 'connected'.
The work around is to ignore a single icmp error message.
I can't test the change - the fault could never be reliably reproduced and
the failing systems run on the Amazon cloud.
(And I've retired and don't work for that company any more.)

David

> 
> Fixes: ca065d0cf80f ("udp: no longer use SLAB_DESTROY_BY_RCU")
> Assisted-by: LLM
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> ---
>  include/linux/udp.h |  11 +++--
>  include/net/sock.h  |  23 +++++----
>  include/net/udp.h   |   5 +-
>  net/ipv4/udp.c      | 112 ++++++++++++++++++++++++++++++--------------
>  net/ipv4/udp_diag.c |   5 +-
>  net/ipv6/udp.c      |  40 ++++++++++++----
>  6 files changed, 131 insertions(+), 65 deletions(-)
> 
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index 998906ec3b32add4652bde3ee9c28e8b2b98ce51..a1990350fd22f3418724c0e06d4b7405779f7c2b 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -243,14 +243,15 @@ static inline void udp_allow_gso(struct sock *sk)
>  	udp_set_bit(ACCEPT_FRAGLIST, sk);
>  }
>  
> -#define udp_portaddr_for_each_entry(__sk, list) \
> -	hlist_for_each_entry(__sk, list, __sk_common.skc_portaddr_node)
> +#define udp_portaddr_for_each_entry(__sk, node, list) \
> +	hlist_nulls_for_each_entry(__sk, node, list, __sk_common.skc_portaddr_node)
>  
>  #define udp_portaddr_for_each_entry_from(__sk) \
> -	hlist_for_each_entry_from(__sk, __sk_common.skc_portaddr_node)
> +	for (; __sk; __sk = hlist_nulls_entry_safe((__sk)->__sk_common.skc_portaddr_node.next, \
> +						   typeof(*(__sk)), __sk_common.skc_portaddr_node))
>  
> -#define udp_portaddr_for_each_entry_rcu(__sk, list) \
> -	hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
> +#define udp_portaddr_for_each_entry_rcu(__sk, node, list) \
> +	hlist_nulls_for_each_entry_rcu(__sk, node, list, __sk_common.skc_portaddr_node)
>  
>  #if !IS_ENABLED(CONFIG_BASE_SMALL)
>  #define udp_lrpa_for_each_entry_rcu(__up, node, list) \
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 60ea55dc18854a9759f5df618cc8c904d2323e95..78626aba52dca6ab9fcae60f5bc277089f143633 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -184,7 +184,7 @@ struct sock_common {
>  	int			skc_bound_dev_if;
>  	union {
>  		struct hlist_node	skc_bind_node;
> -		struct hlist_node	skc_portaddr_node;
> +		struct hlist_nulls_node	skc_portaddr_node;
>  	};
>  	struct proto		*skc_prot;
>  	possible_net_t		skc_net;
> @@ -930,7 +930,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)
> @@ -965,18 +969,19 @@ static inline void sk_add_bind_node(struct sock *sk,
>  	hlist_for_each_entry_safe(__sk, tmp, list, sk_bind_node)
>  
>  /**
> - * sk_for_each_entry_offset_rcu - iterate over a list at a given struct offset
> + * sk_nulls_for_each_entry_offset_rcu - iterate over a list at a given struct offset
>   * @tpos:	the type * to use as a loop cursor.
> - * @pos:	the &struct hlist_node to use as a loop cursor.
> + * @pos:	the &struct hlist_nulls_node to use as a loop cursor.
>   * @head:	the head for your list.
> - * @offset:	offset of hlist_node within the struct.
> + * @offset:	offset of hlist_nulls_node within the struct.
>   *
>   */
> -#define sk_for_each_entry_offset_rcu(tpos, pos, head, offset)		       \
> -	for (pos = rcu_dereference(hlist_first_rcu(head));		       \
> -	     pos != NULL &&						       \
> +#define sk_nulls_for_each_entry_offset_rcu(tpos, pos, head, offset)	       \
> +	for (({ barrier(); }),						       \
> +	     pos = rcu_dereference_raw(hlist_nulls_first_rcu(head));	       \
> +	     (!is_a_nulls(pos)) &&					       \
>  		({ tpos = (typeof(*tpos) *)((void *)pos - offset); 1;});       \
> -	     pos = rcu_dereference(hlist_next_rcu(pos)))
> +	     pos = rcu_dereference_raw(hlist_nulls_next_rcu(pos)))
>  
>  static inline struct user_namespace *sk_user_ns(const struct sock *sk)
>  {
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 1fee17274745f0b52837b7eb2498dbc423a450fd..1bba33479341e07f2dde9acc19b0b09cafa3ef29 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -56,10 +56,7 @@ struct udp_skb_cb {
>   */
>  struct udp_hslot {
>  	union {
> -		struct hlist_head	head;
> -		/* hash4 uses hlist_nulls to avoid moving wrongly onto another
> -		 * hlist, because rehash() can happen with lookup().
> -		 */
> +		struct hlist_nulls_head	head;
>  		struct hlist_nulls_head	nulls_head;
>  	};
>  	int			count;
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index b090bd1f59e86cd22edd9622b17b5679346ea24d..309220bf2fba9e7ccf48676d312924a6881bc34f 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -136,10 +136,11 @@ static int udp_lib_lport_inuse(struct net *net, __u16 num,
>  			       unsigned long *bitmap,
>  			       struct sock *sk, unsigned int log)
>  {
> +	struct hlist_nulls_node *node;
>  	kuid_t uid = sk_uid(sk);
>  	struct sock *sk2;
>  
> -	sk_for_each(sk2, &hslot->head) {
> +	sk_nulls_for_each(sk2, node, &hslot->head) {
>  		if (net_eq(sock_net(sk2), net) &&
>  		    sk2 != sk &&
>  		    (bitmap || udp_sk(sk2)->udp_port_hash == num) &&
> @@ -171,12 +172,13 @@ static int udp_lib_lport_inuse2(struct net *net, __u16 num,
>  				struct udp_hslot *hslot2,
>  				struct sock *sk)
>  {
> +	struct hlist_nulls_node *node;
>  	kuid_t uid = sk_uid(sk);
>  	struct sock *sk2;
>  	int res = 0;
>  
>  	spin_lock(&hslot2->lock);
> -	udp_portaddr_for_each_entry(sk2, &hslot2->head) {
> +	udp_portaddr_for_each_entry(sk2, node, &hslot2->head) {
>  		if (net_eq(sock_net(sk2), net) &&
>  		    sk2 != sk &&
>  		    (udp_sk(sk2)->udp_port_hash == num) &&
> @@ -201,10 +203,11 @@ static int udp_lib_lport_inuse2(struct net *net, __u16 num,
>  static int udp_reuseport_add_sock(struct sock *sk, struct udp_hslot *hslot)
>  {
>  	struct net *net = sock_net(sk);
> +	struct hlist_nulls_node *node;
>  	kuid_t uid = sk_uid(sk);
>  	struct sock *sk2;
>  
> -	sk_for_each(sk2, &hslot->head) {
> +	sk_nulls_for_each(sk2, node, &hslot->head) {
>  		if (net_eq(sock_net(sk2), net) &&
>  		    sk2 != sk &&
>  		    sk2->sk_family == sk->sk_family &&
> @@ -323,7 +326,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);
>  
> @@ -331,11 +334,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);
>  	}
> @@ -440,10 +443,14 @@ static struct sock *udp4_lib_lookup1(const struct net *net,
>  {
>  	unsigned int slot = udp_hashfn(net, hnum, udptable->mask);
>  	struct udp_hslot *hslot = &udptable->hash[slot];
> -	struct sock *sk, *result = NULL;
> -	int score, badness = 0;
> +	struct hlist_nulls_node *node;
> +	struct sock *sk, *result;
> +	int score, badness;
>  
> -	sk_for_each_rcu(sk, &hslot->head) {
> +begin:
> +	result = NULL;
> +	badness = 0;
> +	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
>  		score = compute_score(sk, net,
>  				      saddr, sport, daddr, hnum, dif, sdif);
>  		if (score > badness) {
> @@ -451,6 +458,13 @@ static struct sock *udp4_lib_lookup1(const struct net *net,
>  			badness = score;
>  		}
>  	}
> +	/*
> +	 * if the nulls value we got at the end of this lookup is
> +	 * not the expected one, we must restart lookup.
> +	 * We probably met an item that was moved to another chain.
> +	 */
> +	if (unlikely(get_nulls_value(node) != slot))
> +		goto begin;
>  
>  	return result;
>  }
> @@ -463,13 +477,16 @@ static struct sock *udp4_lib_lookup2(const struct net *net,
>  				     struct udp_hslot *hslot2,
>  				     struct sk_buff *skb)
>  {
> +	unsigned int slot2 = UDP_HSLOT_MAIN(hslot2) - net->ipv4.udp_table->hash2;
> +	struct hlist_nulls_node *node;
>  	struct sock *sk, *result;
>  	int score, badness;
>  	bool need_rescore;
>  
> +begin:
>  	result = NULL;
>  	badness = 0;
> -	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> +	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
>  		need_rescore = false;
>  rescore:
>  		score = compute_score(need_rescore ? result : sk, net, saddr,
> @@ -510,6 +527,13 @@ static struct sock *udp4_lib_lookup2(const struct net *net,
>  			goto rescore;
>  		}
>  	}
> +	/*
> +	 * if the nulls value we got at the end of this lookup is
> +	 * not the expected one, we must restart lookup.
> +	 * We probably met an item that was moved to another chain.
> +	 */
> +	if (unlikely(get_nulls_value(node) != slot2))
> +		goto begin;
>  	return result;
>  }
>  
> @@ -562,7 +586,7 @@ static struct sock *udp4_lib_lookup4(const struct net *net,
>  	 * expected one, we must restart lookup. We probably met an item that
>  	 * was moved to another chain due to rehash.
>  	 */
> -	if (get_nulls_value(node) != slot)
> +	if (unlikely(get_nulls_value(node) != slot))
>  		goto begin;
>  
>  	return NULL;
> @@ -2251,13 +2275,13 @@ void udp_lib_unhash(struct sock *sk)
>  		spin_lock_bh(&hslot->lock);
>  		if (rcu_access_pointer(sk->sk_reuseport_cb))
>  			reuseport_detach_sock(sk);
> -		if (sk_del_node_init_rcu(sk)) {
> +		if (sk_nulls_del_node_init_rcu(sk)) {
>  			hslot->count--;
>  			inet_sk(sk)->inet_num = 0;
>  			sock_prot_inuse_add(net, sk->sk_prot, -1);
>  
>  			spin_lock(&hslot2->lock);
> -			hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> +			hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
>  			hslot2->count--;
>  			spin_unlock(&hslot2->lock);
>  
> @@ -2291,13 +2315,18 @@ void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4)
>  
>  			if (hslot2 != nhslot2) {
>  				spin_lock(&hslot2->lock);
> -				hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> +				hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
>  				hslot2->count--;
>  				spin_unlock(&hslot2->lock);
>  
>  				spin_lock(&nhslot2->lock);
> -				hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> -							 &nhslot2->head);
> +				if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
> +				    sk->sk_family == AF_INET6)
> +					hlist_nulls_add_tail_rcu(&udp_sk(sk)->udp_portaddr_node,
> +								 &nhslot2->head);
> +				else
> +					hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> +								 &nhslot2->head);
>  				nhslot2->count++;
>  				spin_unlock(&nhslot2->lock);
>  			}
> @@ -2513,9 +2542,9 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	unsigned int hash2, hash2_any, offset;
>  	unsigned short hnum = ntohs(uh->dest);
>  	struct sock *sk, *first = NULL;
> +	struct hlist_nulls_node *node;
>  	int dif = skb->dev->ifindex;
>  	int sdif = inet_sdif(skb);
> -	struct hlist_node *node;
>  	struct udp_hslot *hslot;
>  	struct sk_buff *nskb;
>  	bool use_hash2;
> @@ -2525,7 +2554,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	hash2 = 0;
>  	hslot = udp_hashslot(udptable, net, hnum);
>  	use_hash2 = hslot->count > 10;
> -	offset = offsetof(typeof(*sk), sk_node);
> +	offset = offsetof(typeof(*sk), sk_nulls_node);
>  
>  	if (use_hash2) {
>  		hash2_any = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
> @@ -2536,7 +2565,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
>  	}
>  
> -	sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
> +	sk_nulls_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
>  		if (!__udp_is_mcast_sock(net, sk, uh->dest, daddr,
>  					 uh->source, saddr, dif, sdif, hnum))
>  			continue;
> @@ -2749,6 +2778,7 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
>  {
>  	struct udp_table *udptable = net->ipv4.udp_table;
>  	unsigned short hnum = ntohs(loc_port);
> +	struct hlist_nulls_node *node;
>  	struct sock *sk, *result;
>  	struct udp_hslot *hslot;
>  	unsigned int slot;
> @@ -2760,8 +2790,9 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
>  	if (hslot->count > 10)
>  		return NULL;
>  
> +begin:
>  	result = NULL;
> -	sk_for_each_rcu(sk, &hslot->head) {
> +	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
>  		if (__udp_is_mcast_sock(net, sk, loc_port, loc_addr,
>  					rmt_port, rmt_addr, dif, sdif, hnum)) {
>  			if (result)
> @@ -2769,6 +2800,13 @@ static struct sock *__udp4_lib_mcast_demux_lookup(struct net *net,
>  			result = sk;
>  		}
>  	}
> +	/*
> +	 * if the nulls value we got at the end of this lookup is
> +	 * not the expected one, we must restart lookup.
> +	 * We probably met an item that was moved to another chain.
> +	 */
> +	if (unlikely(get_nulls_value(node) != slot))
> +		goto begin;
>  
>  	return result;
>  }
> @@ -2785,6 +2823,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
>  	struct udp_table *udptable = net->ipv4.udp_table;
>  	INET_ADDR_COOKIE(acookie, rmt_addr, loc_addr);
>  	unsigned short hnum = ntohs(loc_port);
> +	struct hlist_nulls_node *node;
>  	struct udp_hslot *hslot2;
>  	unsigned int hash2;
>  	__portpair ports;
> @@ -2794,7 +2833,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
>  	hslot2 = udp_hashslot2(udptable, hash2);
>  	ports = INET_COMBINED_PORTS(rmt_port, hnum);
>  
> -	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> +	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
>  		if (inet_match(net, sk, acookie, ports, dif, sdif))
>  			return sk;
>  		/* Only check first socket in chain */
> @@ -3228,6 +3267,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start)
>  {
>  	struct udp_iter_state *state = seq->private;
>  	struct net *net = seq_file_net(seq);
> +	struct hlist_nulls_node *node;
>  	struct udp_table *udptable;
>  	struct sock *sk;
>  
> @@ -3237,11 +3277,11 @@ static struct sock *udp_get_first(struct seq_file *seq, int start)
>  	     ++state->bucket) {
>  		struct udp_hslot *hslot = &udptable->hash[state->bucket];
>  
> -		if (hlist_empty(&hslot->head))
> +		if (hlist_nulls_empty(&hslot->head))
>  			continue;
>  
>  		spin_lock_bh(&hslot->lock);
> -		sk_for_each(sk, &hslot->head) {
> +		sk_nulls_for_each(sk, node, &hslot->head) {
>  			if (seq_sk_match(seq, sk))
>  				goto found;
>  		}
> @@ -3259,7 +3299,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk)
>  	struct udp_table *udptable;
>  
>  	do {
> -		sk = sk_next(sk);
> +		sk = sk_nulls_next(sk);
>  	} while (sk && !seq_sk_match(seq, sk));
>  
>  	if (!sk) {
> @@ -3431,12 +3471,12 @@ static struct sock *bpf_iter_udp_batch(struct seq_file *seq)
>  	for (; state->bucket <= udptable->mask; state->bucket++) {
>  		struct udp_hslot *hslot2 = &udptable->hash2[state->bucket].hslot;
>  
> -		if (hlist_empty(&hslot2->head))
> +		if (hlist_nulls_empty(&hslot2->head))
>  			goto next_bucket;
>  
>  		spin_lock_bh(&hslot2->lock);
> -		sk = hlist_entry_safe(hslot2->head.first, struct sock,
> -				      __sk_common.skc_portaddr_node);
> +		sk = hlist_nulls_entry_safe(hslot2->head.first, struct sock,
> +					    __sk_common.skc_portaddr_node);
>  		/* Resume from the first (in iteration order) unseen socket from
>  		 * the last batch that still exists in resume_bucket. Most of
>  		 * the time this will just be where the last iteration left off
> @@ -3488,9 +3528,9 @@ static struct sock *bpf_iter_udp_batch(struct seq_file *seq)
>  
>  			/* Pick up where we left off. */
>  			sk = iter->batch[iter->end_sk - 1].sk;
> -			sk = hlist_entry_safe(sk->__sk_common.skc_portaddr_node.next,
> -					      struct sock,
> -					      __sk_common.skc_portaddr_node);
> +			sk = hlist_nulls_entry_safe(sk->__sk_common.skc_portaddr_node.next,
> +						    struct sock,
> +						    __sk_common.skc_portaddr_node);
>  			batch_sks = iter->end_sk;
>  			goto fill_batch;
>  		}
> @@ -3717,12 +3757,12 @@ static void __init udp_table_init(struct udp_table *table, const char *name)
>  
>  	table->hash2 = (void *)(table->hash + (table->mask + 1));
>  	for (i = 0; i <= table->mask; i++) {
> -		INIT_HLIST_HEAD(&table->hash[i].head);
> +		INIT_HLIST_NULLS_HEAD(&table->hash[i].head, i);
>  		table->hash[i].count = 0;
>  		spin_lock_init(&table->hash[i].lock);
>  	}
>  	for (i = 0; i <= table->mask; i++) {
> -		INIT_HLIST_HEAD(&table->hash2[i].hslot.head);
> +		INIT_HLIST_NULLS_HEAD(&table->hash2[i].hslot.head, i);
>  		table->hash2[i].hslot.count = 0;
>  		spin_lock_init(&table->hash2[i].hslot.lock);
>  	}
> @@ -3771,11 +3811,11 @@ static struct udp_table __net_init *udp_pernet_table_alloc(unsigned int hash_ent
>  	udptable->log = ilog2(hash_entries);
>  
>  	for (i = 0; i < hash_entries; i++) {
> -		INIT_HLIST_HEAD(&udptable->hash[i].head);
> +		INIT_HLIST_NULLS_HEAD(&udptable->hash[i].head, i);
>  		udptable->hash[i].count = 0;
>  		spin_lock_init(&udptable->hash[i].lock);
>  
> -		INIT_HLIST_HEAD(&udptable->hash2[i].hslot.head);
> +		INIT_HLIST_NULLS_HEAD(&udptable->hash2[i].hslot.head, i);
>  		udptable->hash2[i].hslot.count = 0;
>  		spin_lock_init(&udptable->hash2[i].hslot.lock);
>  	}
> diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
> index f4b24e628cf8ded821d0c1887dd6ca5b83c4c8e2..5e0b4e07d9c1d80a727647273c9793ca11626827 100644
> --- a/net/ipv4/udp_diag.c
> +++ b/net/ipv4/udp_diag.c
> @@ -100,15 +100,16 @@ static void udp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
>  
>  	for (slot = s_slot; slot <= table->mask; s_num = 0, slot++) {
>  		struct udp_hslot *hslot = &table->hash[slot];
> +		struct hlist_nulls_node *node;
>  		struct sock *sk;
>  
>  		num = 0;
>  
> -		if (hlist_empty(&hslot->head))
> +		if (hlist_nulls_empty(&hslot->head))
>  			continue;
>  
>  		spin_lock_bh(&hslot->lock);
> -		sk_for_each(sk, &hslot->head) {
> +		sk_nulls_for_each(sk, node, &hslot->head) {
>  			struct inet_sock *inet = inet_sk(sk);
>  
>  			if (!net_eq(sock_net(sk), net))
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 93478d1ad5769c6058567ff4deb433b0656b8129..f14132d4006718ce1e065986b734fb098bae4d9a 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -201,10 +201,14 @@ static struct sock *udp6_lib_lookup1(const struct net *net,
>  {
>  	unsigned int slot = udp_hashfn(net, hnum, udptable->mask);
>  	struct udp_hslot *hslot = &udptable->hash[slot];
> -	struct sock *sk, *result = NULL;
> -	int score, badness = 0;
> +	struct hlist_nulls_node *node;
> +	struct sock *sk, *result;
> +	int score, badness;
>  
> -	sk_for_each_rcu(sk, &hslot->head) {
> +begin:
> +	result = NULL;
> +	badness = 0;
> +	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
>  		score = compute_score(sk, net,
>  				      saddr, sport, daddr, hnum, dif, sdif);
>  		if (score > badness) {
> @@ -212,6 +216,13 @@ static struct sock *udp6_lib_lookup1(const struct net *net,
>  			badness = score;
>  		}
>  	}
> +	/*
> +	 * if the nulls value we got at the end of this lookup is
> +	 * not the expected one, we must restart lookup.
> +	 * We probably met an item that was moved to another chain.
> +	 */
> +	if (unlikely(get_nulls_value(node) != slot))
> +		goto begin;
>  
>  	return result;
>  }
> @@ -223,13 +234,16 @@ static struct sock *udp6_lib_lookup2(const struct net *net,
>  		int dif, int sdif, struct udp_hslot *hslot2,
>  		struct sk_buff *skb)
>  {
> +	unsigned int slot2 = UDP_HSLOT_MAIN(hslot2) - net->ipv4.udp_table->hash2;
> +	struct hlist_nulls_node *node;
>  	struct sock *sk, *result;
>  	int score, badness;
>  	bool need_rescore;
>  
> +begin:
>  	result = NULL;
>  	badness = -1;
> -	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> +	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
>  		need_rescore = false;
>  rescore:
>  		score = compute_score(need_rescore ? result : sk, net, saddr,
> @@ -270,6 +284,13 @@ static struct sock *udp6_lib_lookup2(const struct net *net,
>  			goto rescore;
>  		}
>  	}
> +	/*
> +	 * if the nulls value we got at the end of this lookup is
> +	 * not the expected one, we must restart lookup.
> +	 * We probably met an item that was moved to another chain.
> +	 */
> +	if (unlikely(get_nulls_value(node) != slot2))
> +		goto begin;
>  	return result;
>  }
>  
> @@ -315,7 +336,7 @@ static struct sock *udp6_lib_lookup4(const struct net *net,
>  	 * expected one, we must restart lookup. We probably met an item that
>  	 * was moved to another chain due to rehash.
>  	 */
> -	if (get_nulls_value(node) != slot)
> +	if (unlikely(get_nulls_value(node) != slot))
>  		goto begin;
>  
>  	return NULL;
> @@ -956,9 +977,9 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	unsigned int hash2, hash2_any, offset;
>  	unsigned short hnum = ntohs(uh->dest);
>  	struct sock *sk, *first = NULL;
> +	struct hlist_nulls_node *node;
>  	int sdif = inet6_sdif(skb);
>  	int dif = inet6_iif(skb);
> -	struct hlist_node *node;
>  	struct udp_hslot *hslot;
>  	struct sk_buff *nskb;
>  	bool use_hash2;
> @@ -968,7 +989,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	hash2 = 0;
>  	hslot = udp_hashslot(udptable, net, hnum);
>  	use_hash2 = hslot->count > 10;
> -	offset = offsetof(typeof(*sk), sk_node);
> +	offset = offsetof(typeof(*sk), sk_nulls_node);
>  
>  	if (use_hash2) {
>  		hash2_any = ipv6_portaddr_hash(net, &in6addr_any, hnum) &
> @@ -979,7 +1000,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
>  	}
>  
> -	sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
> +	sk_nulls_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
>  		if (!__udp_v6_is_mcast_sock(net, sk, uh->dest, daddr,
>  					    uh->source, saddr, dif, sdif,
>  					    hnum))
> @@ -1205,6 +1226,7 @@ static struct sock *__udp6_lib_demux_lookup(struct net *net,
>  {
>  	struct udp_table *udptable = net->ipv4.udp_table;
>  	unsigned short hnum = ntohs(loc_port);
> +	struct hlist_nulls_node *node;
>  	struct udp_hslot *hslot2;
>  	unsigned int hash2;
>  	__portpair ports;
> @@ -1214,7 +1236,7 @@ static struct sock *__udp6_lib_demux_lookup(struct net *net,
>  	hslot2 = udp_hashslot2(udptable, hash2);
>  	ports = INET_COMBINED_PORTS(rmt_port, hnum);
>  
> -	udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> +	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
>  		if (sk->sk_state == TCP_ESTABLISHED &&
>  		    inet6_match(net, sk, rmt_addr, loc_addr, ports, dif, sdif))
>  			return sk;

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

[netdev-bot+sashiko]

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

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

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 patch converts every UDP primary/secondary hash bucket to
  hlist_nulls and makes the nulls-value restart mandatory for lockless…
- [Medium] udp_lib_rehash() (net/ipv4/udp.c) gains an IPv6+SO_REUSEPORT
  tail-insert branch that did not exist before: the pre-image is an…
- [Medium] The newly added tail insertion defeats the very nulls-value
  check this patch adds, because a node can leave a bucket and come back…
- [Low] udp_lib_rehash() now calls hlist_nulls_add_tail_rcu() for IPv6
  SO_REUSEPORT sockets while holding spin_lock_bh(&hslot->lock) with…
- [Low] The patch renames sk_for_each_entry_offset_rcu() to
  sk_nulls_for_each_entry_offset_rcu() in include/net/sock.h and updates…

--

udp: restore hlist_nulls for primary and secondary hash tables

This converts udptable->hash and udptable->hash2 back to hlist_nulls while
keeping SOCK_RCU_FREE, and adds a get_nulls_value() based restart to the
lockless lookup helpers so a reader that follows a relocated socket into
another bucket retries instead of terminating early.

A few questions about the conversion below.

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 60ea55dc18854..78626aba52dca 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h

[ ... ]

> @@ -965,18 +969,19 @@ static inline void sk_add_bind_node(struct sock *sk,
>  	hlist_for_each_entry_safe(__sk, tmp, list, sk_bind_node)
>  
>  /**
> - * sk_for_each_entry_offset_rcu - iterate over a list at a given struct offset
> + * sk_nulls_for_each_entry_offset_rcu - iterate over a list at a given struct offset
>   * @tpos:	the type * to use as a loop cursor.
> - * @pos:	the &struct hlist_node to use as a loop cursor.
> + * @pos:	the &struct hlist_nulls_node to use as a loop cursor.
>   * @head:	the head for your list.
> - * @offset:	offset of hlist_node within the struct.
> + * @offset:	offset of hlist_nulls_node within the struct.
>   *
>   */
> -#define sk_for_each_entry_offset_rcu(tpos, pos, head, offset)		       \
> -	for (pos = rcu_dereference(hlist_first_rcu(head));		       \
> -	     pos != NULL &&						       \
> +#define sk_nulls_for_each_entry_offset_rcu(tpos, pos, head, offset)	       \
> +	for (({ barrier(); }),						       \
> +	     pos = rcu_dereference_raw(hlist_nulls_first_rcu(head));	       \
> +	     (!is_a_nulls(pos)) &&					       \
>  		({ tpos = (typeof(*tpos) *)((void *)pos - offset); 1;});       \
> -	     pos = rcu_dereference(hlist_next_rcu(pos)))
> +	     pos = rcu_dereference_raw(hlist_nulls_next_rcu(pos)))

[Severity: Low]
This isn't a bug, but .clang-format still lists only the old name in
ForEachMacros:

  - 'sk_for_each_entry_offset_rcu'

After this rename that entry matches no macro in the tree, and
sk_nulls_for_each_entry_offset_rcu is not listed at all, so clang-format
formats the two converted loop bodies in net/ipv4/udp.c and net/ipv6/udp.c
as call arguments rather than loop bodies.  Should the entry be renamed
along with the macro?

> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index b090bd1f59e86..309220bf2fba9 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c

[ ... ]

> @@ -440,10 +443,14 @@ static struct sock *udp4_lib_lookup1(const struct net *net,
>  {
>  	unsigned int slot = udp_hashfn(net, hnum, udptable->mask);
>  	struct udp_hslot *hslot = &udptable->hash[slot];
> -	struct sock *sk, *result = NULL;
> -	int score, badness = 0;
> +	struct hlist_nulls_node *node;
> +	struct sock *sk, *result;
> +	int score, badness;
>  
> -	sk_for_each_rcu(sk, &hslot->head) {
> +begin:
> +	result = NULL;
> +	badness = 0;
> +	sk_nulls_for_each_rcu(sk, node, &hslot->head) {
>  		score = compute_score(sk, net,
>  				      saddr, sport, daddr, hnum, dif, sdif);
>  		if (score > badness) {
> @@ -451,6 +458,13 @@ static struct sock *udp4_lib_lookup1(const struct net *net,
>  			badness = score;
>  		}
>  	}
> +	/*
> +	 * if the nulls value we got at the end of this lookup is
> +	 * not the expected one, we must restart lookup.
> +	 * We probably met an item that was moved to another chain.
> +	 */
> +	if (unlikely(get_nulls_value(node) != slot))
> +		goto begin;
>  
>  	return result;
>  }

[ ... ]

> @@ -2291,13 +2315,18 @@ void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4)
>  
>  			if (hslot2 != nhslot2) {
>  				spin_lock(&hslot2->lock);
> -				hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> +				hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
>  				hslot2->count--;
>  				spin_unlock(&hslot2->lock);
>  
>  				spin_lock(&nhslot2->lock);
> -				hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> -							 &nhslot2->head);
> +				if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
> +				    sk->sk_family == AF_INET6)
> +					hlist_nulls_add_tail_rcu(&udp_sk(sk)->udp_portaddr_node,
> +								 &nhslot2->head);
> +				else
> +					hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_portaddr_node,
> +								 &nhslot2->head);
>  				nhslot2->count++;
>  				spin_unlock(&nhslot2->lock);
>  			}

[Severity: Medium]
The pre-image here was an unconditional head insert.  Is the new IPv6 plus
SO_REUSEPORT tail insert part of the hlist_nulls conversion?  The changelog
only describes restoring hlist_nulls and adding the traversal restarts, so
this chain ordering change is not mentioned.

reuseport_detach_sock() clears only sk->sk_reuseport_cb, not
sk->sk_reuseport, so the new branch is taken on every hash2-changing rehash
of an IPv6 reuseport socket.

With CONFIG_BASE_SMALL=y (udp6_lib_lookup4() is a stub and udp_has_hash4()
returns false) the new ordering looks like it can misdeliver persistently:

  socket A: IPv6, SO_REUSEPORT, bound to [X]:P, left unconnected
  socket B: IPv6, SO_REUSEPORT, bound to [::]:P, then connected so that
            its selected source address is X

udp_v6_rehash() -> udp_lib_rehash() now appends B behind A in hash2 bucket
hash2(X,P).  For a packet from B's peer, udp6_lib_lookup2() scores A first
(compute_score() returns 0 for A, which beats the initial badness of -1), A
is not TCP_ESTABLISHED so inet6_lookup_reuseport() returns A from its
one-socket group, reuseport_has_conns(A) is false and A is returned before B
is ever scored.  __udp6_lib_lookup() then takes

	/* Got non-wildcard socket or error on first lookup */
	if (result)
		goto done;

and never falls back to udp6_lib_lookup1().  Is B's connected traffic then
delivered to A for as long as this setup exists?

In default builds udpv6_connect() -> udp6_hash4() places B in hash4 and
udp6_lib_lookup4() resolves it before udp6_lib_lookup2() runs, which hides
this.

If this is repairing a separate soreuseport ordering problem, would it be
clearer as its own patch with its own Fixes: tag?  Otherwise, should the
rehash site keep hlist_nulls_add_head_rcu() as before?

[Severity: Medium]
Can this tail insertion defeat the nulls-value check added elsewhere in the
patch?  hlist_nulls_add_tail_rcu() does:

	for (i = h->first; !is_a_nulls(i); i = i->next)
		last = i;

	if (last) {
		WRITE_ONCE(n->next, last->next);

so the re-inserted node's ->next becomes the same bucket's nulls marker,
while __hlist_nulls_del() deliberately leaves the removed node's ->next
pointing into the chain.

Consider hash2 bucket A holding [S, T], where S is an IPv6 reuseport socket
and T matches an incoming packet.  A reader in udp6_lib_lookup2() or
udp4_lib_lookup2() has consumed S but has not yet loaded S->next.  Another
CPU runs connect():

udpv6_connect() -> udp_v6_rehash() -> udp_lib_rehash()   /* A -> B */

and then disconnect(), which for a wildcard-bound socket holding
SOCK_BINDPORT_LOCK without SOCK_BINDADDR_LOCK calls sk->sk_prot->rehash()
from __udp_disconnect():

	if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) {
		inet_reset_saddr(sk);
		if (sk->sk_prot->rehash &&
		    (sk->sk_userlocks & SOCK_BINDPORT_LOCK))
			sk->sk_prot->rehash(sk);
	}

						 /* B -> A, tail insert */

S now sits after T and S->next is the nulls marker of bucket A.  The parked
reader loads that marker, never visits T, and

	if (unlikely(get_nulls_value(node) != slot2))
		goto begin;

does not fire because the bucket index matches.  If the truncated lookup
still returns a lower-scoring socket, __udp6_lib_lookup() takes
'if (result) goto done' and never runs the udp6_lib_lookup1() fallback.

Is comparing bucket identity alone enough once a node can leave a bucket and
be appended back to the same bucket?

[Severity: Low]
This path now does an O(N) chain walk with softirqs disabled.
hlist_nulls_add_tail_rcu() walks the whole destination chain:

	for (i = h->first; !is_a_nulls(i); i = i->next)
		last = i;

and here it runs under spin_lock_bh(&hslot->lock) with nhslot2->lock nested,
where N is the number of sockets in the destination hash2 bucket (a whole
reuseport group sharing address and port hashes into one bucket).  The
previous code used the O(1) hlist_add_head_rcu().

An unprivileged task can repeat this without creating sockets, by looping
connect()/disconnect() on a wildcard-bound IPv6 reuseport socket, since each
pair changes hash2 and performs the walk twice.  The same O(N) walk already
exists in the bind path through udp_lib_get_port(), so the magnitude is not
new, only cheaper to repeat.  Is holding the primary bucket lock with BH
disabled across that walk acceptable on the rehash path?

[ ... ]

> @@ -2525,7 +2554,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	hash2 = 0;
>  	hslot = udp_hashslot(udptable, net, hnum);
>  	use_hash2 = hslot->count > 10;
> -	offset = offsetof(typeof(*sk), sk_node);
> +	offset = offsetof(typeof(*sk), sk_nulls_node);
>  
>  	if (use_hash2) {
>  		hash2_any = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
> @@ -2536,7 +2565,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
>  	}
>  
> -	sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
> +	sk_nulls_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
>  		if (!__udp_is_mcast_sock(net, sk, uh->dest, daddr,
>  					 uh->source, saddr, dif, sdif, hnum))
>  			continue;

[Severity: High]
Should this walk also validate the terminating nulls value?  The lookup
helpers converted in this same patch restart with

	if (unlikely(get_nulls_value(node) != slot))
		goto begin;

but sk_nulls_for_each_entry_offset_rcu() only stops on is_a_nulls(pos), and
neither __udp4_lib_mcast_deliver() nor __udp6_lib_mcast_deliver() looks at
node afterwards.

Both expected indexes are already in scope: udp_hashslot() above derives the
primary index from udp_hashfn(net, hnum, udptable->mask), and the hash2 walk
uses the local hash2.

This runs from udp_rcv() -> __udp4_lib_rcv() -> __udp4_lib_mcast_deliver()
under rcu_read_lock() only, with no hslot->lock.  If a concurrent
udp_lib_rehash() (connect() or disconnect() on a wildcard-bound socket), or
udp_lib_unhash() followed by bind()/connect() -> udp_lib_get_port() ->
sk_nulls_add_node_rcu(), moves a socket out of the bucket being walked, can
this loop follow the moved node's ->next into the other chain, stop on that
chain's nulls marker, and skip the remaining multicast listeners of the
original bucket?

And when the chain it lands in is the hash2_any chain, can those sockets be
visited a second time by the

	if (use_hash2 && hash2 != hash2_any) {
		hash2 = hash2_any;
		goto start_lookup;
	}

pass, delivering the datagram twice?

One complication: by the time the jump could be detected, skbs have already
been cloned and queued, so a plain 'goto begin' restart like the one in
udp4_lib_lookup1() does not seem usable here.

For completeness, __udp4_lib_demux_lookup() and __udp6_lib_demux_lookup()
were also converted to nulls iteration without the check.  There only the
first chain entry is inspected and inet_match()/inet6_match() revalidate the
4-tuple, so the effect looks benign, but the new contract is applied
inconsistently across readers of the same buckets.

[ ... ]

> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 93478d1ad5769..f14132d400671 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c

[ ... ]

> @@ -979,7 +1000,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
>  	}
>  
> -	sk_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
> +	sk_nulls_for_each_entry_offset_rcu(sk, node, &hslot->head, offset) {
>  		if (!__udp_v6_is_mcast_sock(net, sk, uh->dest, daddr,
>  					    uh->source, saddr, dif, sdif,
>  					    hnum))

[Severity: High]
Same question for the IPv6 side: this walk covers either the primary bucket
(offset sk_nulls_node) or udptable->hash2[hash2].hslot (offset
skc_portaddr_node), both of which udp_lib_rehash() and udp_lib_unhash() can
change without an RCU grace period, yet get_nulls_value(node) is not
compared against udp_hashfn(net, hnum, udptable->mask) or hash2 here the way
udp6_lib_lookup1() and udp6_lib_lookup2() now do.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922095747.3059954-1-edumazet%40google.com

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

[Eric Dumazet]

On Wed, Sep 23, 2026 at 2:59 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 2 · Low: 2

Thanks for the review, comments below.

> [Severity: Medium]
> The pre-image here was an unconditional head insert.  Is the new IPv6 plus
> SO_REUSEPORT tail insert part of the hlist_nulls conversion?  The changelog
> only describes restoring hlist_nulls and adding the traversal restarts, so
> this chain ordering change is not mentioned.

No, it is not, and it should not have been there.  It was an attempt to
make udp_lib_rehash() consistent with udp_lib_get_port(), but it is not
needed for the hlist_nulls conversion and does not belong in this patch.

v2 keeps the unconditional head insert.  The CONFIG_BASE_SMALL
misdelivery you describe is a consequence of this hunk and goes away
with it.

> [Severity: Medium]
> Can this tail insertion defeat the nulls-value check added elsewhere in the
> patch?

Yes, and this is the better argument for dropping that hunk.

hlist_nulls_add_tail_rcu() sets n->next = last->next, that is the nulls
marker of the bucket itself, while hlist_nulls_add_head_rcu() sets
n->next = h->first.  Only the latter is compatible with the nulls
scheme: a reader parked on a node that leaves a bucket and comes back
must be sent to the head of the chain, not to its end marker.  With the
tail insert the reader stops with a matching nulls value, so the check
can not fire and the rest of the chain is silently skipped, exactly as
you describe.

v2 keeps head insertion at this site and says why.

Note I am keeping the tail insert in sk_nulls_add_node_rcu(): the
pre-image sk_add_node_rcu(), used by udp_lib_get_port(), already did
hlist_add_tail_rcu() for IPv6 SO_REUSEPORT sockets, see d894ba18d4e4
("soreuseport: fix ordering for mixed v4/v6 sockets").  udp_lib_get_port()
only inserts unhashed sockets, and for the same-bucket re-insert case it
behaves as before the conversion, since hlist_add_tail_rcu() left
n->next = NULL and readers stopped there as well.  No change in
behavior, so not something to address in a fix for net.

> [Severity: Low]
> This path now does an O(N) chain walk with softirqs disabled.

Goes away with the above.

> [Severity: High]
> Should this walk also validate the terminating nulls value?

I do not think so, and this is not a regression.

Before this patch the same loop walked a plain hlist.  A socket moved by
udp_lib_rehash() kept its ->next pointing into the new chain, so the
reader already wandered into the other bucket, stopped on that bucket's
NULL and missed the remaining listeners of the original chain, and the
hash2_any pass could already revisit sockets.  The conversion does not
make this worse, it only makes the condition detectable.

Detecting it does not help.  As you note, by the time we reach the end
of the chain skbs have been cloned and queued, so a "goto begin" restart
would deliver duplicates, which is worse than missing a listener during
a concurrent rehash.  Multicast delivery here is best effort.

v2 adds a comment at both mcast_deliver() sites to record this.

> For completeness, __udp4_lib_demux_lookup() and __udp6_lib_demux_lookup()
> were also converted to nulls iteration without the check.

These look at the first entry only and break out, and
inet_match()/inet6_match() validate the 4-tuple, so there is nothing to
restart.

> [Severity: Low]
> This isn't a bug, but .clang-format still lists only the old name in
> ForEachMacros:

Good catch, v2 renames the entry.

Thanks !

pw-bot: cr