BUG/MINOR: quic: reorder fragmented RX CRYPTO frames by their offsets

This issue impacts the QUIC listeners. It is the same as the one fixed by this
commit:

	BUG/MINOR: quic: repeat packet parsing to deal with fragmented CRYPTO

As chrome, ngtcp2 client decided to fragment its CRYPTO frames but in a much
more agressive way. This could be fixed with a list local to qc_parse_pkt_frms()
to please chrome thanks to the commit above. But this is not sufficient for
ngtcp2 which often splits its ClientHello message into more than 10 fragments
with very small ones. This leads the packet parser to interrupt the CRYPTO frames
parsing due to the ncbuf gap size limit.

To fix this, this patch approximatively proceeds the same way but with an
ebtree to reorder the CRYPTO by their offsets. These frames are directly
inserted into a local ebtree. Then this ebtree is reused to provide the
reordered CRYPTO data to the underlying ncbuf (non contiguous buffer). This way
there are very few less chances for the ncbufs used to store CRYPTO data
to reach a too much fragmented state.

Must be backported as far as 2.6.

(cherry picked from commit d753f24096dafdef227fe66621513086c2c663c1)
[cf: ctx adjt]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9146b06962565992724ec12e35b3b77bb3274805)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit b673c57a165614598919f3b7ccfe1344bc7e3d97)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

