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 ecf30e6daa468d743c1b171479c7946fdce99c77..ed15f53ed58aafb1d5a7fe8443627e64ea013810 100644 (file)
--- 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 <version> as QUIC version. <ipv4>
  * boolean is set to 1 for IPv4 connection, 0 for IPv6. <server> is set to 1
  * for QUIC servers (or haproxy listeners).
- * <dcid> is the destination connection ID, <scid> is the source connection ID,
- * <token> the token found to be used for this connection with <token_len> as
+ * <dcid> is the destination connection ID, <scid> is the source connection ID.
+ * This latter <scid> CID as the same value on the wire as the one for <conn_id>
+ * 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
+ * <conn_id> in the CIDs tree for this connection (qc->cids).
+ * <token> is the token found to be used for this connection with <token_len> as
  * length. Endpoints addresses are specified via <local_addr> and <peer_addr>.
  * 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, <qc> is the correct connection for this <pkt> Initial
+                                * packet. <conn_id> 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)