From: Amaury Denoyelle Date: Fri, 27 Jan 2023 16:54:15 +0000 (+0100) Subject: MEDIUM: quic: implement a retransmit limit per frame X-Git-Tag: v2.8-dev3~39 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e4abb1f2da6e70e79fe71fae5c762696dcc549b8;p=thirdparty%2Fhaproxy.git MEDIUM: quic: implement a retransmit limit per frame Add a new field in quic_frame structure. This field is set to 0 and incremented each time a sent packet is declared lost. If reached a hard-coded limit, the connection is deemed as failing and is closed immediately with a CONNECTION_CLOSE using INTERNAL_ERROR. By default, limit is set to 10. This should ensure that overall memory usage is limited if a peer behaves incorrectly. This should be backported up to 2.7. --- diff --git a/include/haproxy/quic_conn-t.h b/include/haproxy/quic_conn-t.h index 3ea42ea434..fa4ff54a6d 100644 --- a/include/haproxy/quic_conn-t.h +++ b/include/haproxy/quic_conn-t.h @@ -93,6 +93,8 @@ typedef unsigned long long ull; #define QUIC_RETRY_DURATION_MS 10000 /* Default Retry threshold */ #define QUIC_DFLT_RETRY_THRESHOLD 100 /* in connection openings */ +/* Default limit of loss detection on a single frame. If exceeded, connection is closed. */ +#define QUIC_DFLT_MAX_FRAME_LOSS 10 /* * 0 1 2 3 diff --git a/include/haproxy/quic_frame-t.h b/include/haproxy/quic_frame-t.h index 59014e4dce..3cd11710ad 100644 --- a/include/haproxy/quic_frame-t.h +++ b/include/haproxy/quic_frame-t.h @@ -270,6 +270,7 @@ struct quic_frame { struct list reflist; /* List head containing duplicated children frames. */ struct list ref; /* List elem from parent frame reflist. Set if frame is a duplicate (used for retransmission). */ unsigned int flags; /* QUIC_FL_TX_FRAME_* */ + unsigned int loss_count; /* Counter for each occurence of this frame marked as lost. */ }; diff --git a/include/haproxy/quic_frame.h b/include/haproxy/quic_frame.h index 287be8fd97..ea33425ac8 100644 --- a/include/haproxy/quic_frame.h +++ b/include/haproxy/quic_frame.h @@ -197,14 +197,15 @@ static inline struct quic_frame *qc_frm_alloc(int type) frm->pkt = NULL; frm->origin = NULL; frm->flags = 0; + frm->loss_count = 0; return frm; } /* Allocate a quic_frame by duplicating frame. This will create a new * frame of the same type with the same content. Internal fields such as packet - * owner and flags are however resetted for the newly allocated frame. Frame - * must be freed with qc_frm_free(). + * owner and flags are however resetted for the newly allocated frame except + * for the loss counter. Frame must be freed with qc_frm_free(). * * Returns the allocated frame or NULL on failure. */ @@ -218,7 +219,7 @@ static inline struct quic_frame *qc_frm_dup(struct quic_frame *origin) *frm = *origin; - /* Reinit all internal members. */ + /* Reinit all internal members except loss_count. */ LIST_INIT(&frm->list); LIST_INIT(&frm->reflist); frm->pkt = NULL; diff --git a/src/quic_conn.c b/src/quic_conn.c index ec03fe6ae9..783015d91e 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -1752,17 +1752,22 @@ static inline struct eb64_node *qc_ackrng_pkts(struct quic_conn *qc, return node; } -/* Remove all frames from and reinsert them in the - * same order they have been sent into . +/* Remove all frames from and reinsert them in the same order + * they have been sent into . The loss counter of each frame is + * incremented and checked if it does not exceed retransmission limit. + * + * Returns 1 on success, 0 if a frame loss limit is exceeded. A + * CONNECTION_CLOSE is scheduled in this case. */ -static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, - struct quic_tx_packet *pkt, - struct list *pktns_frm_list) +static inline int qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, + struct quic_tx_packet *pkt, + struct list *pktns_frm_list) { struct quic_frame *frm, *frmbak; struct list tmp = LIST_HEAD_INIT(tmp); struct list *pkt_frm_list = &pkt->frms; uint64_t pn = pkt->pn_node.key; + int close = 0; TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc); @@ -1827,6 +1832,12 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, qc_frm_free(&frm); } else { + if (++frm->loss_count >= QUIC_DFLT_MAX_FRAME_LOSS) { + TRACE_ERROR("retransmission limit reached, closing the connection", QUIC_EV_CONN_PRSAFRM, qc); + quic_set_connection_close(qc, quic_err_transport(QC_ERR_INTERNAL_ERROR)); + close = 1; + } + if (QUIC_FT_STREAM_8 <= frm->type && frm->type <= QUIC_FT_STREAM_F) { /* Mark this STREAM frame as lost. A look up their stream descriptor * will be performed to check the stream is not consumed or released. @@ -1840,7 +1851,9 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, LIST_SPLICE(pktns_frm_list, &tmp); + end: TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc); + return !close; } /* Free TX packet and its attached frames. @@ -1973,19 +1986,20 @@ static inline void qc_release_pktns_frms(struct quic_conn *qc, TRACE_LEAVE(QUIC_EV_CONN_PHPKTS, qc); } -/* Handle list of lost packets detected at handling - * their TX frames. - * Send a packet loss event to the congestion controller if - * in flight packet have been lost. - * Also frees the packet in list. - * Never fails. +/* Handle list of lost packets detected at handling their TX + * frames. Send a packet loss event to the congestion controller if in flight + * packet have been lost. Also frees the packet in list. + * + * Returns 1 on success else 0 if loss limit has been exceeded. A + * CONNECTION_CLOSE was prepared to close the connection ASAP. */ -static inline void qc_release_lost_pkts(struct quic_conn *qc, - struct quic_pktns *pktns, - struct list *pkts, - uint64_t now_us) +static inline int qc_release_lost_pkts(struct quic_conn *qc, + struct quic_pktns *pktns, + struct list *pkts, + uint64_t now_us) { struct quic_tx_packet *pkt, *tmp, *oldest_lost, *newest_lost; + int close = 0; TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc); @@ -2002,7 +2016,8 @@ static inline void qc_release_lost_pkts(struct quic_conn *qc, if (pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING) qc->path->ifae_pkts--; /* Treat the frames of this lost packet. */ - qc_requeue_nacked_pkt_tx_frms(qc, pkt, &pktns->tx.frms); + if (!qc_requeue_nacked_pkt_tx_frms(qc, pkt, &pktns->tx.frms)) + close = 1; LIST_DELETE(&pkt->list); if (!oldest_lost) { oldest_lost = newest_lost = pkt; @@ -2014,27 +2029,29 @@ static inline void qc_release_lost_pkts(struct quic_conn *qc, } } - if (newest_lost) { - /* Sent a congestion event to the controller */ - struct quic_cc_event ev = { }; + if (!close) { + if (newest_lost) { + /* Sent a congestion event to the controller */ + struct quic_cc_event ev = { }; - ev.type = QUIC_CC_EVT_LOSS; - ev.loss.time_sent = newest_lost->time_sent; + ev.type = QUIC_CC_EVT_LOSS; + ev.loss.time_sent = newest_lost->time_sent; - quic_cc_event(&qc->path->cc, &ev); - } + quic_cc_event(&qc->path->cc, &ev); + } - /* If an RTT have been already sampled, has been set. - * We must check if we are experiencing a persistent congestion. - * If this is the case, the congestion controller must re-enter - * slow start state. - */ - if (qc->path->loss.rtt_min && newest_lost != oldest_lost) { - unsigned int period = newest_lost->time_sent - oldest_lost->time_sent; + /* If an RTT have been already sampled, has been set. + * We must check if we are experiencing a persistent congestion. + * If this is the case, the congestion controller must re-enter + * slow start state. + */ + if (qc->path->loss.rtt_min && newest_lost != oldest_lost) { + unsigned int period = newest_lost->time_sent - oldest_lost->time_sent; - if (quic_loss_persistent_congestion(&qc->path->loss, period, - now_ms, qc->max_ack_delay)) - qc->path->cc.algo->slow_start(&qc->path->cc); + if (quic_loss_persistent_congestion(&qc->path->loss, period, + now_ms, qc->max_ack_delay)) + qc->path->cc.algo->slow_start(&qc->path->cc); + } } /* cannot be NULL at this stage because we have ensured @@ -2048,6 +2065,7 @@ static inline void qc_release_lost_pkts(struct quic_conn *qc, leave: TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc); + return !close; } /* Parse ACK frame into from a buffer at address with being at @@ -2154,7 +2172,8 @@ static inline int qc_parse_ack_frm(struct quic_conn *qc, if (!LIST_ISEMPTY(&newly_acked_pkts)) { if (!eb_is_empty(&qel->pktns->tx.pkts)) { qc_packet_loss_lookup(qel->pktns, qc, &lost_pkts); - qc_release_lost_pkts(qc, qel->pktns, &lost_pkts, now_ms); + if (!qc_release_lost_pkts(qc, qel->pktns, &lost_pkts, now_ms)) + goto leave; } qc_treat_newly_acked_pkts(qc, &newly_acked_pkts); if (quic_peer_validated_addr(qc)) @@ -4625,8 +4644,8 @@ struct task *qc_process_timer(struct task *task, void *ctx, unsigned int state) qc_packet_loss_lookup(pktns, qc, &lost_pkts); if (!LIST_ISEMPTY(&lost_pkts)) tasklet_wakeup(qc->wait_event.tasklet); - qc_release_lost_pkts(qc, pktns, &lost_pkts, now_ms); - qc_set_timer(qc); + if (qc_release_lost_pkts(qc, pktns, &lost_pkts, now_ms)) + qc_set_timer(qc); goto out; }