From a60609f1aa3e5f61d2a2286fdb40ebf6936a80ee Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Thu, 30 May 2024 18:06:27 +0200 Subject: [PATCH] BUG/MINOR: quic: fix padding of INITIAL packets API for sending has been extended to support emission on more than 2 QEL instances. However, this has rendered the PADDING emission for INITIAL packets less previsible. Indeed, if qc_send() is used with empty QEL instances, a padding frame may be generated before handling the last QEL registered, which could cause unnecessary padding to be emitted. This commit simplify PADDING by only activating it for the last QEL registered. This ensures that no superfluous padding is generated as if the minimal INITIAL datagram length is reached, padding is resetted before handling last QEL instance. This bug is labelled as minor as haproxy already emit big enough INITIAL packets coalesced with HANDSHAKE one without needing padding. This however render the padding code difficult to test. Thus, it may be useful to force emission on INITIAL qel only without coalescing HANDSHAKE packet. Here is a sample to reproduce it : --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -794,6 +794,14 @@ struct task *quic_conn_io_cb(struct task *t, void *context, unsigned int state) } } + if (qc->iel && qel_need_sending(qc->iel, qc)) { + struct list empty = LIST_HEAD_INIT(empty); + qel_register_send(&send_list, qc->iel, &qc->iel->pktns->tx.frms); + if (qc->hel) + qel_register_send(&send_list, qc->hel, &empty); + qc_send(qc, 0, &send_list); + } + /* Insert each QEL into sending list if needed. */ list_for_each_entry(qel, &qc->qel_list, list) { if (qel_need_sending(qel, qc)) This should be backported up to 3.0. --- src/quic_tx.c | 77 +++++++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 40 deletions(-) diff --git a/src/quic_tx.c b/src/quic_tx.c index 39b917652..84e9f3267 100644 --- a/src/quic_tx.c +++ b/src/quic_tx.c @@ -155,16 +155,27 @@ static void qc_txb_store(struct buffer *buf, uint16_t length, const size_t hdlen = sizeof(uint16_t) + sizeof(void *); BUG_ON_HOT(b_contig_space(buf) < hdlen); /* this must not happen */ + /* If first packet is INITIAL, ensure datagram is sufficiently padded. */ + BUG_ON(first_pkt->type == QUIC_PACKET_TYPE_INITIAL && + (first_pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING) && + length < QUIC_INITIAL_PACKET_MINLEN); + write_u16(b_tail(buf), length); write_ptr(b_tail(buf) + sizeof(length), first_pkt); b_add(buf, hdlen + length); } -/* Returns 1 if a packet may be built for from encryption level - * with as ack-eliciting frame list to send, 0 if not. - * must equal to 1 if an immediate close was asked, 0 if not. - * must equalt to 1 if a probing packet is required, 0 if not. - * Also set <*must_ack> to inform the caller if an acknowledgement should be sent. +/* Reports if data are ready to be sent for encryption level on + * connection. + * + * is the ack-eliciting frames list to send, if any. Other parameters + * can be set individually for some special frame types : for immediate + * close, to emit probing frames. + * + * This function will also set to inform the caller that an + * acknowledgement should be sent. + * + * Returns true if data to emit else false. */ static int qc_may_build_pkt(struct quic_conn *qc, struct list *frms, struct quic_enc_level *qel, int cc, int probe, @@ -330,15 +341,7 @@ static int qc_send_ppkts(struct buffer *buf, struct ssl_sock_ctx *ctx) for (pkt = first_pkt; pkt; pkt = next_pkt) { struct quic_cc *cc = &qc->path->cc; - /* RFC 9000 14.1 Initial datagram size - * a server MUST expand the payload of all UDP datagrams carrying ack-eliciting - * Initial packets to at least the smallest allowed maximum datagram size of - * 1200 bytes. - */ qc->cntrs.sent_pkt++; - BUG_ON_HOT(pkt->type == QUIC_PACKET_TYPE_INITIAL && - (pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING) && - dglen < QUIC_INITIAL_PACKET_MINLEN); pkt->time_sent = time_sent; if (pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING) { @@ -510,7 +513,7 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, list_for_each_entry_safe(qel, tmp_qel, qels, el_send) { struct quic_tls_ctx *tls_ctx; const struct quic_version *ver; - struct list *frms = qel->send_frms, *next_frms; + struct list *frms = qel->send_frms; struct quic_enc_level *next_qel; if (qel == qc->eel) { @@ -521,14 +524,9 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, qc_select_tls_ver(qc, qel, &tls_ctx, &ver); /* Retrieve next QEL. Set it to NULL if on qels last element. */ - if (qel->el_send.n != qels) { - next_qel = LIST_ELEM(qel->el_send.n, struct quic_enc_level *, el_send); - next_frms = next_qel->send_frms; - } - else { - next_qel = NULL; - next_frms = NULL; - } + next_qel = LIST_NEXT(&qel->el_send, struct quic_enc_level *, el_send); + if (&next_qel->el_send == qels) + next_qel = NULL; /* Build as much as datagrams at encryption level. * Each datagram is prepended with its length followed by the address @@ -546,7 +544,9 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, if (!cc) probe = qel->pktns->tx.pto_probe; - if (!qc_may_build_pkt(qc, frms, qel, cc, probe, &must_ack)) { + /* Remove QEL if nothing to send anymore. Padding is only emitted for last QEL. */ + if (!qc_may_build_pkt(qc, frms, qel, cc, probe, &must_ack) && + (!padding || next_qel)) { /* Remove qel from send_list if nothing to send. */ LIST_DEL_INIT(&qel->el_send); qel->send_frms = NULL; @@ -577,30 +577,28 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, } /* RFC 9000 14.1 Initial datagram size - * a server MUST expand the payload of all UDP datagrams carrying ack-eliciting - * Initial packets to at least the smallest allowed maximum datagram size of - * 1200 bytes. * - * Ensure that no ack-eliciting packets are sent into too small datagrams + * Similarly, a server MUST expand the payload of all UDP + * datagrams carrying ack-eliciting Initial packets to at least the + * smallest allowed maximum datagram size of 1200 bytes. */ - if (qel == qc->iel && !LIST_ISEMPTY(frms)) { + if (qel == qc->iel && (!LIST_ISEMPTY(frms) || probe)) { + /* Ensure that no ack-eliciting packets are sent into too small datagrams */ if (end - pos < QUIC_INITIAL_PACKET_MINLEN) { TRACE_PROTO("No more enough room to build an Initial packet", QUIC_EV_CONN_PHPKTS, qc); break; } - /* Pad this Initial packet if there is no ack-eliciting frames to send from - * the next packet number space. - */ - if (!next_frms || LIST_ISEMPTY(next_frms)) - padding = 1; + /* padding will be set for last QEL */ + padding = 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, probe, cc, &err); + must_ack, padding && !next_qel, + probe, cc, &err); switch (err) { case -3: if (first_pkt) @@ -628,6 +626,10 @@ static int qc_prep_pkts(struct quic_conn *qc, struct buffer *buf, total += cur_pkt->len; dglen += cur_pkt->len; + /* Reset padding if datagram is big enough. */ + if (dglen >= QUIC_INITIAL_PACKET_MINLEN) + padding = 0; + if (qc->flags & QUIC_FL_CONN_RETRANS_OLD_DATA) cur_pkt->flags |= QUIC_FL_TX_PACKET_PROBE_WITH_OLD_DATA; @@ -647,12 +649,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) && qel != qc->ael && next_qel) { - if (qel == qc->iel && - (!qc_is_listener(qc) || - cur_pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING)) - padding = 1; - + if (LIST_ISEMPTY(frms) && next_qel) { prv_pkt = cur_pkt; } else {