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 cf6a4e5091073b3f9cd4c2fc8f62a45c8475a562..f51da039a8b22279ee51268d0e02c5bc6fa7c0ec 100644 (file)
--- 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 1d11c6a27d632f21f6805978f5ceef2a92065a9e..e84959ff477bfb8400662d790b26d64f88a6c91f 100644 (file)
--- 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 b48add9c7f63c7edf3cb3764497581061babce4b..a2814c7b5858ebed825853920b50caf1831f6b92 100644 (file)
--- 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 e4cc35bc9a750134d1a3fa50a2bad8100fb4358c..dd68eb6d72d444c875709c0a9a6d76d153634d7a 100644 (file)
--- 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 <frms> on connection <qc>.
  *
- * 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 <frms> 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 <frms> on connection <qc>. 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 <frms> on connection <qc>. Use this
- * function when probing is required.
+/* Try to send application frames from list <frms> on connection <qc>. 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);