RepoMirrors
/
haproxy
mirror of http://git.haproxy.org/git/haproxy.git/
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

BUG/MINOR: quic: Missing acknowledgments for trailing packets 

This bug was revealed by key_update QUIC tracker test. During this test,
after the handshake has succeeded, the client sends a 1-RTT packet containing
only a PING frame. On our side, we do not acknowledge it, because we have
no ack-eliciting packet to send. This is not correct. We must acknowledge all
the ack-eliciting packets unconditionally. But we must not send too much
ACK frames: every two packets since we have sent an ACK frame. This is the test
(nb_aepkts_since_last_ack >= QUIC_MAX_RX_AEPKTS_SINCE_LAST_ACK) which ensure
this is the case. But this condition must be checked at the very last time,
when we are building a packet and when an acknowledgment is required. This
is not to qc_may_build_pkt() to do that. This boolean function decides if
we have packets to send. It must return "true" if there is no more ack-eliciting
packets to send and if an acknowledgement is required.

We must also add a "force_ack" parameter to qc_build_pkt() to force the
acknowledments during the handshakes (for each packet). If not, they will
be sent every two packets. This parameter must be passed to qc_do_build_pkt()
and be checked alongside the others conditions when building a packet to
decide to add an ACK frame or not to this packet.

Must be backported to 2.6.
This commit is contained in:
Frédéric Lécaille 2022-06-21 15:14:59 +02:00
parent 1bad7db4a1
commit 77ac6f5667
1 changed files with 17 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
src/xprt_quic.c
View File

