BUG/MEDIUM: quic: fix actconn on quic_conn alloc failure

Since the following commit, quic_conn instances are accounted into
global actconn and compared against maxconn.

  commit 7735cf3
  MEDIUM: quic: count quic_conn instance for maxconn

Increment is always done prior to real allocation to guarantee minimal
resource consumption. Special care is taken to ensure there will always
be one decrement operation for each increment. To help this, decrement
is centralized in quic_conn_release().

This behaves incorrectly in case of an intermediary allocation failure
inside qc_new_conn(). In this case, quic_conn_release() will decrement
actconn. Then, a NULL qc is returned in quic_rx_pkt_retrieve_conn()
which will also decrement the counter on its own error code path.

To properly fix this, actconn incrementation has been moved directly
inside qc_new_conn(). It is thus easier to cover every cases :
* if alloc failure before or on pool_head_quic_conn, actconn is
  decremented manually at the end of qc_new_conn()
* after this step, actconn will be decremented by quic_conn_release()
  either on intermediary alloc failure or on proper connection release

This bug happens on memory allocation failure so it should be rare.
However, its impact is not negligeable as if actconn counter is wrapped
it will block any future connection allocation for both QUIC and TCP.

This must be backported up to 2.6.

src/quic_conn.c

@@ -37,6 +37,7 @@
 #include <haproxy/connection.h>
 #include <haproxy/fd.h>
 #include <haproxy/freq_ctr.h>
+#include <haproxy/frontend.h>
 #include <haproxy/global.h>
 #include <haproxy/h3.h>
 #include <haproxy/hq_interop.h>
@@ -1158,18 +1159,31 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4,
                               int server, int token, void *owner)
 {
 	int i;
-	struct quic_conn *qc;
+	struct quic_conn *qc = NULL;
 	struct listener *l = NULL;
 	struct quic_cc_algo *cc_algo = NULL;
+	unsigned int next_actconn = 0;
 
 	TRACE_ENTER(QUIC_EV_CONN_INIT);
 
+	next_actconn = increment_actconn();
+	if (!next_actconn) {
+		_HA_ATOMIC_INC(&maxconn_reached);
+		TRACE_STATE("maxconn reached", QUIC_EV_CONN_INIT);
+		goto err;
+	}
+
 	qc = pool_alloc(pool_head_quic_conn);
 	if (!qc) {
 		TRACE_ERROR("Could not allocate a new connection", QUIC_EV_CONN_INIT);
 		goto err;
 	}
 
+	/* Now that quic_conn instance is allocated, quic_conn_release() will
+	 * ensure global accounting is decremented.
+	 */
+	next_actconn = 0;
+
 	/* Initialize in priority qc members required for a safe dealloc. */
 	qc->nictx = NULL;
 	/* Prevents these CID to be dumped by TRACE() calls */
@@ -1361,6 +1375,14 @@ struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4,
 
  err:
 	quic_conn_release(qc);
+
+	/* Decrement global counters. Useful only for errors happening before
+	 * or on pool_head_quic_conn alloc. All other failures are covered by
+	 * quic_conn_release().
+	 */
+	if (next_actconn)
+		_HA_ATOMIC_DEC(&actconn);
+
 	TRACE_LEAVE(QUIC_EV_CONN_INIT);
 	return NULL;
 }
@@ -1435,12 +1457,6 @@ void quic_conn_release(struct quic_conn *qc)
 		qc_free_ssl_sock_ctx(&qc->xprt_ctx);
 	}
 
-	/* Decrement on quic_conn free. quic_cc_conn instances are not counted
-	 * into global counters because they are designed to run for a limited
-	 * time with a limited memory.
-	 */
-	_HA_ATOMIC_DEC(&actconn);
-
 	/* in the unlikely (but possible) case the connection was just added to
 	 * the accept_list we must delete it from there.
 	 */
@@ -1498,6 +1514,12 @@ void quic_conn_release(struct quic_conn *qc)
 	pool_free(pool_head_quic_conn, qc);
 	qc = NULL;
 
+	/* Decrement global counters when quic_conn is deallocated.
+	 * quic_cc_conn instances are not accounted as they run for a short
+	 * time with limited ressources.
+	 */
+	_HA_ATOMIC_DEC(&actconn);
+
 	TRACE_PROTO("QUIC conn. freed", QUIC_EV_CONN_FREED, qc);
  leave:
 	TRACE_LEAVE(QUIC_EV_CONN_CLOSE, qc);

src/quic_rx.c

@@ -14,7 +14,6 @@
 
 #include <haproxy/quic_rx.h>
 
-#include <haproxy/frontend.h>
 #include <haproxy/h3.h>
 #include <haproxy/list.h>
 #include <haproxy/ncbuf.h>
@@ -1903,7 +1902,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
 	struct quic_conn *qc = NULL;
 	struct proxy *prx;
 	struct quic_counters *prx_counters;
-	unsigned int next_actconn = 0, next_sslconn = 0;
+	unsigned int next_sslconn = 0;
 
 	TRACE_ENTER(QUIC_EV_CONN_LPKT);
 
@@ -1961,14 +1960,6 @@ 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;
 
-			next_actconn = increment_actconn();
-			if (!next_actconn) {
-				_HA_ATOMIC_INC(&maxconn_reached);
-				TRACE_STATE("drop packet on maxconn reached",
-				            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
-				goto err;
-			}
-
 			next_sslconn = increment_sslconn();
 			if (!next_sslconn) {
 				TRACE_STATE("drop packet on sslconn reached",
@@ -1994,13 +1985,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
 				goto err;
 			}
 
-			/* Now quic_conn is allocated. If a future error
-			 * occurred it will be freed with quic_conn_release()
-			 * which also ensure actconn/sslconns is decremented.
-			 * Reset guard values to prevent a double decrement.
-			 */
-			next_sslconn = next_actconn = 0;
-
+			next_sslconn = 0;
 			/* 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,
@@ -2051,9 +2036,6 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
 	else
 		HA_ATOMIC_INC(&prx_counters->dropped_pkt);
 
-	/* Reset active conn counter if needed. */
-	if (next_actconn)
-		_HA_ATOMIC_DEC(&actconn);
 	if (next_sslconn)
 		_HA_ATOMIC_DEC(&global.sslconns);