diff --git a/include/haproxy/quic_frame-t.h b/include/haproxy/quic_frame-t.h
index fe80f27..6efea0e 100644 (file)
--- a/include/haproxy/quic_frame-t.h
+++ b/include/haproxy/quic_frame-t.h
@@ -148,6 +148,7 @@ struct qf_stop_sending {
 struct qf_crypto {
        struct list list;
        uint64_t offset;
+       struct eb64_node offset_node;
        uint64_t len;
        const struct quic_enc_level *qel;
        const unsigned char *data;

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 5cb7301..ccb970c 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -833,13 +833,12 @@ static inline unsigned int quic_ack_delay_ms(struct qf_ack *ack_frm,
 static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                              struct quic_enc_level *qel)
 {
-       struct list retry_frms = LIST_HEAD_INIT(retry_frms);
-       struct quic_frame *frm = NULL, *frm_tmp;
+       struct eb_root cf_frms_tree = EB_ROOT;
+       struct eb64_node *node;
+       struct quic_frame *frm = NULL;
        const unsigned char *pos, *end;
        enum quic_rx_ret_frm ret;
        int fast_retrans = 0;
-       /* parsing may be rerun multiple times, but no more than <iter>. */
-       int iter = 3, parsing_stage = 0;
 
        TRACE_ENTER(QUIC_EV_CONN_PRSHPKT, qc);
        /* Skip the AAD */
@@ -917,36 +916,9 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
                        break;
                }
                case QUIC_FT_CRYPTO:
-                       ret = qc_handle_crypto_frm(qc, &frm->crypto, pkt, qel);
-                       switch (ret) {
-                       case QUIC_RX_RET_FRM_FATAL:
-                               goto err;
-
-                       case QUIC_RX_RET_FRM_AGAIN:
-                               if (parsing_stage == 0) {
-                                       TRACE_STATE("parsing stage set to 1 (AGAIN encountered)", QUIC_EV_CONN_PRSHPKT, qc);
-                                       ++parsing_stage;
-                               }
-                               /* Save frame in temp list to reparse it later. A new instance must be used for next packet frames. */
-                               LIST_APPEND(&retry_frms, &frm->list);
-                               frm = NULL;
-                               break;
-
-                       case QUIC_RX_RET_FRM_DUP:
-                               if (qc_is_listener(qc) && qel == qc->iel &&
-                                   !(qc->flags & QUIC_FL_CONN_HANDSHAKE_SPEED_UP)) {
-                                       fast_retrans = 1;
-                               }
-                               break;
-
-                       case QUIC_RX_RET_FRM_DONE:
-                               if (parsing_stage == 1) {
-                                       TRACE_STATE("parsing stage set to 2 (DONE after AGAIN)", QUIC_EV_CONN_PRSHPKT, qc);
-                                       ++parsing_stage;
-                               }
-                               break;
-                       }
-
+                       frm->crypto.offset_node.key = frm->crypto.offset;
+                       eb64_insert(&cf_frms_tree, &frm->crypto.offset_node);
+                       frm = NULL;
                        break;
                case QUIC_FT_NEW_TOKEN:
                        if (qc_is_listener(qc)) {
@@ -1115,57 +1087,39 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
        if (frm)
                qc_frm_free(qc, &frm);
 
-       while (!LIST_ISEMPTY(&retry_frms)) {
-               if (--iter <= 0) {
-                       TRACE_ERROR("interrupt parsing due to max iteration reached",
-                                   QUIC_EV_CONN_PRSHPKT, qc);
-                       goto err;
-               }
-               else if (parsing_stage <= 1) {
-                       TRACE_ERROR("interrupt parsing due to buffering blocked on gap size limit",
-                                   QUIC_EV_CONN_PRSHPKT, qc);
+       node = eb64_first(&cf_frms_tree);
+       while (node) {
+               frm = eb64_entry(node, struct quic_frame, crypto.offset_node);
+               /* only CRYPTO frames may be reparsed for now */
+               BUG_ON(frm->type != QUIC_FT_CRYPTO);
+               node = eb64_next(node);
+               ret = qc_handle_crypto_frm(qc, &frm->crypto, pkt, qel);
+               switch (ret) {
+               case QUIC_RX_RET_FRM_FATAL:
                        goto err;
-               }
 
-               parsing_stage = 0;
-               list_for_each_entry_safe(frm, frm_tmp, &retry_frms, list) {
-                       /* only CRYPTO frames may be reparsed for now */
-                       BUG_ON(frm->type != QUIC_FT_CRYPTO);
-                       ret = qc_handle_crypto_frm(qc, &frm->crypto, pkt, qel);
-                       switch (ret) {
-                       case QUIC_RX_RET_FRM_FATAL:
-                               goto err;
+               case QUIC_RX_RET_FRM_AGAIN:
+                       TRACE_STATE("AGAIN encountered", QUIC_EV_CONN_PRSHPKT, qc);
+                       goto err;
 
-                       case QUIC_RX_RET_FRM_AGAIN:
-                               if (parsing_stage == 0) {
-                                       TRACE_STATE("parsing stage set to 1 (AGAIN encountered)", QUIC_EV_CONN_PRSHPKT, qc);
-                                       ++parsing_stage;
-                               }
-                               break;
+               case QUIC_RX_RET_FRM_DONE:
+                       TRACE_PROTO("frame handled", QUIC_EV_CONN_PRSAFRM, qc, frm);
+                       break;
 
-                       case QUIC_RX_RET_FRM_DONE:
-                               TRACE_PROTO("frame handled after a new parsing iteration",
-                                           QUIC_EV_CONN_PRSAFRM, qc, frm);
-                               if (parsing_stage == 1) {
-                                       TRACE_STATE("parsing stage set to 2 (DONE after AGAIN)", QUIC_EV_CONN_PRSHPKT, qc);
-                                       ++parsing_stage;
-                               }
-                               __fallthrough;
-                       case QUIC_RX_RET_FRM_DUP:
-                               qc_frm_free(qc, &frm);
-                               break;
+               case QUIC_RX_RET_FRM_DUP:
+                       if (qc_is_listener(qc) && qel == qc->iel &&
+                               !(qc->flags & QUIC_FL_CONN_HANDSHAKE_SPEED_UP)) {
+                               fast_retrans = 1;
                        }
+                       break;
                }
 
-               /* Always reset <frm> as it may be dangling after
-                * list_for_each_entry_safe() usage. Especially necessary to
-                * prevent a crash if loop is interrupted on max iteration.
-                */
-               frm = NULL;
+               eb64_delete(&frm->crypto.offset_node);
+               qc_frm_free(qc, &frm);
        }
 
        /* Error should be returned if some frames cannot be parsed. */
-       BUG_ON(!LIST_ISEMPTY(&retry_frms));
+       BUG_ON(!eb_is_empty(&cf_frms_tree));
 
        if (fast_retrans && qc->iel && qc->hel) {
                struct quic_enc_level *iqel = qc->iel;
@@ -1201,7 +1155,11 @@ static int qc_parse_pkt_frms(struct quic_conn *qc, struct quic_rx_packet *pkt,
  err:
        if (frm)
                qc_frm_free(qc, &frm);
-       list_for_each_entry_safe(frm, frm_tmp, &retry_frms, list) {
+       node = eb64_first(&cf_frms_tree);
+       while (node) {
+               frm = eb64_entry(node, struct quic_frame, crypto.offset_node);
+               node = eb64_next(node);
+               eb64_delete(&frm->crypto.offset_node);
                qc_frm_free(qc, &frm);
        }