MEDIUM/OPTIM: quic: alloc quic_conn after CID collision check

On Initial packet parsing, a new quic_conn instance is allocated via
qc_new_conn(). Then a CID is allocated with its value derivated from
client ODCID. On CID tree insert, a collision can occur if another
thread was already parsing an Initial packet from the same client. In
this case, the connection is released and the packet will be requeued to
the other thread.

Originally, CID collision check was performed prior to quic_conn
allocation. This was changed by the commit below, as this could cause
issue on quic_conn alloc failure.

  commit 4ae29be18c5b212dd2a1a8e9fa0ee2fcb9dbb4b3
  BUG/MINOR: quic: Possible endless loop in quic_lstnr_dghdlr()

However, this procedure is less optimal. Indeed, qc_new_conn() performs
many steps, thus it could be better to skip it on Initial CID collision,
which can happen frequently. This patch restores the older order of
operations, with CID collision check prior to quic_conn allocation.

To ensure this does not cause again the same bug, the CID is removed in
case of quic_conn alloc failure. This should prevent any loop as it
ensures that a CID found in the global tree does not point to a NULL
quic_conn, unless if CID is attach to a foreign thread. When this thread
will parse a re-enqueued packet, either the quic_conn is already
allocated or the CID has been removed, triggering a fresh CID and
quic_conn allocation procedure.

diff --git a/include/haproxy/quic_cid.h b/include/haproxy/quic_cid.h
index db6d4e00f443a8e59b44bd8f93bfac4642231948..d95092282b07c5d39f0e98d810ea5170f78c57da 100644 (file)
--- a/include/haproxy/quic_cid.h
+++ b/include/haproxy/quic_cid.h
@@ -17,15 +17,16 @@
 
 extern struct quic_cid_tree *quic_cid_trees;
 
-struct quic_connection_id *quic_cid_alloc(struct quic_conn *qc);
+struct quic_connection_id *quic_cid_alloc(void);
 
-int quic_cid_generate(struct quic_connection_id *conn_id);
+int quic_cid_generate(struct quic_connection_id *conn_id, uint64_t hash);
 
 int quic_cid_derive_from_odcid(struct quic_connection_id *conn_id,
                                const struct quic_cid *orig,
                                const struct sockaddr_storage *addr);
 
-void quic_cid_register_seq_num(struct quic_connection_id *conn_id);
+void quic_cid_register_seq_num(struct quic_connection_id *conn_id,
+                               struct quic_conn *qc);
 
 int quic_cid_insert(struct quic_connection_id *conn_id, int *new_tid);
 int quic_cmp_cid_conn(const unsigned char *cid, size_t cid_len,

diff --git a/src/quic_cid.c b/src/quic_cid.c
index 44698e1db1f8dfa1c01156c32519d30e632aff4e..288f914673dd2e81473c247147a05bf45595f7dd 100644 (file)
--- a/src/quic_cid.c
+++ b/src/quic_cid.c
@@ -114,74 +114,73 @@ static struct quic_cid quic_derive_cid(const struct quic_cid *orig,
        return cid;
 }
 
-/* Allocate a quic_connection_id object and associate it with <qc> connection.
- * The CID object is not yet inserted in any tree storage.
+/* Allocate a quic_connection_id object and associate it to the current thread.
+ * The CID object is not yet associated to a connection or inserted in any tree
+ * storage.
  *
  * Returns the CID or NULL on allocation failure.
  */
-struct quic_connection_id *quic_cid_alloc(struct quic_conn *qc)
+struct quic_connection_id *quic_cid_alloc(void)
 {
        struct quic_connection_id *conn_id;
 
        /* TODO use a better trace scope */
-       TRACE_ENTER(QUIC_EV_CONN_TXPKT, qc);
+       TRACE_ENTER(QUIC_EV_CONN_TXPKT);
 
        conn_id = pool_alloc(pool_head_quic_connection_id);
        if (!conn_id) {
-               TRACE_ERROR("cid allocation failed", QUIC_EV_CONN_TXPKT, qc);
+               TRACE_ERROR("cid allocation failed", QUIC_EV_CONN_TXPKT);
                goto err;
        }
 
-       conn_id->qc = qc;
        HA_ATOMIC_STORE(&conn_id->tid, tid);
+       conn_id->qc = NULL;
 
-       TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc);
+       TRACE_LEAVE(QUIC_EV_CONN_TXPKT);
        return conn_id;
 
  err:
-       TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc);
+       TRACE_LEAVE(QUIC_EV_CONN_TXPKT);
        return NULL;
 }
 
 /* Generate the value of <conn_id> and its associated stateless token. The CID
  * value is calculated from a random generator or via quic_newcid_from_hash64()
- * external callback if defined with hash64 key from connection.
+ * external callback if defined with <hash64> key.
  *
  * Returns 0 on success else non-zero.
  */
