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 7735cf3854eb155a50a5ea747406f2a25657e25c
  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.

One small downside of this change is that a CID is now always allocated
before quic_conn even if maxconn will be reached. However, this is
considered as of minor importance compared to a more robust code.

This must be backported up to 2.6.

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 750f9018e1bd1d4636919f66c8ad6d0e03e2c8ff..7f300c9d038eda63b0be4cdfd9915707cccb7784 100644 (file)
--- a/src/quic_conn.c
+++ b/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. Done only for errors happening before or
+        * on pool_head_quic_conn alloc. All other cases 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);

diff --git a/src/quic_rx.c b/src/quic_rx.c
index ad6f62c81213e73d41c63556a5161a54f8b7ac99..da98403a445392fe7251964ed0431ee39c345c7d 100644 (file)
--- a/src/quic_rx.c
+++ b/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);