BUG/MINOR: quic: don't coalesce probing and ACK packet of same type

Haproxy QUIC stack suffers from a limitation : it's not possible to emit
a packet which contains probing data and a ACK frame in it. Thus, in
case qc_do_build_pkt() is invoked which both values as true, probing has
the priority and ACK is ignored.

However, this has the undesired side-effect of possibly generating two
coalesced packets of the same type in the same datagram : the first one
with the probing data and the second with an ACK frame. This is caused
by qc_prep_pkts() loop which may call qc_do_build_pkt() multiple times
with the same QEL instance. This case is normally use when a full
datagram has been built but there is still content to emit on the
current encryption level.

To fix this, alter qc_prep_pkts() loop : if both probing and ACK is
requested, force the datagram to be written after packet encoding. This
will result in a datagram containing the packet with probing data as
final entry. A new datagram is started for the next packet which will
can contain the ACK frame.

This also has some impact on INITIAL padding. Indeed, if packet must be
the last due to probing emission, qc_prep_pkts() will also activate
padding to ensure final datagram is at least 1.200 bytes long.

Note that coalescing two packets of the same type is not invalid
according to QUIC RFC. However it could cause issue with some shaky
implementations, so it is considered as a bug.

This must be backported up to 2.6.

diff --git a/src/quic_tx.c b/src/quic_tx.c
index 99a51d92c6bef7642165e4af293f7f858b266d34..695841732f0269b186770fdbfb80d0c1f4fb9797 100644 (file)
--- a/src/quic_tx.c
+++ b/src/quic_tx.c
@@ -648,6 +648,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
                        enum quic_pkt_type pkt_type;
                        struct quic_tx_packet *cur_pkt;
                        enum qc_build_pkt_err err;
                        enum quic_pkt_type pkt_type;
                        struct quic_tx_packet *cur_pkt;
                        enum qc_build_pkt_err err;

+                       int final_packet = 0;

 
                        TRACE_PROTO("TX prep pkts", QUIC_EV_CONN_PHPKTS, qc, qel);
 
 
                        TRACE_PROTO("TX prep pkts", QUIC_EV_CONN_PHPKTS, qc, qel);
 


@@ -706,10 +707,19 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
                                padding = 1;
                        }
 
                                padding = 1;
                        }
 

+
+                       /* TODO currently it's not possible to emit an ACK and probing data simultaneously (see qc_do_build_pkt()).
+                        * As a side-effect, this could cause coalescing of two packets of the same type which should be avoided.
+                        * To implement this, a new datagram is forced by invokation of qc_txb_store(). This must then be checked
+                        * if padding is required as in this case this will be the last packet of the current datagram.
+                        */
+                       if (probe && (must_ack || (qel->pktns->flags & QUIC_FL_PKTNS_ACK_REQUIRED)))
+                               final_packet = 1;
+

                        pkt_type = quic_enc_level_pkt_type(qc, qel);
                        cur_pkt = qc_build_pkt(&pos, end, qel, tls_ctx, frms,
                                               qc, ver, dglen, pkt_type, must_ack,
                        pkt_type = quic_enc_level_pkt_type(qc, qel);
                        cur_pkt = qc_build_pkt(&pos, end, qel, tls_ctx, frms,
                                               qc, ver, dglen, pkt_type, must_ack,

-                                              padding && !next_qel,
+                                              padding && (!next_qel || final_packet),

                                               probe, cc, &err);
                        if (!cur_pkt) {
                                switch (err) {
                                               probe, cc, &err);
                        if (!cur_pkt) {
                                switch (err) {


@@ -787,7 +797,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf,
                        if (probe && qel == qc->ael)
                                break;
 
                        if (probe && qel == qc->ael)
                                break;
 

-                       if (LIST_ISEMPTY(frms)) {
+                       if (LIST_ISEMPTY(frms) && !final_packet) {

                                /* Everything sent. Continue within the same datagram. */
                                prv_pkt = cur_pkt;
                        }
                                /* Everything sent. Continue within the same datagram. */
                                prv_pkt = cur_pkt;
                        }