From: Christopher Faulet <cfaulet@haproxy.com>
Date: Tue, 4 Jun 2024 16:10:51 +0000 (+0200)
Subject: BUG/MEDIUM: mux-quic: Don't unblock zero-copy fwding if blocked during nego
X-Git-Tag: v3.1-dev1~65
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9748df29ff655a9626d6e75ea9db79bb9afa2a50;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: mux-quic: Don't unblock zero-copy fwding if blocked during nego

The previous fix (792a645ec2 ["BUG/MEDIUM: mux-quic: Unblock zero-copy
forwarding if the txbuf can be released"]) introduced a regression. The
zero-copy data forwarding must only be unblocked if it was blocked by the
producer, after a successful negotiation.

It is important because during a negotiation, the consumer may be blocked
for another reason. Because of the flow control for instance. In that case,
there is not necessarily a TX buffer. And it unexpected to try to release an
unallocated TX buf.

In addition, the same may happen while a TX buf is still in-use. In that
case, it must also not be released. So testing the TX buffer is not the
right solution.

To fix the issue, a new IOBUF flag was added (IOBUF_FL_FF_WANT_ROOM). It
must be set by the producer if it is blocked after a sucessful negotiation
because it needs more room. In that case, we know a buffer was provided by
the consummer. In done_fastfwd() callback function, it is then possible to
safely unblock the zero-copy data forwarding if this flag is set.

This patch must be backported to 3.0 with the commit above.
---

diff --git a/include/haproxy/stconn-t.h b/include/haproxy/stconn-t.h
index f418e95b5f..c3468478e7 100644
--- a/include/haproxy/stconn-t.h
+++ b/include/haproxy/stconn-t.h
@@ -39,6 +39,7 @@ enum iobuf_flags {
 						 *  .done_fastfwd() on consumer side must take care of this flag
 						 */
 	IOBUF_FL_EOI              = 0x00000010, /* A EOI was encountered on producer side */
+	IOBUF_FL_FF_WANT_ROOM     = 0x00000020, /* Producer need more room in the IOBUF to forward data */
 };
 
 /* Flags used */
diff --git a/include/haproxy/stconn.h b/include/haproxy/stconn.h
index f60eaa88db..e6548a6226 100644
--- a/include/haproxy/stconn.h
+++ b/include/haproxy/stconn.h
@@ -474,7 +474,7 @@ static inline size_t se_nego_ff(struct sedesc *se, struct buffer *input, size_t
 	if (se_fl_test(se, SE_FL_T_MUX)) {
 		const struct mux_ops *mux = se->conn->mux;
 
-		se->iobuf.flags &= ~IOBUF_FL_FF_BLOCKED;
+		se->iobuf.flags &= ~(IOBUF_FL_FF_BLOCKED|IOBUF_FL_FF_WANT_ROOM);
 		if (mux->nego_fastfwd && mux->done_fastfwd) {
 			/* Disable zero-copy forwarding if EOS or an error was reported. */
 			if (se_fl_test(se, SE_FL_EOS|SE_FL_ERROR|SE_FL_ERR_PENDING)) {
diff --git a/src/applet.c b/src/applet.c
index 2ad9316113..2bc3eb5ee5 100644
--- a/src/applet.c
+++ b/src/applet.c
@@ -694,7 +694,7 @@ int appctx_fastfwd(struct stconn *sc, unsigned int count, unsigned int flags)
 
 	if (se_fl_test(appctx->sedesc, SE_FL_WANT_ROOM)) {
 		/* The applet request more room, report the info at the iobuf level */
-		sdo->iobuf.flags |= IOBUF_FL_FF_BLOCKED;
+		sdo->iobuf.flags |= (IOBUF_FL_FF_BLOCKED|IOBUF_FL_FF_WANT_ROOM);
 		TRACE_STATE("waiting for more room", APPLET_EV_RECV|APPLET_EV_BLK, appctx);
 	}
 
@@ -716,7 +716,7 @@ int appctx_fastfwd(struct stconn *sc, unsigned int count, unsigned int flags)
 	/* else */
 	/* 	applet_have_more_data(appctx); */
 
-	if (se_done_ff(sdo) != 0 || !(sdo->iobuf.flags & IOBUF_FL_FF_BLOCKED)) {
+	if (se_done_ff(sdo) != 0 || !(sdo->iobuf.flags & (IOBUF_FL_FF_BLOCKED|IOBUF_FL_FF_WANT_ROOM))) {
 		/* Something was forwarding or the consumer states it is not
 		 * blocked anyore, don't reclaim more room */
 		se_fl_clr(appctx->sedesc, SE_FL_WANT_ROOM);
diff --git a/src/mux_quic.c b/src/mux_quic.c
index 64b8ac0a38..8272b93da7 100644
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -3049,11 +3049,12 @@ static size_t qmux_strm_done_ff(struct stconn *sc)
 	if (!(qcs->flags & QC_SF_FIN_STREAM) && !sd->iobuf.data) {
 		TRACE_STATE("no data sent", QMUX_EV_STRM_SEND, qcs->qcc->conn, qcs);
 
-		/* There is nothing to forward and the SD is blocked. Try to
-		 * release the TXBUF to retry.
+		/* There is nothing to forward and the SD was blocked after a
+		 * successful nego by the producer. We can try to release the
+		 * TXBUF to retry. In this case, the TX buf MUST exist.
 		 */
-		if ((qcs->sd->iobuf.flags & IOBUF_FL_FF_BLOCKED) && !qcc_release_stream_txbuf(qcs))
-			qcs->sd->iobuf.flags &= ~IOBUF_FL_FF_BLOCKED;
+		if ((qcs->sd->iobuf.flags & IOBUF_FL_FF_WANT_ROOM) && !qcc_release_stream_txbuf(qcs))
+			qcs->sd->iobuf.flags &= ~(IOBUF_FL_FF_BLOCKED|IOBUF_FL_FF_WANT_ROOM);
 		goto end;
 	}