From: Amaury Denoyelle Date: Thu, 6 Nov 2025 18:19:26 +0000 (+0100) Subject: MINOR: quic: split CID alloc/generation function X-Git-Tag: v3.3-dev13~55 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=419e5509d87d9d4f1d75e619bfacf2b4b827b7b2;p=thirdparty%2Fhaproxy.git MINOR: quic: split CID alloc/generation function Split new_quic_cid() function into multiple ones. This patch should not introduce any visible change. The objective is to render CID allocation and generation more modular. The first advantage of this patch is to bring code simplication. In particular, conn CID sequence number increment and insertion into connection tree is simpler than before. Another improvment is also that errors could now be handled easier at each different steps of the CID init. This patch is a prerequisite for the fix on CID collision, thus it must be backported prior to it to every affected version. --- diff --git a/include/haproxy/quic_cid.h b/include/haproxy/quic_cid.h index 87f675308..2ee368f95 100644 --- a/include/haproxy/quic_cid.h +++ b/include/haproxy/quic_cid.h @@ -17,10 +17,15 @@ extern struct quic_cid_tree *quic_cid_trees; -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); +struct quic_connection_id *quic_cid_alloc(struct quic_conn *qc); + +int quic_cid_generate(struct quic_connection_id *conn_id); + +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); 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 cea5d371a..6f83bfced 100644 --- a/src/quic_cid.c +++ b/src/quic_cid.c @@ -114,47 +114,58 @@ static struct quic_cid quic_derive_cid(const struct quic_cid *orig, return cid; } -/* Allocate a new CID and attach it to ebtree. +/* Allocate a quic_connection_id object and associate it with connection. + * The CID object is not yet inserted in any tree storage. * - * If and params are non null, the new CID value is directly - * derived from them. Else a random value is generated. The CID is then marked - * with the current thread ID. - * - * Returns the new CID if succeeded, NULL if not. + * Returns the CID or NULL on allocation failure. */ -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) +struct quic_connection_id *quic_cid_alloc(struct quic_conn *qc) { struct quic_connection_id *conn_id; + /* TODO use a better trace scope */ TRACE_ENTER(QUIC_EV_CONN_TXPKT, qc); - /* Caller must set either none or both values. */ - BUG_ON(!!orig != !!addr); - conn_id = pool_alloc(pool_head_quic_connection_id); if (!conn_id) { TRACE_ERROR("cid allocation failed", QUIC_EV_CONN_TXPKT, qc); goto err; } + conn_id->qc = qc; + HA_ATOMIC_STORE(&conn_id->tid, tid); + + TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); + return conn_id; + + err: + TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); + 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. + * + * Returns 0 on success else non-zero. + */ +int quic_cid_generate(struct quic_connection_id *conn_id) +{ + const struct quic_conn *qc = conn_id->qc; + + /* TODO use a better trace scope */ + TRACE_ENTER(QUIC_EV_CONN_TXPKT, qc); + conn_id->cid.len = QUIC_HAP_CID_LEN; - if (!orig) { - if (quic_newcid_from_hash64) - quic_newcid_from_hash64(conn_id->cid.data, conn_id->cid.len, qc->hash64, - global.cluster_secret, sizeof(global.cluster_secret)); - else 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); - goto err; - } + if (quic_newcid_from_hash64) { + quic_newcid_from_hash64(conn_id->cid.data, conn_id->cid.len, qc->hash64, + global.cluster_secret, sizeof(global.cluster_secret)); } - else { - /* Derive the new CID value from original CID. */ - conn_id->cid = quic_derive_cid(orig, addr); + else 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); + goto err; } if (quic_stateless_reset_token_init(conn_id) != 1) { @@ -162,22 +173,69 @@ struct quic_connection_id *new_quic_cid(struct eb_root *root, goto err; } - conn_id->qc = qc; - HA_ATOMIC_STORE(&conn_id->tid, tid); - - conn_id->seq_num.key = qc ? qc->next_cid_seq_num++ : 0; - conn_id->retire_prior_to = 0; - /* insert the allocated CID in the quic_conn tree */ - if (root) - eb64_insert(root, &conn_id->seq_num); + TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); + return 0; + err: TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); - return conn_id; + return 1; +} + +/* Generate the value of and its associated stateless token. The CID + * value is derived from client ODCID and address. This is an + * alternative method to quic_cid_generate() which is used for the first CID of + * a server connection in response to a client INITIAL packet. + * + * The benefit of this CID value is to skip storage of ODCIDs in the global + * CIDs tree. This is an optimization to reduce contention on the CIDs tree + * given that ODCIDs usage is quite limited during a connection lifetime. + * Client address is used to reduce the collision risk. + * + * Returns 0 on success else non-zero. + */ +int quic_cid_derive_from_odcid(struct quic_connection_id *conn_id, + const struct quic_cid *orig, + const struct sockaddr_storage *addr) +{ + /* TODO use a better trace scope */ + TRACE_ENTER(QUIC_EV_CONN_TXPKT); + + conn_id->cid.len = QUIC_HAP_CID_LEN; + + /* Derive the new CID value from original CID. */ + conn_id->cid = quic_derive_cid(orig, addr); + + if (quic_stateless_reset_token_init(conn_id) != 1) { + TRACE_ERROR("quic_stateless_reset_token_init() failed", QUIC_EV_CONN_TXPKT); + goto err; + } + + TRACE_LEAVE(QUIC_EV_CONN_TXPKT); + return 0; err: - pool_free(pool_head_quic_connection_id, conn_id); + TRACE_LEAVE(QUIC_EV_CONN_TXPKT); + return 1; +} + +/* Store CID into connection tree, associated with the next + * 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) +{ + struct quic_conn *qc = conn_id->qc; + + /* TODO use a better trace scope */ + TRACE_ENTER(QUIC_EV_CONN_TXPKT, 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 */ + eb64_insert(qc->cids, &conn_id->seq_num); + ++qc->next_cid_seq_num; + TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); - return NULL; } /* Insert in global CID tree. It may fail if an identical value is diff --git a/src/quic_conn.c b/src/quic_conn.c index aba6d0cf8..81d57ea81 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -525,18 +525,26 @@ int quic_build_post_handshake_frames(struct quic_conn *qc) goto err; } - conn_id = new_quic_cid(qc->cids, qc, NULL, NULL); + conn_id = quic_cid_alloc(qc); 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)) { + 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); + goto err; + } + /* 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_register_seq_num(conn_id); quic_connection_id_to_frm_cpy(frm, conn_id); LIST_APPEND(&frm_list, &frm->list); } @@ -1182,6 +1190,7 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, goto err; } *qc->cids = EB_ROOT; + qc->next_cid_seq_num = 0; /* QUIC Server (or listener). */ if (l) { @@ -1201,7 +1210,6 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, qc->odcid = *dcid; /* Copy the packet SCID to reuse it as DCID for sending */ qc->dcid = *scid; - conn_id->qc = qc; } /* QUIC Client (outgoing connection to servers) */ else { @@ -1225,15 +1233,22 @@ 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 = new_quic_cid(NULL, qc, NULL, NULL); - if (!conn_cid) + conn_cid = quic_cid_alloc(qc); + if (!conn_cid) { + TRACE_ERROR("error on CID allocation", QUIC_EV_CONN_INIT, qc); + goto err; + } + + if (quic_cid_generate(conn_cid)) { + TRACE_ERROR("error on CID generation", QUIC_EV_CONN_INIT, qc); + pool_free(pool_head_quic_connection_id, conn_cid); goto err; + } _quic_cid_insert(conn_cid); + quic_cid_register_seq_num(conn_cid); dcid = &qc->dcid; conn_id = conn_cid; - - qc->next_cid_seq_num = 1; } qc->err = quic_err_transport(QC_ERR_NO_ERROR); @@ -1258,11 +1273,7 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, goto err; } - if (!l) { - /* Attach the current CID to the connection */ - eb64_insert(qc->cids, &conn_id->seq_num); - } - else if (HA_ATOMIC_LOAD(&l->rx.quic_mode) == QUIC_SOCK_MODE_CONN && + if (l && HA_ATOMIC_LOAD(&l->rx.quic_mode) == QUIC_SOCK_MODE_CONN && (quic_tune.fe.opts & QUIC_TUNE_FE_SOCK_PER_CONN) && is_addr(local_addr)) { /* Listener only */ diff --git a/src/quic_rx.c b/src/quic_rx.c index 6da808d37..6b8f61ad5 100644 --- a/src/quic_rx.c +++ b/src/quic_rx.c @@ -1017,17 +1017,26 @@ 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 = new_quic_cid(qc->cids, qc, NULL, NULL); + conn_id = quic_cid_alloc(qc); 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)); qc_notify_err(qc); goto err; } - else { - _quic_cid_insert(conn_id); - qc_build_new_connection_id_frm(qc, conn_id); + + if (quic_cid_generate(conn_id)) { + 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); + qc_notify_err(qc); + goto err; } + _quic_cid_insert(conn_id); + + quic_cid_register_seq_num(conn_id); + + qc_build_new_connection_id_frm(qc, conn_id); break; } case QUIC_FT_PATH_CHALLENGE: @@ -1736,15 +1745,19 @@ 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; - /* Generate the first connection CID. This is derived from the client - * ODCID and address. This allows to retrieve the connection from the - * ODCID without storing it in the CID tree. This is an interesting - * optimization as the client is expected to stop using its ODCID in - * favor of our generated value. - */ - conn_id = new_quic_cid(NULL, NULL, &pkt->dcid, &pkt->saddr); - if (!conn_id) + conn_id = quic_cid_alloc(NULL); + if (!conn_id) { + TRACE_ERROR("error on first CID allocation", + QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version); + goto err; + } + + if (quic_cid_derive_from_odcid(conn_id, &pkt->dcid, &pkt->saddr)) { + TRACE_ERROR("error on CID generation", + QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version); + pool_free(pool_head_quic_connection_id, conn_id); goto err; + } qc = qc_new_conn(pkt->version, ipv4, &pkt->dcid, &pkt->scid, &token_odcid, conn_id, &dgram->daddr, &pkt->saddr, @@ -1754,24 +1767,26 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt, 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) + 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)); + 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 + * from the same client on different threads. + * will be set to redispatch the + * current packet. + */ pool_free(pool_head_quic_connection_id, conn_id); quic_conn_release(qc); qc = NULL; } else { - /* From here, is the correct connection for this Initial - * packet. must be inserted in the CIDs tree for this - * connection. - */ - eb64_insert(qc->cids, &conn_id->seq_num); - /* Initialize the next CID sequence number to be used for this connection. */ - qc->next_cid_seq_num = 1; + quic_cid_register_seq_num(conn_id); if (dgram->flags & QUIC_DGRAM_FL_REJECT) quic_set_connection_close(qc, quic_err_transport(QC_ERR_CONNECTION_REFUSED));