From: Amaury Denoyelle Date: Thu, 2 Feb 2023 15:12:09 +0000 (+0100) Subject: MINOR: quic: refactor frame deallocation X-Git-Tag: v2.8-dev3~40 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=57b3eaa79365a6cc6c951bf44348d313cef785ce;p=thirdparty%2Fhaproxy.git MINOR: quic: refactor frame deallocation Define a new function qc_frm_free() to handle frame deallocation. New BUG_ON() statements ensure that the deallocated frame is not referenced by other frame. To support this, all LIST_DELETE() have been replaced by LIST_DEL_INIT(). This should enforce that frame deallocation is robust. As a complement, qc_frm_unref() has been moved into quic_frame module. It is justified as this is a utility function related to frame deallocation. It allows to use it in quic_pktns_tx_pkts_release() before calling qc_frm_free(). This should be backported up to 2.7. --- diff --git a/include/haproxy/quic_conn.h b/include/haproxy/quic_conn.h index b14765ba55..60afccd8de 100644 --- a/include/haproxy/quic_conn.h +++ b/include/haproxy/quic_conn.h @@ -532,9 +532,10 @@ static inline void quic_pktns_tx_pkts_release(struct quic_pktns *pktns, struct q if (pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING) qc->path->ifae_pkts--; list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) { - LIST_DELETE(&frm->list); + qc_frm_unref(frm, qc); + LIST_DEL_INIT(&frm->list); quic_tx_packet_refdec(frm->pkt); - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); } eb64_delete(&pkt->pn_node); quic_tx_packet_refdec(pkt); diff --git a/include/haproxy/quic_frame.h b/include/haproxy/quic_frame.h index 2b5286ff22..287be8fd97 100644 --- a/include/haproxy/quic_frame.h +++ b/include/haproxy/quic_frame.h @@ -176,7 +176,8 @@ static inline struct quic_err quic_err_app(uint64_t code) return (struct quic_err){ .code = code, .app = 1 }; } -/* Allocate a quic_frame with type . +/* Allocate a quic_frame with type . Frame must be freed with + * qc_frm_free(). * * Returns the allocated frame or NULL on failure. */ @@ -202,7 +203,8 @@ static inline struct quic_frame *qc_frm_alloc(int type) /* 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. + * owner and flags are however resetted for the newly allocated frame. Frame + * must be freed with qc_frm_free(). * * Returns the allocated frame or NULL on failure. */ @@ -229,5 +231,27 @@ static inline struct quic_frame *qc_frm_dup(struct quic_frame *origin) return frm; } +void qc_frm_unref(struct quic_frame *frm, struct quic_conn *qc); + +/* Free a quic_frame. Remove it from parent element if still attached. */ +static inline void qc_frm_free(struct quic_frame **frm) +{ + + /* Caller must ensure that no other frame points to . Use + * qc_frm_unref() to handle this properly. + */ + BUG_ON(!LIST_ISEMPTY(&((*frm)->reflist))); + BUG_ON(LIST_INLIST(&((*frm)->ref))); + + /* TODO simplify frame deallocation. In some code paths, we must + * manually call this LIST_DEL_INIT before using + * quic_tx_packet_refdec() and freeing the frame. + */ + LIST_DEL_INIT(&((*frm)->list)); + + pool_free(pool_head_quic_frame, *frm); + *frm = NULL; +} + #endif /* USE_QUIC */ #endif /* _HAPROXY_QUIC_FRAME_H */ diff --git a/src/mux_quic.c b/src/mux_quic.c index 1540c3bfde..1060635d24 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -1604,7 +1604,7 @@ static int qcs_send_reset(struct qcs *qcs) LIST_APPEND(&frms, &frm->list); if (qc_send_frames(qcs->qcc, &frms)) { - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); TRACE_DEVEL("cannot send RESET_STREAM", QMUX_EV_QCS_SEND, qcs->qcc->conn, qcs); return 1; } @@ -1659,7 +1659,7 @@ static int qcs_send_stop_sending(struct qcs *qcs) LIST_APPEND(&frms, &frm->list); if (qc_send_frames(qcs->qcc, &frms)) { - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); TRACE_DEVEL("cannot send STOP_SENDING", QMUX_EV_QCS_SEND, qcs->qcc->conn, qcs); return 1; } @@ -1827,10 +1827,8 @@ static int qc_send(struct qcc *qcc) if (!LIST_ISEMPTY(&frms)) { struct quic_frame *frm, *frm2; - list_for_each_entry_safe(frm, frm2, &frms, list) { - LIST_DELETE(&frm->list); - pool_free(pool_head_quic_frame, frm); - } + list_for_each_entry_safe(frm, frm2, &frms, list) + qc_frm_free(&frm); } TRACE_LEAVE(QMUX_EV_QCC_SEND, qcc->conn); @@ -1974,8 +1972,7 @@ static void qc_release(struct qcc *qcc) while (!LIST_ISEMPTY(&qcc->lfctl.frms)) { struct quic_frame *frm = LIST_ELEM(qcc->lfctl.frms.n, struct quic_frame *, list); - LIST_DELETE(&frm->list); - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); } if (qcc->app_ops && qcc->app_ops->release) diff --git a/src/quic_conn.c b/src/quic_conn.c index a42a07a271..ec03fe6ae9 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -1517,29 +1517,6 @@ static int qc_pkt_decrypt(struct quic_conn *qc, struct quic_enc_level *qel, } -/* Remove references to frame */ -static void qc_frm_unref(struct quic_conn *qc, struct quic_frame *frm) -{ - struct quic_frame *f, *tmp; - - TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc); - - list_for_each_entry_safe(f, tmp, &frm->reflist, ref) { - f->origin = NULL; - LIST_DELETE(&f->ref); - if (f->pkt) { - TRACE_DEVEL("remove frame reference", - QUIC_EV_CONN_PRSAFRM, qc, f, &f->pkt->pn_node.key); - } - else { - TRACE_DEVEL("remove frame reference for unsent frame", - QUIC_EV_CONN_PRSAFRM, qc, f); - } - } - - TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc); -} - /* Release frame and mark its copies as acknowledged */ void qc_release_frm(struct quic_conn *qc, struct quic_frame *frm) { @@ -1563,7 +1540,7 @@ void qc_release_frm(struct quic_conn *qc, struct quic_frame *frm) if (f->pkt) { f->flags |= QUIC_FL_TX_FRAME_ACKED; f->origin = NULL; - LIST_DELETE(&f->ref); + LIST_DEL_INIT(&f->ref); pn = f->pkt->pn_node.key; TRACE_DEVEL("mark frame as acked from packet", QUIC_EV_CONN_PRSAFRM, qc, f, &pn); @@ -1571,17 +1548,16 @@ void qc_release_frm(struct quic_conn *qc, struct quic_frame *frm) else { TRACE_DEVEL("freeing unsent frame", QUIC_EV_CONN_PRSAFRM, qc, f); - LIST_DELETE(&f->ref); - LIST_DELETE(&f->list); - pool_free(pool_head_quic_frame, f); + LIST_DEL_INIT(&f->ref); + qc_frm_free(&f); } } - LIST_DELETE(&frm->list); + LIST_DEL_INIT(&frm->list); pn = frm->pkt->pn_node.key; quic_tx_packet_refdec(frm->pkt); TRACE_DEVEL("freeing frame from packet", QUIC_EV_CONN_PRSAFRM, qc, frm, &pn); - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc); } @@ -1792,12 +1768,12 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, list_for_each_entry_safe(frm, frmbak, pkt_frm_list, list) { /* First remove this frame from the packet it was attached to */ - LIST_DELETE(&frm->list); + LIST_DEL_INIT(&frm->list); quic_tx_packet_refdec(pkt); /* At this time, this frame is not freed but removed from its packet */ frm->pkt = NULL; /* Remove any reference to this frame */ - qc_frm_unref(qc, frm); + qc_frm_unref(frm, qc); switch (frm->type) { case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F: { @@ -1810,7 +1786,7 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, TRACE_DEVEL("released stream", QUIC_EV_CONN_PRSAFRM, qc, frm); TRACE_DEVEL("freeing frame from packet", QUIC_EV_CONN_PRSAFRM, qc, frm, &pn); - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); continue; } @@ -1819,7 +1795,7 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, if (strm_frm->offset.key + strm_frm->len <= stream_desc->ack_offset) { TRACE_DEVEL("ignored frame in already acked range", QUIC_EV_CONN_PRSAFRM, qc, frm); - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); continue; } else if (strm_frm->offset.key < stream_desc->ack_offset) { @@ -1839,8 +1815,8 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, TRACE_DEVEL("ignored frame with old data from packet", QUIC_EV_CONN_PRSAFRM, qc, frm, &pn); if (frm->origin) - LIST_DELETE(&frm->ref); - pool_free(pool_head_quic_frame, frm); + LIST_DEL_INIT(&frm->ref); + qc_frm_free(&frm); continue; } @@ -1848,7 +1824,7 @@ static inline void qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc, TRACE_DEVEL("already acked frame", QUIC_EV_CONN_PRSAFRM, qc, frm); TRACE_DEVEL("freeing frame from packet", QUIC_EV_CONN_PRSAFRM, qc, frm, &pn); - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); } else { if (QUIC_FT_STREAM_8 <= frm->type && frm->type <= QUIC_FT_STREAM_F) { @@ -1881,10 +1857,8 @@ static inline void free_quic_tx_packet(struct quic_conn *qc, if (!pkt) goto leave; - list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) { - LIST_DELETE(&frm->list); - pool_free(pool_head_quic_frame, frm); - } + list_for_each_entry_safe(frm, frmbak, &pkt->frms, list) + qc_frm_free(&frm); pool_free(pool_head_quic_tx_packet, pkt); leave: @@ -1993,10 +1967,8 @@ static inline void qc_release_pktns_frms(struct quic_conn *qc, TRACE_ENTER(QUIC_EV_CONN_PHPKTS, qc); - list_for_each_entry_safe(frm, frmbak, &pktns->tx.frms, list) { - LIST_DELETE(&frm->list); - pool_free(pool_head_quic_frame, frm); - } + list_for_each_entry_safe(frm, frmbak, &pktns->tx.frms, list) + qc_frm_free(&frm); TRACE_LEAVE(QUIC_EV_CONN_PHPKTS, qc); } @@ -3566,7 +3538,7 @@ static int quic_build_post_handshake_frames(struct quic_conn *qc) cid = new_quic_cid(&qc->cids, qc, i); if (!cid) { - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); TRACE_ERROR("CID allocation error", QUIC_EV_CONN_IO_CB, qc); goto err; } @@ -3589,7 +3561,7 @@ static int quic_build_post_handshake_frames(struct quic_conn *qc) err: /* free the frames */ list_for_each_entry_safe(frm, frmbak, &frm_list, list) - pool_free(pool_head_quic_frame, frm); + qc_frm_free(&frm); node = eb64_lookup_ge(&qc->cids, first); while (node) { @@ -6718,7 +6690,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist, room -= flen; if (dlen == cf->crypto.len) { /* CRYPTO data have been consumed. */ - LIST_DELETE(&cf->list); + LIST_DEL_INIT(&cf->list); LIST_APPEND(outlist, &cf->list); } else { @@ -6760,8 +6732,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist, node = eb64_lookup(&qc->streams_by_id, strm->id); if (!node) { TRACE_DEVEL("released stream", QUIC_EV_CONN_PRSAFRM, qc, cf); - LIST_DELETE(&cf->list); - pool_free(pool_head_quic_frame, cf); + qc_frm_free(&cf); continue; } @@ -6769,8 +6740,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist, if (strm->offset.key + strm->len <= stream_desc->ack_offset) { TRACE_DEVEL("ignored frame frame in already acked range", QUIC_EV_CONN_PRSAFRM, qc, cf); - LIST_DELETE(&cf->list); - pool_free(pool_head_quic_frame, cf); + qc_frm_free(&cf); continue; } else if (strm->offset.key < stream_desc->ack_offset) { @@ -6818,7 +6788,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist, room -= flen; if (dlen == cf->stream.len) { /* STREAM data have been consumed. */ - LIST_DELETE(&cf->list); + LIST_DEL_INIT(&cf->list); LIST_APPEND(outlist, &cf->list); /* Do not notify MUX on retransmission. */ @@ -6890,7 +6860,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist, *len += flen; room -= flen; - LIST_DELETE(&cf->list); + LIST_DEL_INIT(&cf->list); LIST_APPEND(outlist, &cf->list); break; } @@ -7144,7 +7114,7 @@ static int qc_do_build_pkt(unsigned char *pos, const unsigned char *end, /* Note that was added from to list by * qc_build_frms(). */ - LIST_DELETE(&cf->list); + LIST_DEL_INIT(&cf->list); LIST_INSERT(frms, &cf->list); continue; } diff --git a/src/quic_frame.c b/src/quic_frame.c index 38b524ec15..530a640770 100644 --- a/src/quic_frame.c +++ b/src/quic_frame.c @@ -1171,3 +1171,25 @@ int qc_build_frm(unsigned char **buf, const unsigned char *end, return ret; } +/* Detach all duplicated frames from reflist. */ +void qc_frm_unref(struct quic_frame *frm, struct quic_conn *qc) +{ + struct quic_frame *f, *tmp; + + TRACE_ENTER(QUIC_EV_CONN_PRSAFRM, qc); + + list_for_each_entry_safe(f, tmp, &frm->reflist, ref) { + f->origin = NULL; + LIST_DEL_INIT(&f->ref); + if (f->pkt) { + TRACE_DEVEL("remove frame reference", + QUIC_EV_CONN_PRSAFRM, qc, f, &f->pkt->pn_node.key); + } + else { + TRACE_DEVEL("remove frame reference for unsent frame", + QUIC_EV_CONN_PRSAFRM, qc, f); + } + } + + TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc); +}