From 2b2499ce6f8c3efb7778020b1237f4ccbeb56a63 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 11 Aug 2025 18:04:59 +0200 Subject: [PATCH] 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. (cherry picked from commit 7d554ca6295033cbf1b82a15980ae8c0a0c94ec3) Signed-off-by: Christopher Faulet (cherry picked from commit fc284fc80e976f2ada70e309677b413f72526b78) Signed-off-by: Christopher Faulet (cherry picked from commit ea53b6930ee9151a1a451a825fc53ad31e0c5bf9) [ad: fix conflicts mostly due to non GSO support] Signed-off-by: Amaury Denoyelle --- src/quic_tx.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/quic_tx.c b/src/quic_tx.c index 65b1917..d66775f 100644 --- a/src/quic_tx.c +++ b/src/quic_tx.c @@ -537,6 +537,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, int err, probe, must_ack; enum quic_pkt_type pkt_type; struct quic_tx_packet *cur_pkt; + int final_packet = 0; TRACE_PROTO("TX prep pkts", QUIC_EV_CONN_PHPKTS, qc, qel); probe = 0; @@ -599,10 +600,19 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, 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, - padding && !next_qel, + padding && (!next_qel || final_packet), probe, cc, &err); switch (err) { case -3: @@ -653,7 +663,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, * the same datagram, except if is the Application data * encryption level which cannot be selected to do that. */ - if (LIST_ISEMPTY(frms) && next_qel) { + if (LIST_ISEMPTY(frms) && next_qel && !final_packet) { prv_pkt = cur_pkt; } else { -- 1.7.10.4