MINOR: quic: remove access to CID global tree outside of quic_cid module

haproxy generates for each QUIC connection a set of CID. The peer must
reuse them as DCID for its emitted packet. On datagram reception, DCID
field serves as identifier to dispatch them on their correct thread.

These CIDs are stored in a global CID tree. Access to this data
structure must always be protected with CID_LOCK. This commit is a
refactoring to regroup all CID tree access in quic_cid module. Several
code parts are ajusted :

* quic_cid_insert() is extended to check for insertion race-condition.
  This is useful on quic_conn instantiation. Code where such race cannot
  happen can use unsafe _quic_cid_insert() instead.

* on RETIRE_CONNECTION_ID frame reception, existing quic_cid_delete()
  function is used.

* remove tree lookup from qc_check_dcid(), extracted in the new
  quic_cmp_cid_conn() function. Ultimately, the latter should be removed
  as CID lookup could be conducted on quic_conn owned tree without
  locking.

diff --git a/include/haproxy/quic_cid.h b/include/haproxy/quic_cid.h
index 2e61218cc039821e1f75f3ce5640f73d0fdf244e..ac0bde0b15e5db5db9131d8f9975cb6f67e367ed 100644 (file)
--- a/include/haproxy/quic_cid.h
+++ b/include/haproxy/quic_cid.h
@@ -18,9 +18,14 @@ struct quic_connection_id *new_quic_cid(struct eb_root *root,
                                         struct quic_conn *qc,
                                         const struct quic_cid *orig,
                                         const struct sockaddr_storage *addr);
+
+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,
+                      struct quic_conn *qc);
 int quic_get_cid_tid(const unsigned char *cid, size_t cid_len,
                      const struct sockaddr_storage *cli_addr,
                      unsigned char *pos, size_t len);
+
 struct quic_conn *retrieve_qc_conn_from_cid(struct quic_rx_packet *pkt,
                                             struct sockaddr_storage *saddr,
                                             int *new_tid);
@@ -67,8 +72,11 @@ static inline uchar quic_cid_tree_idx(const struct quic_cid *cid)
        return _quic_cid_tree_idx(cid->data);
 }
 
