]> git.ipfire.org Git - thirdparty/kernel/stable.git/commitdiff
Fix race for duplicate reqsk on identical SYN
authorluoxuanqiang <luoxuanqiang@kylinos.cn>
Fri, 21 Jun 2024 01:39:29 +0000 (09:39 +0800)
committerGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Fri, 5 Jul 2024 07:37:54 +0000 (09:37 +0200)
[ Upstream commit ff46e3b4421923937b7f6e44ffcd3549a074f321 ]

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 potentially create the same sk (sock) but two different reqsk
(request_sock) in tcp_conn_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 will 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.

========================================================================

This behavior is consistently reproducible in my local setup,
which comprises:

                  | 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
  bonded 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
  NETB2, 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: Flags [S], seq 320236027,
10.10.10.10.45182 > localhost: Flags [S], seq 320236027,
localhost > 10.10.10.10.45182: Flags [S.], seq 2967855116,
localhost > 10.10.10.10.45182: Flags [S.], seq 2967855123, <==
10.10.10.10.45182 > localhost: Flags [.], ack 4294967290,
10.10.10.10.45182 > localhost: Flags [.], ack 4294967290,
localhost > 10.10.10.10.45182: Flags [R], seq 2967855117, <==
localhost > 10.10.10.10.45182: Flags [R], seq 2967855117,

Two SYNACKs with different seq numbers are sent by localhost,
resulting in an anomaly.

========================================================================

The attempted solution is as follows:
Add a return value to inet_csk_reqsk_queue_hash_add() to confirm if the
ehash insertion is successful (Up to now, the reason for unsuccessful
insertion is that a reqsk for the same connection has already been
inserted). If the insertion fails, release the reqsk.

Due to the refcnt, Kuniyuki suggests also adding a return value check
for the DCCP module; if ehash insertion fails, indicating a successful
insertion of the same connection, simply release the reqsk as well.

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

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: luoxuanqiang <luoxuanqiang@kylinos.cn>
Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Link: https://lore.kernel.org/r/20240621013929.1386815-1-luoxuanqiang@kylinos.cn
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
include/net/inet_connection_sock.h
net/dccp/ipv4.c
net/dccp/ipv6.c
net/ipv4/inet_connection_sock.c
net/ipv4/tcp_input.c

index ccf171f7eb60d462e0ebf49c9e876016e963ffa5..146ece8563cae4a3bda2a88e5623d9461ec5d89e 100644 (file)
@@ -266,7 +266,7 @@ struct dst_entry *inet_csk_route_child_sock(const struct sock *sk,
 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,
+bool inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
                                   unsigned long timeout);
 struct sock *inet_csk_complete_hashdance(struct sock *sk, struct sock *child,
                                         struct request_sock *req,
index 44b033fe1ef6859df0703c7e580cf20c771ad479..f94d30b171992f5e621f0d8defc0463692336abe 100644 (file)
@@ -655,8 +655,11 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
        if (dccp_v4_send_response(sk, req))
                goto drop_and_free;
 
-       inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);
-       reqsk_put(req);
+       if (unlikely(!inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT)))
+               reqsk_free(req);
+       else
+               reqsk_put(req);
+
        return 0;
 
 drop_and_free:
index ded07e09f8135aaf6aaa08f0adcb009d8614223c..ddbd490b3531b2505f0dc8759a22d2eb0b68e1a7 100644 (file)
@@ -398,8 +398,11 @@ static int dccp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
        if (dccp_v6_send_response(sk, req))
                goto drop_and_free;
 
-       inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);
-       reqsk_put(req);
+       if (unlikely(!inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT)))
+               reqsk_free(req);
+       else
+               reqsk_put(req);
+
        return 0;
 
 drop_and_free:
index 3b38610958ee4bc3d9296c9b5e5fce6ab5c97c2c..39e9070fe3cdfb4f5044e5f051c3e7de94a82d95 100644 (file)
@@ -1121,25 +1121,34 @@ drop:
        inet_csk_reqsk_queue_drop_and_put(oreq->rsk_listener, oreq);
 }
 
-static void reqsk_queue_hash_req(struct request_sock *req,
+static bool reqsk_queue_hash_req(struct request_sock *req,
                                 unsigned long timeout)
 {
+       bool found_dup_sk = false;
+
+       if (!inet_ehash_insert(req_to_sk(req), NULL, &found_dup_sk))
+               return false;
+
+       /* 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);
 
-       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.
         */
        smp_wmb();
        refcount_set(&req->rsk_refcnt, 2 + 1);
+       return true;
 }
 
-void inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
+bool inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req,
                                   unsigned long timeout)
 {
-       reqsk_queue_hash_req(req, timeout);
+       if (!reqsk_queue_hash_req(req, timeout))
+               return false;
+
        inet_csk_reqsk_queue_added(sk);
+       return true;
 }
 EXPORT_SYMBOL_GPL(inet_csk_reqsk_queue_hash_add);
 
index 1054a440332d301796aedec76a2771e8ad4d6e0a..d37b45b90a61cf84f400a2106cf613d75046fc06 100644 (file)
@@ -7243,7 +7243,12 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
                tcp_rsk(req)->tfo_listener = false;
                if (!want_cookie) {
                        req->timeout = tcp_timeout_init((struct sock *)req);
-                       inet_csk_reqsk_queue_hash_add(sk, req, req->timeout);
+                       if (unlikely(!inet_csk_reqsk_queue_hash_add(sk, req,
+                                                                   req->timeout))) {
+                               reqsk_free(req);
+                               return 0;
+                       }
+
                }
                af_ops->send_synack(sk, dst, &fl, req, &foc,
                                    !want_cookie ? TCP_SYNACK_NORMAL :