-int quic_cid_generate(struct quic_connection_id *conn_id)
+int quic_cid_generate(struct quic_connection_id *conn_id, uint64_t hash64)
 {
-       const struct quic_conn *qc = conn_id->qc;
-
        /* TODO use a better trace scope */
-       TRACE_ENTER(QUIC_EV_CONN_TXPKT, qc);
+       TRACE_ENTER(QUIC_EV_CONN_TXPKT);
 
        conn_id->cid.len = QUIC_HAP_CID_LEN;
 
        if (quic_newcid_from_hash64) {
-               TRACE_DEVEL("calculate CID value from conn hash", QUIC_EV_CONN_TXPKT, qc);
-               quic_newcid_from_hash64(conn_id->cid.data, conn_id->cid.len, qc->hash64,
+               TRACE_DEVEL("calculate CID value from conn hash", QUIC_EV_CONN_TXPKT);
+               quic_newcid_from_hash64(conn_id->cid.data, conn_id->cid.len, hash64,
                                        global.cluster_secret, sizeof(global.cluster_secret));
        }
        else {
-               TRACE_DEVEL("generate CID value from random generator", QUIC_EV_CONN_TXPKT, qc);
+               TRACE_DEVEL("generate CID value from random generator", QUIC_EV_CONN_TXPKT);
                if (RAND_bytes(conn_id->cid.data, conn_id->cid.len) != 1) {
                        /* TODO: RAND_bytes() should be replaced */
-                       TRACE_ERROR("RAND_bytes() failed", QUIC_EV_CONN_TXPKT, qc);
+                       TRACE_ERROR("RAND_bytes() failed", QUIC_EV_CONN_TXPKT);
                        goto err;
                }
        }
 
        if (quic_stateless_reset_token_init(conn_id) != 1) {
-               TRACE_ERROR("quic_stateless_reset_token_init() failed", QUIC_EV_CONN_TXPKT, qc);
+               TRACE_ERROR("quic_stateless_reset_token_init() failed", QUIC_EV_CONN_TXPKT);
                goto err;
        }
 
-       TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc);
+       TRACE_LEAVE(QUIC_EV_CONN_TXPKT);
        return 0;
 
  err:
-       TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc);
+       TRACE_LEAVE(QUIC_EV_CONN_TXPKT);
        return 1;
 }
 