@ -211,7 +211,7 @@ static struct quic_tx_packet *qc_build_pkt(unsigned char **pos, const unsigned c
struct quic_enc_level *qel, struct quic_tls_ctx *ctx, struct quic_enc_level *qel, struct quic_tls_ctx *ctx,
struct list *frms, struct quic_conn *qc, struct list *frms, struct quic_conn *qc,
const struct quic_version *ver, size_t dglen, int pkt_type, const struct quic_version *ver, size_t dglen, int pkt_type,
int padding, int probe, int cc, int *err); int force_ack, int padding, int probe, int cc, int *err);
static struct task *quic_conn_app_io_cb(struct task *t, void *context, unsigned int state); static struct task *quic_conn_app_io_cb(struct task *t, void *context, unsigned int state);
static void qc_idle_timer_do_rearm(struct quic_conn *qc); static void qc_idle_timer_do_rearm(struct quic_conn *qc);
static void qc_idle_timer_rearm(struct quic_conn *qc, int read); static void qc_idle_timer_rearm(struct quic_conn *qc, int read);
@ -2687,7 +2687,7 @@ static int qc_may_build_pkt(struct quic_conn *qc, struct list *frms,
struct quic_enc_level *qel, int cc, int probe, int force_ack) struct quic_enc_level *qel, int cc, int probe, int force_ack)
{ {
unsigned int must_ack = force_ack || unsigned int must_ack = force_ack ||
qel->pktns->rx.nb_aepkts_since_last_ack >= QUIC_MAX_RX_AEPKTS_SINCE_LAST_ACK; (LIST_ISEMPTY(frms) && (qel->pktns->flags & QUIC_FL_PKTNS_ACK_REQUIRED));
/* Do not build any more packet if the TX secrets are not available or /* Do not build any more packet if the TX secrets are not available or
* if there is nothing to send, i.e. if no CONNECTION_CLOSE or ACK are required * if there is nothing to send, i.e. if no CONNECTION_CLOSE or ACK are required
@ -2760,8 +2760,8 @@ static int qc_prep_app_pkts(struct quic_conn *qc, struct qring *qr,
end = pos + qc->path->mtu; end = pos + qc->path->mtu;
} }
pkt = qc_build_pkt(&pos, end, qel, &qel->tls_ctx, frms, qc, NULL, 0, 0, pkt = qc_build_pkt(&pos, end, qel, &qel->tls_ctx, frms, qc, NULL, 0,
QUIC_PACKET_TYPE_SHORT, probe, cc, &err); QUIC_PACKET_TYPE_SHORT, 0, 0, probe, cc, &err);
switch (err) { switch (err) {
case -2: case -2:
goto err; goto err;
@ -2901,7 +2901,8 @@ static int qc_prep_pkts(struct quic_conn *qc, struct qring *qr,
} }
cur_pkt = qc_build_pkt(&pos, end, qel, tls_ctx, frms, cur_pkt = qc_build_pkt(&pos, end, qel, tls_ctx, frms,
qc, ver, dglen, padding, pkt_type, probe, cc, &err); qc, ver, dglen, pkt_type,
force_ack, padding, probe, cc, &err);
switch (err) { switch (err) {
case -2: case -2:
goto err; goto err;
@ -6016,7 +6017,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end, static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
size_t dglen, struct quic_tx_packet *pkt, size_t dglen, struct quic_tx_packet *pkt,
int64_t pn, size_t *pn_len, unsigned char **buf_pn, int64_t pn, size_t *pn_len, unsigned char **buf_pn,
int padding, int cc, int probe, int force_ack, int padding, int cc, int probe,
struct quic_enc_level *qel, struct quic_conn *qc, struct quic_enc_level *qel, struct quic_conn *qc,
const struct quic_version *ver, struct list *frms) const struct quic_version *ver, struct list *frms)
{ {
@ -6030,6 +6031,8 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
int add_ping_frm; int add_ping_frm;
struct list frm_list = LIST_HEAD_INIT(frm_list); struct list frm_list = LIST_HEAD_INIT(frm_list);
struct quic_frame *cf; struct quic_frame *cf;
int must_ack;
int nb_aepkts_since_last_ack;
/* Length field value with CRYPTO frames if present. */ /* Length field value with CRYPTO frames if present. */
len_frms = 0; len_frms = 0;
@ -6070,7 +6073,11 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end,
head_len = pos - beg; head_len = pos - beg;
/* Build an ACK frame if required. */ /* Build an ACK frame if required. */
ack_frm_len = 0; ack_frm_len = 0;
if ((qel->pktns->flags & QUIC_FL_PKTNS_ACK_REQUIRED) && !qel->pktns->tx.pto_probe) { nb_aepkts_since_last_ack = qel->pktns->rx.nb_aepkts_since_last_ack;
must_ack = !qel->pktns->tx.pto_probe &&
(force_ack || ((qel->pktns->flags & QUIC_FL_PKTNS_ACK_REQUIRED) &&
(LIST_ISEMPTY(frms) || nb_aepkts_since_last_ack >= QUIC_MAX_RX_AEPKTS_SINCE_LAST_ACK)));
if (force_ack || must_ack) {
struct quic_arngs *arngs = &qel->pktns->rx.arngs; struct quic_arngs *arngs = &qel->pktns->rx.arngs;
BUG_ON(eb_is_empty(&qel->pktns->rx.arngs.root)); BUG_ON(eb_is_empty(&qel->pktns->rx.arngs.root));
ack_frm.tx_ack.arngs = arngs; ack_frm.tx_ack.arngs = arngs;
@ -6273,8 +6280,8 @@ static struct quic_tx_packet *qc_build_pkt(unsigned char **pos,
struct quic_enc_level *qel, struct quic_enc_level *qel,
struct quic_tls_ctx *tls_ctx, struct list *frms, struct quic_tls_ctx *tls_ctx, struct list *frms,
struct quic_conn *qc, const struct quic_version *ver, struct quic_conn *qc, const struct quic_version *ver,
size_t dglen, int padding, size_t dglen, int pkt_type, int force_ack,
int pkt_type, int probe, int cc, int *err) int padding, int probe, int cc, int *err)
{ {
/* The pointer to the packet number field. */ /* The pointer to the packet number field. */
unsigned char *buf_pn; unsigned char *buf_pn;
@ -6299,7 +6306,7 @@ static struct quic_tx_packet *qc_build_pkt(unsigned char **pos,
pn = qel->pktns->tx.next_pn + 1; pn = qel->pktns->tx.next_pn + 1;
if (!qc_do_build_pkt(*pos, buf_end, dglen, pkt, pn, &pn_len, &buf_pn, if (!qc_do_build_pkt(*pos, buf_end, dglen, pkt, pn, &pn_len, &buf_pn,
padding, cc, probe, qel, qc, ver, frms)) { force_ack, padding, cc, probe, qel, qc, ver, frms)) {
*err = -1; *err = -1;
goto err; goto err;
} }