Received-SPF: pass (google.com: domain of linux-kernel+bounces-214383-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: luoxuanqiang <luoxuanqiang@kylinos.cn>
To: edumazet@google.com,
	davem@davemloft.net,
	dsahern@kernel.org,
	kuba@kernel.org,
	pabeni@redhat.com
Cc: netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	luoxuanqiang@kylinos.cn
Subject: [PATCH v1 1/1] Fix race for duplicate reqsk on identical SYN
Date: Fri, 14 Jun 2024 14:00:12 +0800
Message-Id: <20240614060012.158026-1-luoxuanqiang@kylinos.cn>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

When bonding is configured in BOND_MODE_BROADCAST mode, if two identical =
SYN packets
are received at the same time and processed on different CPUs, it can pot=
entially
create the same sk (sock) but two different reqsk (request_sock) in tcp_c=
onn_request().

These two different reqsk will respond with two SYNACK packets, and since=
 the generation
of the seq (ISN) incorporates a timestamp, the final two SYNACK packets w=
ill have
different seq values.

The consequence is that when the Client receives and replies with an ACK =
to the earlier
SYNACK packet, we will reset(RST) it.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D

This behavior is consistently reproducible in my local setup, which compr=
ises:

                  | NETA1 ------ NETB1 |
PC_A --- bond --- |                    | --- bond --- PC_B
                  | NETA2 ------ NETB2 |

- PC_A is the Server and has two network cards, NETA1 and NETA2. I have b=
onded these two
  cards using BOND_MODE_BROADCAST mode and configured them to be handled =
by different CPU.

- PC_B is the Client, also equipped with two network cards, NETB1 and NET=
B2, which are
  also bonded and configured in BOND_MODE_BROADCAST mode.

If the client attempts a TCP connection to the server, it might encounter=
 a failure.
Capturing packets from the server side reveals:

10.10.10.10.45182 > localhost.localdomain.search-agent: Flags [S], seq 32=
0236027,
10.10.10.10.45182 > localhost.localdomain.search-agent: Flags [S], seq 32=
0236027,
localhost.localdomain.search-agent > 10.10.10.10.45182: Flags [S.], seq 2=
967855116,
localhost.localdomain.search-agent > 10.10.10.10.45182: Flags [S.], seq 2=
967855123, <=3D=3D
10.10.10.10.45182 > localhost.localdomain.search-agent: Flags [.], ack 42=
94967290,
10.10.10.10.45182 > localhost.localdomain.search-agent: Flags [.], ack 42=
94967290,
localhost.localdomain.search-agent > 10.10.10.10.45182: Flags [R], seq 29=
67855117, <=3D=3D
localhost.localdomain.search-agent > 10.10.10.10.45182: Flags [R], seq 29=
67855117,

Two SYNACKs with different seq numbers are sent by localhost, resulting i=
n an anomaly.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D

The attempted solution is as follows:
In the tcp_conn_request(), while inserting reqsk into the ehash table, it=
 also checks
if an entry already exists. If found, it avoids reinsertion and releases =
it.

Simultaneously, In the reqsk_queue_hash_req(), the start of the req->rsk_=
timer is
adjusted to be after successful insertion.

Signed-off-by: luoxuanqiang <luoxuanqiang@kylinos.cn>
---
 include/net/inet_connection_sock.h |  2 +-
 net/dccp/ipv4.c                    |  2 +-
 net/dccp/ipv6.c                    |  2 +-
 net/ipv4/inet_connection_sock.c    | 16 ++++++++++++----
 net/ipv4/tcp_input.c               | 11 ++++++++++-
 5 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connec=
tion_sock.h
index 7d6b1254c92d..8773d161d184 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -264,7 +264,7 @@ struct sock *inet_csk_reqsk_queue_add(struct sock *sk=
,
 				      struct request_sock *req,
 				      struct sock *child);
 void inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock =
*req,
-				   unsigned long timeout);
+				   unsigned long timeout, bool *found_dup_sk);
 struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *c=
hild,
 					 struct request_sock *req,
 					 bool own_req);
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index ff41bd6f99c3..13aafdeb9205 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -657,7 +657,7 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_b=
uff *skb)
 	if (dccp_v4_send_response(sk, req))
 		goto drop_and_free;
=20
-	inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);
+	inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT, NULL);
 	reqsk_put(req);
 	return 0;
=20
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 85f4b8fdbe5e..493cdb12ce2b 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -400,7 +400,7 @@ static int dccp_v6_conn_request(struct sock *sk, stru=
ct sk_buff *skb)
 	if (dccp_v6_send_response(sk, req))
 		goto drop_and_free;
=20
-	inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);
+	inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT, NULL);
 	reqsk_put(req);
 	return 0;
=20
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_s=
ock.c
index d81f74ce0f02..d9394db98a5a 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -1123,12 +1123,17 @@ static void reqsk_timer_handler(struct timer_list=
 *t)
 }
=20
 static void reqsk_queue_hash_req(struct request_sock *req,
-				 unsigned long timeout)
+				 unsigned long timeout, bool *found_dup_sk)
 {
+
+	inet_ehash_insert(req_to_sk(req), NULL, found_dup_sk);
+	if(found_dup_sk && *found_dup_sk)
+		return;
+
+	/* The timer needs to be setup after a successful insertion. */
 	timer_setup(&req->rsk_timer, reqsk_timer_handler, TIMER_PINNED);
 	mod_timer(&req->rsk_timer, jiffies + timeout);
=20
-	inet_ehash_insert(req_to_sk(req), NULL, NULL);
 	/* before letting lookups find us, make sure all req fields
 	 * are committed to memory and refcnt initialized.
 	 */
@@ -1137,9 +1142,12 @@ static void reqsk_queue_hash_req(struct request_so=
ck *req,
 }
=20
 void inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock =
*req,
-				   unsigned long timeout)
+				   unsigned long timeout, bool *found_dup_sk)
 {
-	reqsk_queue_hash_req(req, timeout);
+	reqsk_queue_hash_req(req, timeout, found_dup_sk);
+	if(found_dup_sk && *found_dup_sk)
+		return;
+
 	inet_csk_reqsk_queue_added(sk);
 }
 EXPORT_SYMBOL_GPL(inet_csk_reqsk_queue_hash_add);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9c04a9c8be9d..467f1b7bbd5a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -7255,8 +7255,17 @@ int tcp_conn_request(struct request_sock_ops *rsk_=
ops,
 	} else {
 		tcp_rsk(req)->tfo_listener =3D false;
 		if (!want_cookie) {
+			bool found_dup_sk =3D false;
+
 			req->timeout =3D tcp_timeout_init((struct sock *)req);
-			inet_csk_reqsk_queue_hash_add(sk, req, req->timeout);
+			inet_csk_reqsk_queue_hash_add(sk, req, req->timeout,
+							&found_dup_sk);
+
+			if(unlikely(found_dup_sk)){
+				reqsk_free(req);
+				return 0;
+			}
+
 		}
 		af_ops->send_synack(sk, dst, &fl, req, &foc,
 				    !want_cookie ? TCP_SYNACK_NORMAL :
--=20
2.25.1