From b29a1dc2f4a334c1c7fea76c59abb4097422c05c Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Fri, 12 Aug 2022 14:30:04 +0200 Subject: [PATCH] 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. --- include/haproxy/xprt_quic-t.h | 4 +-- src/xprt_quic.c | 47 ++++++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/include/haproxy/xprt_quic-t.h b/include/haproxy/xprt_quic-t.h index f6fcf95e5..cf6a4e509 100644 --- 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 55001e8f4..b9f274256 100644 --- 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 on connection . * - * 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 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 on connection . 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 on connection . 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);