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 cf6a4e5..f51da03 100644
--- a/include/haproxy/xprt_quic-t.h
+++ b/include/haproxy/xprt_quic-t.h
@@ -592,7 +592,7 @@
#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 1d11c6a..e84959f 100644
--- a/include/haproxy/xprt_quic.h
+++ b/include/haproxy/xprt_quic.h
@@ -769,7 +769,7 @@
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 b48add9..a2814c7 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -1460,7 +1460,7 @@
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 e4cc35b..dd68eb6 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -3885,9 +3885,7 @@
/* 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 @@
* 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 @@
}
/* 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 @@
}
/* 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 @@
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 @@
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);