From: Amaury Denoyelle Date: Wed, 17 Aug 2022 14:33:13 +0000 (+0200) Subject: BUG/MEDIUM: quic: fix crash on MUX send notification X-Git-Tag: v2.7-dev4~21 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=704675656bf8b577971f1bbc3be186f4cc362632;p=thirdparty%2Fhaproxy.git BUG/MEDIUM: quic: fix crash on MUX send notification MUX notification on TX has been edited recently : it will be notified only when sending its own data, and not for example on retransmission by the quic-conn layer. This is subject of the patch : b29a1dc2f4a334c1c7fea76c59abb4097422c05c BUG/MINOR: quic: do not notify MUX on frame retransmit A new flag QUIC_FL_CONN_RETRANS_LOST_DATA has been introduced to differentiate qc_send_app_pkts invocation by MUX and directly by the quic-conn layer in quic_conn_app_io_cb(). However, this is a first problem as internal quic-conn layer usage is not limited to retransmission. For example for NEW_CONNECTION_ID emission. Another problem much important is that send functions are also called through quic_conn_io_cb() which has not been protected from MUX notification. This could probably result in crash when trying to notify the MUX. To fix both problems, quic-conn flagging has been inverted : when used by the MUX, quic-conn is flagged with QUIC_FL_CONN_TX_MUX_CONTEXT. To improve the API, MUX must now used qc_send_mux which ensure the flag is set. qc_send_app_pkts is now static and can only be used by the quic-conn layer. This must be backported wherever the previously mentionned patch is. --- diff --git a/include/haproxy/xprt_quic-t.h b/include/haproxy/xprt_quic-t.h index cf6a4e5091..f51da039a8 100644 --- a/include/haproxy/xprt_quic-t.h +++ b/include/haproxy/xprt_quic-t.h @@ -592,7 +592,7 @@ enum qc_mux_state { #define QUIC_FL_CONN_POST_HANDSHAKE_FRAMES_BUILT (1U << 2) #define QUIC_FL_CONN_LISTENER (1U << 3) #define QUIC_FL_CONN_ACCEPT_REGISTERED (1U << 4) -#define QUIC_FL_CONN_RETRANS_LOST_DATA (1U << 5) /* retransmission in progress for lost data */ +#define QUIC_FL_CONN_TX_MUX_CONTEXT (1U << 5) /* sending in progress from the MUX layer */ #define QUIC_FL_CONN_IDLE_TIMER_RESTARTED_AFTER_READ (1U << 6) #define QUIC_FL_CONN_RETRANS_NEEDED (1U << 7) #define QUIC_FL_CONN_RETRANS_OLD_DATA (1U << 8) /* retransmission in progress for probing with already sent data */ diff --git a/include/haproxy/xprt_quic.h b/include/haproxy/xprt_quic.h index 1d11c6a27d..e84959ff47 100644 --- a/include/haproxy/xprt_quic.h +++ b/include/haproxy/xprt_quic.h @@ -769,7 +769,7 @@ int quic_set_app_ops(struct quic_conn *qc, const unsigned char *alpn, size_t alp struct task *quic_lstnr_dghdlr(struct task *t, void *ctx, unsigned int state); int quic_get_dgram_dcid(unsigned char *buf, const unsigned char *end, unsigned char **dcid, size_t *dcid_len); -int qc_send_app_pkts(struct quic_conn *qc, struct list *frms); +int qc_send_mux(struct quic_conn *qc, struct list *frms); void qc_notify_close(struct quic_conn *qc); diff --git a/src/mux_quic.c b/src/mux_quic.c index b48add9c7f..a2814c7b58 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -1460,7 +1460,7 @@ static int qc_send_frames(struct qcc *qcc, struct list *frms) LIST_INIT(&qcc->send_retry_list); - qc_send_app_pkts(qcc->conn->handle.qc, frms); + qc_send_mux(qcc->conn->handle.qc, frms); /* If there is frames left at this stage, transport layer is blocked. * Subscribe on it to retry later. diff --git a/src/xprt_quic.c b/src/xprt_quic.c index e4cc35bc9a..dd68eb6d72 100644 --- a/src/xprt_quic.c +++ b/src/xprt_quic.c @@ -3885,9 +3885,7 @@ static int qc_qel_may_rm_hp(struct quic_conn *qc, struct quic_enc_level *qel) /* Try to send application frames from list on connection . * - * For retransmission you must use wrapper depending on the sending condition : - * - use qc_send_app_retransmit to send data detected as lost - * - use qc_send_app_probing when probing with already sent data + * Use qc_send_app_probing wrapper when probing with old data. * * Returns 1 on success. Some data might not have been sent due to congestion, * in this case they are left in input list. The caller may subscribe on @@ -3897,7 +3895,7 @@ static int qc_qel_may_rm_hp(struct quic_conn *qc, struct quic_enc_level *qel) * TODO review and classify more distinctly transient from definitive errors to * allow callers to properly handle it. */ -int qc_send_app_pkts(struct quic_conn *qc, struct list *frms) +static int qc_send_app_pkts(struct quic_conn *qc, struct list *frms) { int status = 0; struct buffer *buf; @@ -3944,42 +3942,42 @@ int qc_send_app_pkts(struct quic_conn *qc, struct list *frms) } /* Try to send application frames from list on connection . Use this - * function when retransmitting lost frames. + * function when probing is required. * * Returns the result from qc_send_app_pkts function. */ -static forceinline int qc_send_app_retransmit(struct quic_conn *qc, - struct list *frms) +static forceinline int qc_send_app_probing(struct quic_conn *qc, + struct list *frms) { int ret; TRACE_ENTER(QUIC_EV_CONN_TXPKT, qc); - TRACE_STATE("preparing lost data (retransmission)", QUIC_EV_CONN_TXPKT, qc); - qc->flags |= QUIC_FL_CONN_RETRANS_LOST_DATA; + TRACE_STATE("preparing old data (probing)", QUIC_EV_CONN_TXPKT, qc); + qc->flags |= QUIC_FL_CONN_RETRANS_OLD_DATA; ret = qc_send_app_pkts(qc, frms); - qc->flags &= ~QUIC_FL_CONN_RETRANS_LOST_DATA; + qc->flags &= ~QUIC_FL_CONN_RETRANS_OLD_DATA; TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); return ret; } -/* Try to send application frames from list on connection . Use this - * function when probing is required. +/* Try to send application frames from list on connection . This + * function is provided for MUX upper layer usage only. * * Returns the result from qc_send_app_pkts function. */ -static forceinline int qc_send_app_probing(struct quic_conn *qc, - struct list *frms) +int qc_send_mux(struct quic_conn *qc, struct list *frms) { int ret; TRACE_ENTER(QUIC_EV_CONN_TXPKT, qc); + BUG_ON(qc->mux_state != QC_MUX_READY); /* Only MUX can uses this function so it must be ready. */ - TRACE_STATE("preparing old data (probing)", QUIC_EV_CONN_TXPKT, qc); - qc->flags |= QUIC_FL_CONN_RETRANS_OLD_DATA; + TRACE_STATE("preparing data (from MUX)", QUIC_EV_CONN_TXPKT, qc); + qc->flags |= QUIC_FL_CONN_TX_MUX_CONTEXT; ret = qc_send_app_pkts(qc, frms); - qc->flags &= ~QUIC_FL_CONN_RETRANS_OLD_DATA; + qc->flags &= ~QUIC_FL_CONN_TX_MUX_CONTEXT; TRACE_LEAVE(QUIC_EV_CONN_TXPKT, qc); return ret; @@ -4155,8 +4153,8 @@ static struct task *quic_conn_app_io_cb(struct task *t, void *context, unsigned } /* XXX TODO: how to limit the list frames to send */ - if (!qc_send_app_retransmit(qc, &qel->pktns->tx.frms)) { - TRACE_DEVEL("qc_send_app_retransmit() failed", QUIC_EV_CONN_IO_CB, qc); + if (!qc_send_app_pkts(qc, &qel->pktns->tx.frms)) { + TRACE_DEVEL("qc_send_app_pkts() failed", QUIC_EV_CONN_IO_CB, qc); goto out; } @@ -6487,8 +6485,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist, LIST_APPEND(outlist, &cf->list); /* Do not notify MUX on retransmission. */ - if (!(qc->flags & (QUIC_FL_CONN_RETRANS_LOST_DATA|QUIC_FL_CONN_RETRANS_OLD_DATA))) { - BUG_ON(qc->mux_state != QC_MUX_READY); /* MUX must be the caller if not on retransmission. */ + if (qc->flags & QUIC_FL_CONN_TX_MUX_CONTEXT) { qcc_streams_sent_done(cf->stream.stream->ctx, cf->stream.len, cf->stream.offset.key); @@ -6527,8 +6524,7 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist, cf->stream.data = (unsigned char *)b_peek(&cf_buf, dlen); /* Do not notify MUX on retransmission. */ - if (!(qc->flags & (QUIC_FL_CONN_RETRANS_LOST_DATA|QUIC_FL_CONN_RETRANS_OLD_DATA))) { - BUG_ON(qc->mux_state != QC_MUX_READY); /* MUX must be the caller if not on retransmission. */ + if (qc->flags & QUIC_FL_CONN_TX_MUX_CONTEXT) { qcc_streams_sent_done(new_cf->stream.stream->ctx, new_cf->stream.len, new_cf->stream.offset.key);