From: Frédéric Lécaille <flecaille@haproxy.com>
Date: Wed, 8 Mar 2023 17:23:13 +0000 (+0100)
Subject: MINOR: quic: Do not stress the peer during retransmissions of lost packets
X-Git-Tag: v2.8-dev5~10
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=be795ceb91b158b325db11ecac01cc3c4c73afe9;p=thirdparty%2Fhaproxy.git

MINOR: quic: Do not stress the peer during retransmissions of lost packets

This issue was revealed by "Multiple streams" QUIC tracker test which very often
fails (locally) with a file of about 1Mbytes (x4 streams). The log of QUIC tracker
revealed that from its point of view, the 4 files were never all received entirely:

 "results" : {
      "stream_0_rec_closed" : true,
      "stream_0_rec_offset" : 1024250,
      "stream_0_snd_closed" : true,
      "stream_0_snd_offset" : 15,
      "stream_12_rec_closed" : false,
      "stream_12_rec_offset" : 72689,
      "stream_12_snd_closed" : true,
      "stream_12_snd_offset" : 15,
      "stream_4_rec_closed" : true,
      "stream_4_rec_offset" : 1024250,
      "stream_4_snd_closed" : true,
      "stream_4_snd_offset" : 15,
      "stream_8_rec_closed" : true,
      "stream_8_rec_offset" : 1024250,
      "stream_8_snd_closed" : true,
      "stream_8_snd_offset" : 15
   },

But this in contradiction with others QUIC tracker logs which confirms that haproxy
has really (re)sent the stream at the suspected offset(stream_12_rec_offset):

       1152085,
       "transport",
       "packet_received",
       {
          "frames" : [
             {
                "frame_type" : "stream",
                "length" : "155",
                "offset" : "72689",
                "stream_id" : "12"
             }
          ],
          "header" : {
             "dcid" : "a14479169ebb9dba",
             "dcil" : "8",
             "packet_number" : "466",
             "packet_size" : 190
          },
          "packet_type" : "1RTT"
       }

When detected as losts, the packets are enlisted, then their frames are
requeued in their packet number space by qc_requeue_nacked_pkt_tx_frms().
This was done using a local list which was spliced to the packet number
frame list. This had as bad effect to retransmit the frames in the inverse
order they have been sent. This is something the QUIC tracker go client
does not like at all!

Removing the frame splicing fixes this issue and allows haproxy to pass the
"Multiple streams" test.

Must be backported to 2.7.
---

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 6d671911f7..8623afefbe 100644
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -1858,7 +1858,6 @@ static inline int qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
                                                 struct list *pktns_frm_list)
 {
 	struct quic_frame *frm, *frmbak;
-	struct list tmp = LIST_HEAD_INIT(tmp);
 	struct list *pkt_frm_list = &pkt->frms;
 	uint64_t pn = pkt->pn_node.key;
 	int close = 0;
@@ -1932,13 +1931,11 @@ static inline int qc_requeue_nacked_pkt_tx_frms(struct quic_conn *qc,
 				close = 1;
 			}
 
-			LIST_APPEND(&tmp, &frm->list);
+			LIST_APPEND(pktns_frm_list, &frm->list);
 			TRACE_DEVEL("frame requeued", QUIC_EV_CONN_PRSAFRM, qc, frm);
 		}
 	}
 
-	LIST_SPLICE(pktns_frm_list, &tmp);
-
  end:
 	TRACE_LEAVE(QUIC_EV_CONN_PRSAFRM, qc);
 	return !close;