Re: [PATCH] tcp: Clamp MSS value when queueing data to tap, also for pasta

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4784 bytes --]

On Wed, Mar 08, 2023 at 10:16:28PM +0100, Stefano Brivio wrote:
> 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 <tom@devnode.com>
> Link: https://github.com/containers/podman/issues/17703
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  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;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]