From 5a8728d03a491b89a25b5452fdf71a0cebf4e6aa Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 10 Nov 2025 11:02:45 +0100 Subject: [PATCH] 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. --- include/haproxy/quic_cid.h | 7 +++--- src/quic_cid.c | 50 ++++++++++++++++++++------------------ src/quic_conn.c | 12 ++++----- src/quic_rx.c | 41 +++++++++++++++---------------- 4 files changed, 56 insertions(+), 54 deletions(-) diff --git a/include/haproxy/quic_cid.h b/include/haproxy/quic_cid.h index db6d4e00f..d95092282 100644 --- 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 44698e1db..288f91467 100644 --- 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 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 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 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 3273c332d..90f3051f7 100644 --- 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 62d0c9680..dbbe320f0 100644 --- 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));