Re: [RFC PATCH net-next] net: ip: add sysctl_ip_reuse_exact_match

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Thu, 27 Feb 2025 20:31:37 +0800
> For now, the socket lookup will terminate if the socket is reuse port in
> inet_lhash2_lookup(), which makes the socket is not the best match.
> 
> For example, we have socket1 listening on "0.0.0.0:1234" and socket2
> listening on "192.168.1.1:1234", and both of them enabled reuse port. The
> socket1 will always be matched when a connection with the peer ip
> "192.168.1.xx" comes if the socket1 is created later than socket2. This
> is not expected, as socket2 has higher priority.
> 
> This can cause unexpected behavior if TCP MD5 keys is used, as described
> in Documentation/networking/vrf.rst -> Applications.

Could you provide a minimal repro setup ?
I somehow fail to reproduce the issue.


> Introduce the sysctl_ip_reuse_exact_match to make it find a best matched
> socket when reuse port is used.

I think we should not introduce a new sysctl knob and an extra lookup,
rather we can solve that in __inet_hash() taking d894ba18d4e4
("soreuseport: fix ordering for mixed v4/v6 sockets") further.

Could you test this patch ?

---8<---
$ git show
commit 4dbc44a153afe51a2b2698a55213e625a02e23c8
Author: Kuniyuki Iwashima <kuniyu@amazon.com>
Date:   Thu Feb 27 19:53:43 2025 +0000

    tcp: Place non-wildcard sockets before wildcard ones in lhash2.
    
    Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 5eea47f135a4..115248bfe463 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -136,6 +136,9 @@ struct inet_bind_hashbucket {
 struct inet_listen_hashbucket {
 	spinlock_t		lock;
 	struct hlist_nulls_head	nulls_head;
+#if IS_ENABLED(CONFIG_IPV6)
+	struct hlist_nulls_node	*wildcard_node;
+#endif
 };
 
 /* This is for listening sockets, thus all sockets which possess wildcards. */
diff --git a/include/net/sock.h b/include/net/sock.h
index efc031163c33..4e8e10d2067b 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -863,6 +863,16 @@ static inline void __sk_nulls_add_node_tail_rcu(struct sock *sk, struct hlist_nu
 	hlist_nulls_add_tail_rcu(&sk->sk_nulls_node, list);
 }
 
+static inline void __sk_nulls_add_node_before_rcu(struct sock *sk, struct hlist_nulls_node *next)
+{
+	struct hlist_nulls_node *n = &sk->sk_nulls_node;
+
+	WRITE_ONCE(n->pprev, next->pprev);
+	WRITE_ONCE(n->next, next);
+	WRITE_ONCE(next->pprev, &n->next);
+	WRITE_ONCE(*(n->pprev), n);
+}
+
 static inline void sk_nulls_add_node_rcu(struct sock *sk, struct hlist_nulls_head *list)
 {
 	sock_hold(sk);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index ecda4c65788c..acfb693bb1d4 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -729,6 +729,7 @@ int __inet_hash(struct sock *sk, struct sock *osk)
 {
 	struct inet_hashinfo *hashinfo = tcp_or_dccp_get_hashinfo(sk);
 	struct inet_listen_hashbucket *ilb2;
+	bool add_tail = false;
 	int err = 0;
 
 	if (sk->sk_state != TCP_LISTEN) {
@@ -737,21 +738,47 @@ int __inet_hash(struct sock *sk, struct sock *osk)
 		local_bh_enable();
 		return 0;
 	}
+
 	WARN_ON(!sk_unhashed(sk));
 	ilb2 = inet_lhash2_bucket_sk(hashinfo, sk);
 
 	spin_lock(&ilb2->lock);
+
 	if (sk->sk_reuseport) {
 		err = inet_reuseport_add_sock(sk, ilb2);
 		if (err)
 			goto unlock;
+
+		if (inet_rcv_saddr_any(sk))
+			add_tail = true;
 	}
+
 	sock_set_flag(sk, SOCK_RCU_FREE);
-	if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
-		sk->sk_family == AF_INET6)
-		__sk_nulls_add_node_tail_rcu(sk, &ilb2->nulls_head);
-	else
-		__sk_nulls_add_node_rcu(sk, &ilb2->nulls_head);
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (sk->sk_family == AF_INET6) {
+		if (add_tail || !ilb2->wildcard_node)
+			__sk_nulls_add_node_tail_rcu(sk, &ilb2->nulls_head);
+		else
+			__sk_nulls_add_node_before_rcu(sk, ilb2->wildcard_node);
+	} else
+#endif
+	{
+		if (!add_tail)
+			__sk_nulls_add_node_rcu(sk, &ilb2->nulls_head);
+#if IS_ENABLED(CONFIG_IPV6)
+		else if (ilb2->wildcard_node)
+			__sk_nulls_add_node_before_rcu(sk, ilb2->wildcard_node);
+#endif
+		else
+			__sk_nulls_add_node_tail_rcu(sk, &ilb2->nulls_head);
+	}
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (add_tail && (sk->sk_family == AF_INET || !ilb2->wildcard_node))
+		ilb2->wildcard_node = &sk->sk_nulls_node;
+#endif
+
 	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 unlock:
 	spin_unlock(&ilb2->lock);
@@ -794,6 +821,15 @@ void inet_unhash(struct sock *sk)
 		if (rcu_access_pointer(sk->sk_reuseport_cb))
 			reuseport_stop_listen_sock(sk);
 
+#if IS_ENABLED(CONFIG_IPV6)
+		if (&sk->sk_nulls_node == ilb2->wildcard_node) {
+			if (is_a_nulls(sk->sk_nulls_node.next))
+				ilb2->wildcard_node = NULL;
+			else
+				ilb2->wildcard_node = sk->sk_nulls_node.next;
+		}
+#endif
+
 		__sk_nulls_del_node_init_rcu(sk);
 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
 		spin_unlock(&ilb2->lock);
@@ -1183,6 +1219,9 @@ static void init_hashinfo_lhash2(struct inet_hashinfo *h)
 		spin_lock_init(&h->lhash2[i].lock);
 		INIT_HLIST_NULLS_HEAD(&h->lhash2[i].nulls_head,
 				      i + LISTENING_NULLS_BASE);
+#if IS_ENABLED(CONFIG_IPV6)
+		h->lhash2[i].wildcard_node = NULL;
+#endif
 	}
 }
 
---8<---