-/* Insert <conn_id> into global CID tree as a thread-safe operation. */
-static inline void quic_cid_insert(struct quic_connection_id *conn_id)
+/* Insert <conn_id> into global CID tree. Do not check if value is already
+ * present in the tree. As such, it should not be used for the first DCID of a
+ * connection instance.
+ */
+static inline void _quic_cid_insert(struct quic_connection_id *conn_id)
 {
        const uchar idx = quic_cid_tree_idx(&conn_id->cid);
        struct quic_cid_tree *tree = &quic_cid_trees[idx];

diff --git a/src/quic_cid.c b/src/quic_cid.c
index a7e81af94f42544e2dce4d90bc772b60de609960..29140d410c0820fcc9024c150ee02e00b496d4db 100644 (file)
--- a/src/quic_cid.c
+++ b/src/quic_cid.c
@@ -152,6 +152,63 @@ struct quic_connection_id *new_quic_cid(struct eb_root *root,
        return NULL;
 }
 
+/* Insert <conn_id> in global CID tree. It may fail if an identical value is
+ * already stored. In this case, <new_tid> will be filled with the thread ID of
+ * the already stored CID.
+ *
+ * Returns 0 on insert success else non-zero.
+ */
+int quic_cid_insert(struct quic_connection_id *conn_id, int *new_tid)
+{
+       struct ebmb_node *node;
+       struct quic_cid_tree *tree;
+       int ret;
+
+       *new_tid = -1;
+       tree = &quic_cid_trees[quic_cid_tree_idx(&conn_id->cid)];
+
+       HA_RWLOCK_WRLOCK(QC_CID_LOCK, &tree->lock);
+       node = ebmb_insert(&tree->root, &conn_id->node, conn_id->cid.len);
+       if (node != &conn_id->node) {
+               /* Node already inserted, may happen on thread contention. */
+               conn_id = ebmb_entry(node, struct quic_connection_id, node);
+               *new_tid = HA_ATOMIC_LOAD(&conn_id->tid);
+               ret = -1;
+       }
+       else {
+               ret = 0;
+       }
+       HA_RWLOCK_WRUNLOCK(QC_CID_LOCK, &tree->lock);
+
+       return ret;
+}
+
+/* Lookup CID in global CID tree equal to <cid> data with <cid_len> length. If
+ * found, ensure CID instance is linked to <qc> connection.
+ *
+ * Returns a boolean value.
+ */
+int quic_cmp_cid_conn(const unsigned char *cid, size_t cid_len,
+                      struct quic_conn *qc)
+{
+       struct quic_cid_tree *tree;
+       struct quic_connection_id *conn_id;
+       struct ebmb_node *node;
+       int ret = 0;
+
+       tree = &quic_cid_trees[_quic_cid_tree_idx(cid)];
+       HA_RWLOCK_RDLOCK(QC_CID_LOCK, &tree->lock);
+       node = ebmb_lookup(&tree->root, cid, cid_len);
+       if (node) {
+               conn_id = ebmb_entry(node, struct quic_connection_id, node);
+               if (qc == conn_id->qc)
+                       ret = 1;
+       }
+       HA_RWLOCK_RDUNLOCK(QC_CID_LOCK, &tree->lock);
+
+       return ret;
+}
+
 /* Retrieve the thread ID associated to QUIC connection ID <cid> of length
  * <cid_len>. CID may be not found on the CID tree because it is an ODCID. In
  * this case, it will derived using client address <cli_addr> as hash

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 927df07290965d5467f4382ba0422a616bf34b8f..595dcf0448ce655f36768a430ff623fbcaff70ab 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -47,6 +47,7 @@
 #include <haproxy/proxy.h>
 #include <haproxy/quic_ack.h>
 #include <haproxy/quic_cc.h>
+#include <haproxy/quic_cid.h>
 #include <haproxy/quic_cli-t.h>
 #include <haproxy/quic_frame.h>
 #include <haproxy/quic_enc.h>
@@ -507,7 +508,7 @@ int quic_build_post_handshake_frames(struct quic_conn *qc)
                /* TODO To prevent CID tree locking, all CIDs created here
                 * could be allocated at the same time as the first one.
                 */
-               quic_cid_insert(conn_id);
+               _quic_cid_insert(conn_id);
 
                quic_connection_id_to_frm_cpy(frm, conn_id);
                LIST_APPEND(&frm_list, &frm->list);
@@ -1648,12 +1649,6 @@ const struct quic_version *qc_supported_version(uint32_t version)
  */
 int qc_check_dcid(struct quic_conn *qc, unsigned char *dcid, size_t dcid_len)
 {
-       const uchar idx = _quic_cid_tree_idx(dcid);
-       struct quic_connection_id *conn_id;
-       struct ebmb_node *node = NULL;
-       struct quic_cid_tree *tree = &quic_cid_trees[idx];
-       int ret;
-
        /* Test against our default CID or client ODCID. */
        if ((qc->scid.len == dcid_len &&
             memcmp(qc->scid.data, dcid, dcid_len) == 0) ||
@@ -1669,17 +1664,7 @@ int qc_check_dcid(struct quic_conn *qc, unsigned char *dcid, size_t dcid_len)
         *
         * TODO set it to our default CID to avoid this operation next time.
         */
-       ret = 0;
-       HA_RWLOCK_RDLOCK(QC_CID_LOCK, &tree->lock);
-       node = ebmb_lookup(&tree->root, dcid, dcid_len);
-       if (node) {
-               conn_id = ebmb_entry(node, struct quic_connection_id, node);
-               if (qc == conn_id->qc)
-                       ret = 1;
-       }
-       HA_RWLOCK_RDUNLOCK(QC_CID_LOCK, &tree->lock);
-
-       return ret;
+       return quic_cmp_cid_conn(dcid, dcid_len, qc);
 }
 
 /* Wake-up upper layer for sending if all conditions are met :

diff --git a/src/quic_rx.c b/src/quic_rx.c
index d5b45d672f6c3c0c2d482dc62fbe0cac8e3a53a9..05a0c19c2b1d85732256192e19738f1da100293d 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -982,7 +982,6 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                        break;
                case QUIC_FT_RETIRE_CONNECTION_ID:
                {
-                       struct quic_cid_tree *tree __maybe_unused;
                        struct quic_connection_id *conn_id = NULL;
 
                        if (!qc_handle_retire_connection_id_frm(qc, &frm, &pkt->dcid, &conn_id))
@@ -991,10 +990,7 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                        if (!conn_id)
                                break;
 
-                       tree = &quic_cid_trees[quic_cid_tree_idx(&conn_id->cid)];
-                       HA_RWLOCK_WRLOCK(QC_CID_LOCK, &tree->lock);
-                       ebmb_delete(&conn_id->node);
-                       HA_RWLOCK_WRUNLOCK(QC_CID_LOCK, &tree->lock);
+                       quic_cid_delete(conn_id);
                        eb64_delete(&conn_id->seq_num);
                        pool_free(pool_head_quic_connection_id, conn_id);
                        TRACE_PROTO("CID retired", QUIC_EV_CONN_PSTRM, qc);
@@ -1004,7 +1000,7 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                                TRACE_ERROR("CID allocation error", QUIC_EV_CONN_IO_CB, qc);
                        }
                        else {
-                               quic_cid_insert(conn_id);
+                               _quic_cid_insert(conn_id);
                                qc_build_new_connection_id_frm(qc, conn_id);
                        }
                        break;
@@ -1583,8 +1579,6 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
                BUG_ON(!pkt->version); /* This must not happen. */
 
                if (!qc) {
-                       struct quic_cid_tree *tree;
-                       struct ebmb_node *node;
                        struct quic_connection_id *conn_id;
                        int ipv4;
 
@@ -1660,14 +1654,8 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
                                qc->hash64 = quic_hash64_from_cid(conn_id->cid.data, conn_id->cid.len,
                                                                  global.cluster_secret, sizeof(global.cluster_secret));
 
-                       tree = &quic_cid_trees[quic_cid_tree_idx(&conn_id->cid)];
-                       HA_RWLOCK_WRLOCK(QC_CID_LOCK, &tree->lock);
-                       node = ebmb_insert(&tree->root, &conn_id->node, conn_id->cid.len);
-                       if (node != &conn_id->node) {
+                       if (quic_cid_insert(conn_id, new_tid)) {
                                pool_free(pool_head_quic_connection_id, conn_id);
-
-                               conn_id = ebmb_entry(node, struct quic_connection_id, node);
-                               *new_tid = HA_ATOMIC_LOAD(&conn_id->tid);
                                quic_conn_release(qc);
                                qc = NULL;
                        }
@@ -1680,7 +1668,6 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
                                /* Initialize the next CID sequence number to be used for this connection. */
                                qc->next_cid_seq_num = 1;
                        }
-                       HA_RWLOCK_WRUNLOCK(QC_CID_LOCK, &tree->lock);
 
                        if (*new_tid != -1)
                                goto out;