BUG/MINOR: quic: do not notify MUX on frame retransmit

On STREAM emission, quic-conn notifies MUX through a callback named
qcc_streams_sent_done(). This also happens on retransmission : in this
case offset are examined and notification is ignored if already seen.

However, this behavior has slightly changed since
  e53b489826ba9760a527b461095402ca05d2b6be
  BUG/MEDIUM: mux-quic: fix server chunked encoding response

Indeed, if offset diff is NULL, frame is now not ignored. This is to
support FIN notification with a final empty STREAM frame. A side-effect
of this is that if the last stream frame is retransmitted, it won't be
ignored in qcc_streams_sent_done().

In most cases, this side-effect is harmless as qcs instance will soon be
freed after being closed. But if qcs is still alive, this will cause a
BUG_ON crash as it is considered as locally closed.

This bug depends on delay condition and seems to be extremely rare. But
it might be the reason for a crash seen on interop with s2n client on
http3 testcase :

FATAL: bug condition "qcs->st == QC_SS_CLO" matched at src/mux_quic.c:372
  call trace(16):
  | 0x558228912b0d [b8 01 00 00 00 c6 00 00]: main-0x1c7878
  | 0x558228917a70 [48 8b 55 d8 48 8b 45 e0]: qcc_streams_sent_done+0xcf/0x355
  | 0x558228906ff1 [e9 29 05 00 00 48 8b 05]: main-0x1d3394
  | 0x558228907cd9 [48 83 c4 10 85 c0 0f 85]: main-0x1d26ac
  | 0x5582289089c1 [48 83 c4 50 85 c0 75 12]: main-0x1d19c4
  | 0x5582288f8d2a [48 83 c4 40 48 89 45 a0]: main-0x1e165b
  | 0x5582288fc4cc [89 45 b4 83 7d b4 ff 74]: qc_send_app_pkts+0xc6/0x1f0
  | 0x5582288fd311 [85 c0 74 12 eb 01 90 48]: main-0x1dd074
  | 0x558228b2e4c1 [48 c7 c0 d0 60 ff ff 64]: run_tasks_from_lists+0x4e6/0x98e
  | 0x558228b2f13f [8b 55 80 29 c2 89 d0 89]: process_runnable_tasks+0x7d6/0x84c
  | 0x558228ad9aa9 [8b 05 75 16 4b 00 83 f8]: run_poll_loop+0x80/0x48c
  | 0x558228ada12f [48 8b 05 aa c5 20 00 48]: main-0x256
  | 0x7ff01ed2e609 [64 48 89 04 25 30 06 00]: libpthread:+0x8609
  | 0x7ff01e8ca163 [48 89 c7 b8 3c 00 00 00]: libc:clone+0x43/0x5e

To reproduce it locally, code was artificially patched to produce
retransmission and avoid qcs liberation.

In order to fix this and avoid future class of similar problem, the best
way is to not call qcc_streams_sent_done() to notify MUX for
retranmission. To implement this, we test if any of
QUIC_FL_CONN_RETRANS_OLD_DATA or the new flag
QUIC_FL_CONN_RETRANS_LOST_DATA is set. A new wrapper
qc_send_app_retransmit() has been added to set the new flag as a
complement to already existing qc_send_app_probing().

This must be backported up to 2.6.

diff --git a/include/haproxy/xprt_quic-t.h b/include/haproxy/xprt_quic-t.h
index f6fcf95e595d0ff08359e61fb80956a8569ea905..cf6a4e5091073b3f9cd4c2fc8f62a45c8475a562 100644 (file)
--- a/include/haproxy/xprt_quic-t.h
+++ b/include/haproxy/xprt_quic-t.h
@@ -592,10 +592,10 @@ 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)
-/* gap here */
+#define QUIC_FL_CONN_RETRANS_LOST_DATA           (1U << 5) /* retransmission in progress for lost data */
 #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)
+#define QUIC_FL_CONN_RETRANS_OLD_DATA            (1U << 8) /* retransmission in progress for probing with already sent data */
 #define QUIC_FL_CONN_TLS_ALERT                   (1U << 9)
 /* gap here */
 #define QUIC_FL_CONN_HALF_OPEN_CNT_DECREMENTED   (1U << 11) /* The half-open connection counter was decremented */

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 55001e8f414be83be5ec501da786a00c3dd93260..b9f274256518ee7d1a47ad981fcf69bfdbdcd1d7 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -3884,7 +3884,9 @@ 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>.
  *
- * Use qc_send_app_probing wrapper when probing with old data.
+ * 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
  *
  * 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
@@ -3940,6 +3942,27 @@ int qc_send_app_pkts(struct quic_conn *qc, struct list *frms)
        goto leave;
 }
 
+/* Try to send application frames from list <frms> on connection <qc>. Use this
+ * function when retransmitting lost frames.
+ *
+ * Returns the result from qc_send_app_pkts function.
+ */
+static forceinline int qc_send_app_retransmit(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;
+       ret = qc_send_app_pkts(qc, frms);
+       qc->flags &= ~QUIC_FL_CONN_RETRANS_LOST_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.
  *
@@ -4131,8 +4154,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_pkts(qc, &qel->pktns->tx.frms)) {
-               TRACE_DEVEL("qc_send_app_pkts() failed", QUIC_EV_CONN_IO_CB, qc);
+       if (!qc_send_app_retransmit(qc, &qel->pktns->tx.frms)) {
+               TRACE_DEVEL("qc_send_app_retransmit() failed", QUIC_EV_CONN_IO_CB, qc);
                goto out;
        }
 
@@ -6462,12 +6485,9 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                                LIST_DELETE(&cf->list);
                                LIST_APPEND(outlist, &cf->list);
 
-                               /* The MUX stream might be released at this
-                                * stage. This can most notably happen on
-                                * retransmission.
-                                */
-                               if (qc->mux_state == QC_MUX_READY &&
-                                   !cf->stream.stream->release) {
+                               /* 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. */
                                        qcc_streams_sent_done(cf->stream.stream->ctx,
                                                              cf->stream.len,
                                                              cf->stream.offset.key);
@@ -6505,12 +6525,9 @@ static inline int qc_build_frms(struct list *outlist, struct list *inlist,
                                cf->stream.offset.key += dlen;
                                cf->stream.data = (unsigned char *)b_peek(&cf_buf, dlen);
 
-                               /* The MUX stream might be released at this
-                                * stage. This can most notably happen on
-                                * retransmission.
-                                */
-                               if (qc->mux_state == QC_MUX_READY &&
-                                   !cf->stream.stream->release) {
+                               /* 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. */
                                        qcc_streams_sent_done(new_cf->stream.stream->ctx,
                                                              new_cf->stream.len,
                                                              new_cf->stream.offset.key);