From: Frédéric Lécaille Date: Mon, 26 Jun 2023 08:39:56 +0000 (+0200) Subject: BUG/MINOR: quic: Prevent deadlock with CID tree lock X-Git-Tag: v2.9-dev1~32 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1231810963fd00e463ca46af1130c46ede312a52;p=thirdparty%2Fhaproxy.git BUG/MINOR: quic: Prevent deadlock with CID tree lock This bug was introduced by this commit which was not sufficient: BUG/MINOR: quic: Possible endless loop in quic_lstnr_dghdlr() It was revealed by the blackhole interop runner test with neqo as client. qc_conn_release() could be called after having locke the CID tree when two different threads was creating the same connection at the same time. Indeed in this case the last thread which tried to create a new connection for the same an already existing CID could not manage to insert an already inserted CID in the connection CID tree. This was expected. It had to destroy the newly created for nothing connection calling qc_conn_release(). But this function also locks this tree calling free_quic_conn_cids() leading to a deadlock. A solution would have been to delete the new CID created from its tree before calling qc_conn_release(). A better solution is to stop inserting the first CID from qc_new_conn(), and to insert it into the CID tree only if there was not an already created connection. This is whas is implemented by this patch. Must be backported as far as 2.7. --- diff --git a/src/quic_conn.c b/src/quic_conn.c index ecf30e6daa..ed15f53ed5 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -5513,8 +5513,12 @@ static int parse_retry_token(struct quic_conn *qc, /* Allocate a new QUIC connection with as QUIC version. * boolean is set to 1 for IPv4 connection, 0 for IPv6. is set to 1 * for QUIC servers (or haproxy listeners). - * is the destination connection ID, is the source connection ID, - * the token found to be used for this connection with as + * is the destination connection ID, is the source connection ID. + * This latter CID as the same value on the wire as the one for + * which is the first CID of this connection but a different internal representation used to build + * NEW_CONNECTION_ID frames. This is the responsability of the caller to insert + * in the CIDs tree for this connection (qc->cids). + * is the token found to be used for this connection with as * length. Endpoints addresses are specified via and . * Returns the connection if succeeded, NULL if not. */ @@ -5648,9 +5652,6 @@ static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, qc->err = quic_err_transport(QC_ERR_NO_ERROR); conn_id->qc = qc; - 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; if ((global.tune.options & GTUNE_QUIC_SOCK_PER_CONN) && is_addr(local_addr)) { @@ -6961,7 +6962,6 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt, conn_id, &dgram->daddr, &pkt->saddr, 1, !!pkt->token_len, l); if (qc == NULL) { - eb64_delete(&conn_id->seq_num); pool_free(pool_head_quic_connection_id, conn_id); goto err; } @@ -6977,6 +6977,15 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt, 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; + } HA_RWLOCK_WRUNLOCK(QC_CID_LOCK, &tree->lock); if (*new_tid != -1)