@@ -227,13 +226,13 @@ int quic_cid_derive_from_odcid(struct quic_connection_id *conn_id,
  * sequence number available. The CID should already be stored in the global
  * tree to ensure there is no value collision.
  */
-void quic_cid_register_seq_num(struct quic_connection_id *conn_id)
+void quic_cid_register_seq_num(struct quic_connection_id *conn_id,
+                               struct quic_conn *qc)
 {
-       struct quic_conn *qc = conn_id->qc;
-
        /* TODO use a better trace scope */
        TRACE_ENTER(QUIC_EV_CONN_TXPKT, qc);
 
+       conn_id->qc = qc;
        conn_id->seq_num.key = qc->next_cid_seq_num;
        conn_id->retire_prior_to = 0;
        /* insert the allocated CID in the quic_conn tree */
@@ -413,6 +412,11 @@ struct quic_conn *retrieve_qc_conn_from_cid(struct quic_rx_packet *pkt,
                *new_tid = conn_id_tid;
                goto end;
        }
+
+       /* Ensures that conn is always set if CID is found on its thread.
+        * Necessary to prevent loop of re-enqueued Initial packets.
+        */
+       BUG_ON(!conn_id->qc);
        qc = conn_id->qc;
        TRACE_DEVEL("found connection", QUIC_EV_CONN_RXPKT, qc);
 

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 3273c332daae364b53dd0c40aa9bd1ad75e4e6e6..90f3051f7d14f66c7529660e20f8ef5c867fb6e7 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -525,14 +525,14 @@ int quic_build_post_handshake_frames(struct quic_conn *qc)
                        goto err;
                }
 
-               conn_id = quic_cid_alloc(qc);
+               conn_id = quic_cid_alloc();
                if (!conn_id) {
                        qc_frm_free(qc, &frm);
                        TRACE_ERROR("CID allocation error", QUIC_EV_CONN_IO_CB, qc);
                        goto err;
                }
 
-               if (quic_cid_generate(conn_id)) {
+               if (quic_cid_generate(conn_id, qc->hash64)) {
                        qc_frm_free(qc, &frm);
                        pool_free(pool_head_quic_connection_id, conn_id);
                        TRACE_ERROR("error on CID generation", QUIC_EV_CONN_IO_CB, qc);
@@ -546,7 +546,7 @@ int quic_build_post_handshake_frames(struct quic_conn *qc)
                        continue;
                }
 
-               quic_cid_register_seq_num(conn_id);
+               quic_cid_register_seq_num(conn_id, qc);
                quic_connection_id_to_frm_cpy(frm, conn_id);
                LIST_APPEND(&frm_list, &frm->list);
        }
@@ -1235,13 +1235,13 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4,
                memcpy(&qc->odcid, qc->dcid.data, sizeof(qc->dcid.data));
                qc->odcid.len = qc->dcid.len;
 
-               conn_cid = quic_cid_alloc(qc);
+               conn_cid = quic_cid_alloc();
                if (!conn_cid) {
                        TRACE_ERROR("error on CID allocation", QUIC_EV_CONN_INIT, qc);
                        goto err;
                }
 
-               if (quic_cid_generate(conn_cid)) {
+               if (quic_cid_generate(conn_cid, qc->hash64)) {
                        TRACE_ERROR("error on CID generation", QUIC_EV_CONN_INIT, qc);
                        pool_free(pool_head_quic_connection_id, conn_cid);
                        goto err;
@@ -1252,7 +1252,7 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4,
                        goto err;
                }
 
-               quic_cid_register_seq_num(conn_cid);
+               quic_cid_register_seq_num(conn_cid, qc);
                dcid = &qc->dcid;
                conn_id = conn_cid;
        }

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 62d0c9680dcaeb0d71b92be54d47185178e9a584..dbbe320f0bf33cde1ea4f913b6f745b5c1f4f4b9 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -1018,7 +1018,7 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                        pool_free(pool_head_quic_connection_id, conn_id);
                        TRACE_PROTO("CID retired", QUIC_EV_CONN_PSTRM, qc);
 
-                       conn_id = quic_cid_alloc(qc);
+                       conn_id = quic_cid_alloc();
                        if (!conn_id) {
                                TRACE_ERROR("CID allocation error", QUIC_EV_CONN_IO_CB, qc);
                                quic_set_connection_close(qc, quic_err_transport(QC_ERR_INTERNAL_ERROR));
@@ -1027,7 +1027,7 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                        }
 
                        while (retry_rand_cid--) {
-                               if (quic_cid_generate(conn_id)) {
+                               if (quic_cid_generate(conn_id, qc->hash64)) {
                                        TRACE_ERROR("error on CID generation", QUIC_EV_CONN_PSTRM, qc);
                                        quic_set_connection_close(qc, quic_err_transport(QC_ERR_INTERNAL_ERROR));
                                        pool_free(pool_head_quic_connection_id, conn_id);
@@ -1051,7 +1051,7 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                                goto err;
                        }
 
-                       quic_cid_register_seq_num(conn_id);
+                       quic_cid_register_seq_num(conn_id, qc);
 
                        qc_build_new_connection_id_frm(qc, conn_id);
                        break;
@@ -1762,7 +1762,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
                        pkt->saddr = dgram->saddr;
                        ipv4 = dgram->saddr.ss_family == AF_INET;
 
-                       conn_id = quic_cid_alloc(NULL);
+                       conn_id = quic_cid_alloc();
                        if (!conn_id) {
                                TRACE_ERROR("error on first CID allocation",
                                            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
@@ -1776,21 +1776,6 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
                                goto err;
                        }
 
-                       qc = qc_new_conn(pkt->version, ipv4, &pkt->dcid, &pkt->scid, &token_odcid,
-                                        conn_id, &dgram->daddr, &pkt->saddr,
-                                        !!pkt->token_len, l);
-                       if (qc == NULL) {
-                               pool_free(pool_head_quic_connection_id, conn_id);
-                               goto err;
-                       }
-
-                       conn_id->qc = qc;
-                       /* Compute and store into the quic_conn the hash used to compute extra CIDs */
-                       if (quic_hash64_from_cid) {
-                               qc->hash64 = quic_hash64_from_cid(conn_id->cid.data, conn_id->cid.len,
-                                                                 global.cluster_secret, sizeof(global.cluster_secret));
-                       }
-
                        if (quic_cid_insert(conn_id, new_tid)) {
                                /* Collision during CID insertion. This may
                                 * happen when handling two Initial packets
@@ -1800,11 +1785,23 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
                                 */
                                TRACE_STATE("duplicate CID on Initial", QUIC_EV_CONN_LPKT);
                                pool_free(pool_head_quic_connection_id, conn_id);
-                               quic_conn_release(qc);
-                               qc = NULL;
                        }
                        else {
-                               quic_cid_register_seq_num(conn_id);
+                               qc = qc_new_conn(pkt->version, ipv4, &pkt->dcid, &pkt->scid, &token_odcid,
+                                                conn_id, &dgram->daddr, &pkt->saddr,
+                                                !!pkt->token_len, l);
+                               if (qc == NULL) {
+                                       quic_cid_delete(conn_id); /* Removes CID from global tree as it points to a NULL qc. */
+                                       pool_free(pool_head_quic_connection_id, conn_id);
+                                       goto err;
+                               }
+
+                               /* Compute the hash used to derive extra CIDs. */
+                               if (quic_hash64_from_cid) {
+                                       qc->hash64 = quic_hash64_from_cid(conn_id->cid.data, conn_id->cid.len,
+                                                                         global.cluster_secret, sizeof(global.cluster_secret));
+                               }
+                               quic_cid_register_seq_num(conn_id, qc);
 
                                if (dgram->flags & QUIC_DGRAM_FL_REJECT)
                                        quic_set_connection_close(qc, quic_err_transport(QC_ERR_CONNECTION_REFUSED));