From: Amaury Denoyelle Date: Mon, 6 Nov 2023 16:45:14 +0000 (+0100) Subject: BUG/MEDIUM: quic: fix actconn on quic_conn alloc failure X-Git-Tag: v2.9-dev10~146 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a7ba679fe7b6511133441b3250e53a03ace7ed92;p=thirdparty%2Fhaproxy.git 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 750f9018e1..7f300c9d03 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -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 ad6f62c812..da98403a44 100644 --- a/src/quic_rx.c +++ b/src/quic_rx.c @@ -14,7 +14,6 @@ #include -#include #include #include #include @@ -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);