From mboxrd@z Thu Jan 1 00:00:00 1970 Received: by passt.top (Postfix, from userid 1000) id 0834D5A0272; Wed, 8 Mar 2023 22:16:28 +0100 (CET) From: Stefano Brivio To: passt-dev@passt.top Subject: [PATCH] tcp: Clamp MSS value when queueing data to tap, also for pasta Date: Wed, 8 Mar 2023 22:16:28 +0100 Message-Id: <20230308211628.2282752-1-sbrivio@redhat.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: QTDGAEVKKZUI6IMBU6DW2L63GTXNWVJU X-Message-ID-Hash: QTDGAEVKKZUI6IMBU6DW2L63GTXNWVJU X-MailFrom: sbrivio@passt.top X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Tom Mombourquette X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Tom reports that a pattern of repated ~1 MiB chunks downloads over NNTP over TLS, on Podman 4.4 using pasta as network back-end, results in pasta taking one full CPU thread after a while, and the download never succeeds. On that setup, we end up re-sending the same frame over and over, with a consistent 65 534 bytes size, and never get an acknowledgement from the tap-side client. This only happens for the default MTU value (65 520 bytes) or for values that are slightly smaller than that (down to 64 499 bytes). We hit this condition because the MSS value we use in tcp_data_from_sock(), only in pasta mode, is simply clamped to USHRT_MAX, and not to the actual size of the buffers we pre-cooked for sending, which is a bit less than that. It looks like we got away with it until commit 0fb7b2b9080a ("tap: Use different io vector bases depending on tap type") fixed the setting of iov_len. Luckily, since it's pasta, we're queueing up to two frames at a time, so the worst that can happen is a badly segmented TCP stream: we always have some space at the tail of the buffer. Clamp the MSS value to the appropriate maximum given by struct tcp{4,6}_buf_data_t, no matter if we're running in pasta or passt mode. While at it, fix the comments to those structs to reflect the current struct size. This is not really relevant for any further calculation or consideration, but it's convenient to know while debugging this kind of issues. Thanks to Tom for reporting the issue in a very detailed way and for providing a test setup. Reported-by: Tom Mombourquette Link: https://github.com/containers/podman/issues/17703 Signed-off-by: Stefano Brivio --- tcp.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/tcp.c b/tcp.c index a29e387..0214087 100644 --- a/tcp.c +++ b/tcp.c @@ -461,7 +461,7 @@ static struct tcp4_l2_buf_t { struct iphdr iph; /* 44 28 */ struct tcphdr th; /* 64 48 */ uint8_t data[MSS4]; /* 84 68 */ - /* 65541 65525 */ + /* 65536 65532 */ #ifdef __AVX2__ } __attribute__ ((packed, aligned(32))) #else @@ -489,7 +489,7 @@ struct tcp6_l2_buf_t { struct ipv6hdr ip6h; /* 32 20 */ struct tcphdr th; /* 72 60 */ uint8_t data[MSS6]; /* 92 80 */ - /* 65639 65627 */ + /* 65536 65532 */ #ifdef __AVX2__ } __attribute__ ((packed, aligned(32))) #else @@ -1917,15 +1917,13 @@ int tcp_conn_new_sock(const struct ctx *c, sa_family_t af) /** * tcp_conn_tap_mss() - Get MSS value advertised by tap/guest - * @c: Execution context * @conn: Connection pointer * @opts: Pointer to start of TCP options * @optlen: Bytes in options: caller MUST ensure available length * * Return: clamped MSS value */ -static uint16_t tcp_conn_tap_mss(const struct ctx *c, - const struct tcp_tap_conn *conn, +static uint16_t tcp_conn_tap_mss(const struct tcp_tap_conn *conn, const char *opts, size_t optlen) { unsigned int mss; @@ -1936,13 +1934,10 @@ static uint16_t tcp_conn_tap_mss(const struct ctx *c, else mss = ret; - /* Don't upset qemu */ - if (c->mode == MODE_PASST) { - if (CONN_V4(conn)) - mss = MIN(MSS4, mss); - else - mss = MIN(MSS6, mss); - } + if (CONN_V4(conn)) + mss = MIN(MSS4, mss); + else + mss = MIN(MSS6, mss); return MIN(mss, USHRT_MAX); } @@ -2066,7 +2061,7 @@ static void tcp_conn_from_tap(struct ctx *c, int af, const void *addr, conn->wnd_to_tap = WINDOW_DEFAULT; - mss = tcp_conn_tap_mss(c, conn, opts, optlen); + mss = tcp_conn_tap_mss(conn, opts, optlen); if (setsockopt(s, SOL_TCP, TCP_MAXSEG, &mss, sizeof(mss))) trace("TCP: failed to set TCP_MAXSEG on socket %i", s); MSS_SET(conn, mss); @@ -2533,7 +2528,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn, if (!(conn->wnd_from_tap >>= conn->ws_from_tap)) conn->wnd_from_tap = 1; - MSS_SET(conn, tcp_conn_tap_mss(c, conn, opts, optlen)); + MSS_SET(conn, tcp_conn_tap_mss(conn, opts, optlen)); conn->seq_init_from_tap = ntohl(th->seq) + 1; conn->seq_from_tap = conn->seq_init_from_tap; -- 2.39.2