[PATCH 0/2] Avoid bugs related to TCP_WINDOW_CLAMP

[PATCH 0/2] Avoid bugs related to TCP_WINDOW_CLAMP

[David Gibson]

The way we used TCP_WINDOW_CLAMP didn't really correspond to what it
actually did.  More recent kernels also have a nasty bug in the
handler which we could trigger, causing TCP stalls.

David Gibson (2):
  tcp: Rename and small cleanup to tcp_clamp_window()
  tcp: Don't use TCP_WINDOW_CLAMP

 tcp.c      | 70 +++++++++---------------------------------------------
 tcp_conn.h |  7 +++---
 2 files changed, 14 insertions(+), 63 deletions(-)

-- 
2.41.0

[PATCH 1/2] tcp: Rename and small cleanup to tcp_clamp_window()

[David Gibson]

tcp_clamp_window() is _mostly_ about using TCP_WINDOW_CLAMP to control the
sock side advertised window, but it is also responsible for actually
updating the conn->wnd_from_tap value.

Rename to tcp_tap_window_update() to reflect that broader purpose, and pull
the logic that's not TCP_WINDOW_CLAMP related out to the front.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tcp.c b/tcp.c
index 19d16a6..13e82ca 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1789,19 +1789,19 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
 }
 
 /**
- * tcp_clamp_window() - Set new window for connection, clamp on socket
+ * tcp_tap_window_update() - Process an updated window from tap side
  * @c:		Execution context
  * @conn:	Connection pointer
  * @window:	Window value, host order, unscaled
  */
-static void tcp_clamp_window(const struct ctx *c, struct tcp_tap_conn *conn,
-			     unsigned wnd)
+static void tcp_tap_window_update(const struct ctx *c,
+				  struct tcp_tap_conn *conn, unsigned wnd)
 {
 	uint32_t prev_scaled = conn->wnd_from_tap << conn->ws_from_tap;
 	int s = conn->sock;
 
-	wnd <<= conn->ws_from_tap;
-	wnd = MIN(MAX_WINDOW, wnd);
+	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
+	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
 
 	/* TODO: With (at least) Linux kernel versions 6.1 to 6.5, if we end up
 	 * with a zero-sized window on a TCP socket, dropping data (once
@@ -1838,7 +1838,6 @@ static void tcp_clamp_window(const struct ctx *c, struct tcp_tap_conn *conn,
 			return;
 	}
 
-	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
 	if (setsockopt(s, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd)))
 		trace("TCP: failed to set TCP_WINDOW_CLAMP on socket %i", s);
 
@@ -2453,7 +2452,7 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn,
 	if (ack && !tcp_sock_consume(conn, max_ack_seq))
 		tcp_update_seqack_from_tap(c, conn, max_ack_seq);
 
-	tcp_clamp_window(c, conn, max_ack_seq_wnd);
+	tcp_tap_window_update(c, conn, max_ack_seq_wnd);
 
 	if (retr) {
 		trace("TCP: fast re-transmit, ACK: %u, previous sequence: %u",
@@ -2538,7 +2537,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn,
 				      const struct tcphdr *th,
 				      const char *opts, size_t optlen)
 {
-	tcp_clamp_window(c, conn, ntohs(th->window));
+	tcp_tap_window_update(c, conn, ntohs(th->window));
 	tcp_get_tap_ws(conn, opts, optlen);
 
 	/* First value is not scaled */
@@ -2646,7 +2645,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 		if (!th->ack)
 			goto reset;
 
-		tcp_clamp_window(c, conn, ntohs(th->window));
+		tcp_tap_window_update(c, conn, ntohs(th->window));
 
 		tcp_data_from_sock(c, conn);
 
@@ -2670,8 +2669,8 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 	if (count == -1)
 		goto reset;
 
-	/* Note: STALLED matters for tcp_clamp_window(): unset it only after
-	 * processing data (and window) from the tap side
+	/* Note: STALLED matters for tcp_tap_window_update(): unset it only
+	 * after processing data (and window) from the tap side
 	 */
 	conn_flag(c, conn, ~STALLED);
 
-- 
@@ -1789,19 +1789,19 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
 }
 
 /**
- * tcp_clamp_window() - Set new window for connection, clamp on socket
+ * tcp_tap_window_update() - Process an updated window from tap side
  * @c:		Execution context
  * @conn:	Connection pointer
  * @window:	Window value, host order, unscaled
  */
-static void tcp_clamp_window(const struct ctx *c, struct tcp_tap_conn *conn,
-			     unsigned wnd)
+static void tcp_tap_window_update(const struct ctx *c,
+				  struct tcp_tap_conn *conn, unsigned wnd)
 {
 	uint32_t prev_scaled = conn->wnd_from_tap << conn->ws_from_tap;
 	int s = conn->sock;
 
-	wnd <<= conn->ws_from_tap;
-	wnd = MIN(MAX_WINDOW, wnd);
+	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
+	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
 
 	/* TODO: With (at least) Linux kernel versions 6.1 to 6.5, if we end up
 	 * with a zero-sized window on a TCP socket, dropping data (once
@@ -1838,7 +1838,6 @@ static void tcp_clamp_window(const struct ctx *c, struct tcp_tap_conn *conn,
 			return;
 	}
 
-	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
 	if (setsockopt(s, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd)))
 		trace("TCP: failed to set TCP_WINDOW_CLAMP on socket %i", s);
 
@@ -2453,7 +2452,7 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn,
 	if (ack && !tcp_sock_consume(conn, max_ack_seq))
 		tcp_update_seqack_from_tap(c, conn, max_ack_seq);
 
-	tcp_clamp_window(c, conn, max_ack_seq_wnd);
+	tcp_tap_window_update(c, conn, max_ack_seq_wnd);
 
 	if (retr) {
 		trace("TCP: fast re-transmit, ACK: %u, previous sequence: %u",
@@ -2538,7 +2537,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn,
 				      const struct tcphdr *th,
 				      const char *opts, size_t optlen)
 {
-	tcp_clamp_window(c, conn, ntohs(th->window));
+	tcp_tap_window_update(c, conn, ntohs(th->window));
 	tcp_get_tap_ws(conn, opts, optlen);
 
 	/* First value is not scaled */
@@ -2646,7 +2645,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 		if (!th->ack)
 			goto reset;
 
-		tcp_clamp_window(c, conn, ntohs(th->window));
+		tcp_tap_window_update(c, conn, ntohs(th->window));
 
 		tcp_data_from_sock(c, conn);
 
@@ -2670,8 +2669,8 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 	if (count == -1)
 		goto reset;
 
-	/* Note: STALLED matters for tcp_clamp_window(): unset it only after
-	 * processing data (and window) from the tap side
+	/* Note: STALLED matters for tcp_tap_window_update(): unset it only
+	 * after processing data (and window) from the tap side
 	 */
 	conn_flag(c, conn, ~STALLED);
 
-- 
2.41.0

[PATCH 2/2] tcp: Don't use TCP_WINDOW_CLAMP

[David Gibson]

On the L2 tap side, we see TCP headers and know the TCP window that the
ultimate receiver is advertising.  In order to avoid unnecessary buffering
within passt/pasta (or by the kernel on passt/pasta's behalf) we attempt
to advertise that window back to the original sock-side sender using
TCP_WINDOW_CLAMP.

However, TCP_WINDOW_CLAMP just doesn't work like this.  Prior to kernel
commit 3aa7857fe1d7 ("tcp: enable mid stream window clamp"), it simply
had no effect on established sockets.  After that commit, it does affect
established sockets but doesn't behave the way we need:
  * It appears to be designed only to shrink the window, not to allow it to
    re-expand.
  * More importantly, that commit has a serious bug where if the
    setsockopt() is made when the existing kernel advertised window for the
    socket happens to be zero, it will now become locked at zero, stopping
    any further data from being received on the socket.

Since this has never worked as intended, simply remove it.  It might be
possible to re-implement the intended behaviour by manipulating SO_RCVBUF,
so we leave a comment to that effect.

This kernel bug is the underlying cause of both the linked passt bug and
the linked podman bug.  We attempted to fix this before with passt commit
d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag").
However while that commit masked the bug for some cases, it didn't really
address the problem.

Fixes: d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag")
Link: https://github.com/containers/podman/issues/20170
Link: https://bugs.passt.top/show_bug.cgi?id=74

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c      | 65 ++++++++----------------------------------------------
 tcp_conn.h |  7 +++---
 2 files changed, 12 insertions(+), 60 deletions(-)

diff --git a/tcp.c b/tcp.c
index 13e82ca..cfcd40a 100644
--- a/tcp.c
+++ b/tcp.c
@@ -46,8 +46,8 @@
  *   - avoiding port and address translations whenever possible
  *   - mirroring TCP dynamics by observation of socket parameters (TCP_INFO
  *     socket option) and TCP headers of packets coming from the tap interface,
- *     reapplying those parameters in both flow directions (including TCP_MSS,
- *     TCP_WINDOW_CLAMP socket options)
+ *     reapplying those parameters in both flow directions (including TCP_MSS
+ *     socket option)
  * - simplicity: only a small subset of TCP logic is implemented here and
  *   delegated as much as possible to the TCP implementations of guest and host
  *   kernel. This is achieved by:
@@ -236,12 +236,10 @@
  *     - update @seq_ack_from_tap from ack_seq in header
  *     - on two duplicated ACKs, reset @seq_to_tap to @seq_ack_from_tap, and
  *       resend with steps listed above
- *     - set TCP_WINDOW_CLAMP from TCP header from tap
  *
  * - from tap/guest to socket:
  *   - on packet from tap/guest:
  *     - set @ts_tap_act
- *     - set TCP_WINDOW_CLAMP from TCP header from tap
  *     - check seq from header against @seq_from_tap, if data is missing, send
  *       two ACKs with number @seq_ack_to_tap, discard packet
  *     - otherwise queue data to socket, set @seq_from_tap to seq from header
@@ -420,7 +418,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
 };
 
 static const char *tcp_flag_str[] __attribute((__unused__)) = {
-	"STALLED", "LOCAL", "WND_CLAMPED", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
+	"STALLED", "LOCAL", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
 	"ACK_FROM_TAP_DUE",
 };
 
@@ -1790,58 +1788,16 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
 
 /**
  * tcp_tap_window_update() - Process an updated window from tap side
- * @c:		Execution context
  * @conn:	Connection pointer
  * @window:	Window value, host order, unscaled
  */
-static void tcp_tap_window_update(const struct ctx *c,
-				  struct tcp_tap_conn *conn, unsigned wnd)
+static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
 {
-	uint32_t prev_scaled = conn->wnd_from_tap << conn->ws_from_tap;
-	int s = conn->sock;
-
 	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
 	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
 
-	/* TODO: With (at least) Linux kernel versions 6.1 to 6.5, if we end up
-	 * with a zero-sized window on a TCP socket, dropping data (once
-	 * acknowledged by the guest) with recv() and MSG_TRUNC doesn't appear
-	 * to be enough to make the kernel advertise a non-zero window to the
-	 * sender. Forcing a TCP_WINDOW_CLAMP setting, even with the existing
-	 * value, fixes this.
-	 *
-	 * The STALLED flag on a connection is a sufficient indication that we
-	 * might have a zero-sized window on the socket, because it's set if we
-	 * exhausted the tap-side window, or if everything we receive from a
-	 * socket is already in flight to the guest.
-	 *
-	 * So, if STALLED is set, and we received a window value from the tap,
-	 * force a TCP_WINDOW_CLAMP setsockopt(). This should be investigated
-	 * further and fixed in the kernel instead, if confirmed.
-	 */
-	if (!(conn->flags & STALLED) && conn->flags & WND_CLAMPED) {
-		if (prev_scaled == wnd)
-			return;
-
-		/* Discard +/- 1% updates to spare some syscalls. */
-		/* TODO: cppcheck, starting from commit b4d455df487c ("Fix
-		 * 11349: FP negativeIndex for clamped array index (#4627)"),
-		 * reports wnd > prev_scaled as always being true, see also:
-		 *
-		 *	https://github.com/danmar/cppcheck/pull/4627
-		 *
-		 * drop this suppression once that's resolved.
-		 */
-		/* cppcheck-suppress [knownConditionTrueFalse, unmatchedSuppression] */
-		if ((wnd > prev_scaled && wnd * 99 / 100 < prev_scaled) ||
-		    (wnd < prev_scaled && wnd * 101 / 100 > prev_scaled))
-			return;
-	}
-
-	if (setsockopt(s, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd)))
-		trace("TCP: failed to set TCP_WINDOW_CLAMP on socket %i", s);
-
-	conn_flag(c, conn, WND_CLAMPED);
+	/* FIXME: reflect the tap-side receiver's window back to the sock-side
+	 * sender by adjusting SO_RCVBUF? */
 }
 
 /**
@@ -2452,7 +2408,7 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn,
 	if (ack && !tcp_sock_consume(conn, max_ack_seq))
 		tcp_update_seqack_from_tap(c, conn, max_ack_seq);
 
-	tcp_tap_window_update(c, conn, max_ack_seq_wnd);
+	tcp_tap_window_update(conn, max_ack_seq_wnd);
 
 	if (retr) {
 		trace("TCP: fast re-transmit, ACK: %u, previous sequence: %u",
@@ -2537,7 +2493,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn,
 				      const struct tcphdr *th,
 				      const char *opts, size_t optlen)
 {
-	tcp_tap_window_update(c, conn, ntohs(th->window));
+	tcp_tap_window_update(conn, ntohs(th->window));
 	tcp_get_tap_ws(conn, opts, optlen);
 
 	/* First value is not scaled */
@@ -2645,7 +2601,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 		if (!th->ack)
 			goto reset;
 
-		tcp_tap_window_update(c, conn, ntohs(th->window));
+		tcp_tap_window_update(conn, ntohs(th->window));
 
 		tcp_data_from_sock(c, conn);
 
@@ -2669,9 +2625,6 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 	if (count == -1)
 		goto reset;
 
-	/* Note: STALLED matters for tcp_tap_window_update(): unset it only
-	 * after processing data (and window) from the tap side
-	 */
 	conn_flag(c, conn, ~STALLED);
 
 	if (conn->seq_ack_to_tap != conn->seq_from_tap)
diff --git a/tcp_conn.h b/tcp_conn.h
index 01d31d4..0c6a35b 100644
--- a/tcp_conn.h
+++ b/tcp_conn.h
@@ -85,10 +85,9 @@ struct tcp_tap_conn {
 	uint8_t		flags;
 #define STALLED			BIT(0)
 #define LOCAL			BIT(1)
-#define WND_CLAMPED		BIT(2)
-#define ACTIVE_CLOSE		BIT(3)
-#define ACK_TO_TAP_DUE		BIT(4)
-#define ACK_FROM_TAP_DUE	BIT(5)
+#define ACTIVE_CLOSE		BIT(2)
+#define ACK_TO_TAP_DUE		BIT(3)
+#define ACK_FROM_TAP_DUE	BIT(4)
 
 
 #define TCP_MSS_BITS			14
-- 
@@ -85,10 +85,9 @@ struct tcp_tap_conn {
 	uint8_t		flags;
 #define STALLED			BIT(0)
 #define LOCAL			BIT(1)
-#define WND_CLAMPED		BIT(2)
-#define ACTIVE_CLOSE		BIT(3)
-#define ACK_TO_TAP_DUE		BIT(4)
-#define ACK_FROM_TAP_DUE	BIT(5)
+#define ACTIVE_CLOSE		BIT(2)
+#define ACK_TO_TAP_DUE		BIT(3)
+#define ACK_FROM_TAP_DUE	BIT(4)
 
 
 #define TCP_MSS_BITS			14
-- 
2.41.0

Re: [PATCH 2/2] tcp: Don't use TCP_WINDOW_CLAMP

[Stefano Brivio]

On Thu,  9 Nov 2023 20:54:00 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On the L2 tap side, we see TCP headers and know the TCP window that the
> ultimate receiver is advertising.  In order to avoid unnecessary buffering
> within passt/pasta (or by the kernel on passt/pasta's behalf) we attempt
> to advertise that window back to the original sock-side sender using
> TCP_WINDOW_CLAMP.
> 
> However, TCP_WINDOW_CLAMP just doesn't work like this.  Prior to kernel
> commit 3aa7857fe1d7 ("tcp: enable mid stream window clamp"), it simply
> had no effect on established sockets.  After that commit, it does affect
> established sockets but doesn't behave the way we need:
>   * It appears to be designed only to shrink the window, not to allow it to
>     re-expand.
>   * More importantly, that commit has a serious bug where if the
>     setsockopt() is made when the existing kernel advertised window for the
>     socket happens to be zero, it will now become locked at zero, stopping
>     any further data from being received on the socket.
> 
> Since this has never worked as intended, simply remove it.  It might be
> possible to re-implement the intended behaviour by manipulating SO_RCVBUF,
> so we leave a comment to that effect.
> 
> This kernel bug is the underlying cause of both the linked passt bug and
> the linked podman bug.  We attempted to fix this before with passt commit
> d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag").
> However while that commit masked the bug for some cases, it didn't really
> address the problem.
> 
> Fixes: d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag")
> Link: https://github.com/containers/podman/issues/20170
> Link: https://bugs.passt.top/show_bug.cgi?id=74
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c      | 65 ++++++++----------------------------------------------
>  tcp_conn.h |  7 +++---
>  2 files changed, 12 insertions(+), 60 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 13e82ca..cfcd40a 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -46,8 +46,8 @@
>   *   - avoiding port and address translations whenever possible
>   *   - mirroring TCP dynamics by observation of socket parameters (TCP_INFO
>   *     socket option) and TCP headers of packets coming from the tap interface,
> - *     reapplying those parameters in both flow directions (including TCP_MSS,
> - *     TCP_WINDOW_CLAMP socket options)
> + *     reapplying those parameters in both flow directions (including TCP_MSS
> + *     socket option)
>   * - simplicity: only a small subset of TCP logic is implemented here and
>   *   delegated as much as possible to the TCP implementations of guest and host
>   *   kernel. This is achieved by:
> @@ -236,12 +236,10 @@
>   *     - update @seq_ack_from_tap from ack_seq in header
>   *     - on two duplicated ACKs, reset @seq_to_tap to @seq_ack_from_tap, and
>   *       resend with steps listed above
> - *     - set TCP_WINDOW_CLAMP from TCP header from tap
>   *
>   * - from tap/guest to socket:
>   *   - on packet from tap/guest:
>   *     - set @ts_tap_act
> - *     - set TCP_WINDOW_CLAMP from TCP header from tap
>   *     - check seq from header against @seq_from_tap, if data is missing, send
>   *       two ACKs with number @seq_ack_to_tap, discard packet
>   *     - otherwise queue data to socket, set @seq_from_tap to seq from header
> @@ -420,7 +418,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
>  };
>  
>  static const char *tcp_flag_str[] __attribute((__unused__)) = {
> -	"STALLED", "LOCAL", "WND_CLAMPED", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
> +	"STALLED", "LOCAL", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
>  	"ACK_FROM_TAP_DUE",
>  };
>  
> @@ -1790,58 +1788,16 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
>  
>  /**
>   * tcp_tap_window_update() - Process an updated window from tap side
> - * @c:		Execution context
>   * @conn:	Connection pointer
>   * @window:	Window value, host order, unscaled
>   */
> -static void tcp_tap_window_update(const struct ctx *c,
> -				  struct tcp_tap_conn *conn, unsigned wnd)
> +static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
>  {
> -	uint32_t prev_scaled = conn->wnd_from_tap << conn->ws_from_tap;
> -	int s = conn->sock;
> -
>  	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
>  	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
>  
> -	/* TODO: With (at least) Linux kernel versions 6.1 to 6.5, if we end up
> -	 * with a zero-sized window on a TCP socket, dropping data (once
> -	 * acknowledged by the guest) with recv() and MSG_TRUNC doesn't appear
> -	 * to be enough to make the kernel advertise a non-zero window to the
> -	 * sender. Forcing a TCP_WINDOW_CLAMP setting, even with the existing
> -	 * value, fixes this.
> -	 *
> -	 * The STALLED flag on a connection is a sufficient indication that we
> -	 * might have a zero-sized window on the socket, because it's set if we
> -	 * exhausted the tap-side window, or if everything we receive from a
> -	 * socket is already in flight to the guest.
> -	 *
> -	 * So, if STALLED is set, and we received a window value from the tap,
> -	 * force a TCP_WINDOW_CLAMP setsockopt(). This should be investigated
> -	 * further and fixed in the kernel instead, if confirmed.
> -	 */
> -	if (!(conn->flags & STALLED) && conn->flags & WND_CLAMPED) {
> -		if (prev_scaled == wnd)
> -			return;
> -
> -		/* Discard +/- 1% updates to spare some syscalls. */
> -		/* TODO: cppcheck, starting from commit b4d455df487c ("Fix
> -		 * 11349: FP negativeIndex for clamped array index (#4627)"),
> -		 * reports wnd > prev_scaled as always being true, see also:
> -		 *
> -		 *	https://github.com/danmar/cppcheck/pull/4627
> -		 *
> -		 * drop this suppression once that's resolved.
> -		 */
> -		/* cppcheck-suppress [knownConditionTrueFalse, unmatchedSuppression] */
> -		if ((wnd > prev_scaled && wnd * 99 / 100 < prev_scaled) ||
> -		    (wnd < prev_scaled && wnd * 101 / 100 > prev_scaled))
> -			return;
> -	}
> -
> -	if (setsockopt(s, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd)))
> -		trace("TCP: failed to set TCP_WINDOW_CLAMP on socket %i", s);
> -
> -	conn_flag(c, conn, WND_CLAMPED);
> +	/* FIXME: reflect the tap-side receiver's window back to the sock-side
> +	 * sender by adjusting SO_RCVBUF? */

If we adjust SO_RCVBUF, we lose the chance to get bigger receive
buffers by means of auto-tuning, which is especially problematic with
default values for rmem_max (in turn, the case which is giving us
issues here).

I'd rather fix TCP_CLAMP_WINDOW in the kernel and then re-enable this
in passt (I guess they'll be no obvious way to probe this, so we'll
have to rely on the kernel version).

Anyway, this comment is not so fundamental, so I would apply this
series like it is -- the rest looks good to me.

-- 
Stefano

Re: [PATCH 2/2] tcp: Don't use TCP_WINDOW_CLAMP

[David Gibson]

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

On Thu, Nov 09, 2023 at 03:51:36PM +0100, Stefano Brivio wrote:
> On Thu,  9 Nov 2023 20:54:00 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On the L2 tap side, we see TCP headers and know the TCP window that the
> > ultimate receiver is advertising.  In order to avoid unnecessary buffering
> > within passt/pasta (or by the kernel on passt/pasta's behalf) we attempt
> > to advertise that window back to the original sock-side sender using
> > TCP_WINDOW_CLAMP.
> > 
> > However, TCP_WINDOW_CLAMP just doesn't work like this.  Prior to kernel
> > commit 3aa7857fe1d7 ("tcp: enable mid stream window clamp"), it simply
> > had no effect on established sockets.  After that commit, it does affect
> > established sockets but doesn't behave the way we need:
> >   * It appears to be designed only to shrink the window, not to allow it to
> >     re-expand.
> >   * More importantly, that commit has a serious bug where if the
> >     setsockopt() is made when the existing kernel advertised window for the
> >     socket happens to be zero, it will now become locked at zero, stopping
> >     any further data from being received on the socket.
> > 
> > Since this has never worked as intended, simply remove it.  It might be
> > possible to re-implement the intended behaviour by manipulating SO_RCVBUF,
> > so we leave a comment to that effect.
> > 
> > This kernel bug is the underlying cause of both the linked passt bug and
> > the linked podman bug.  We attempted to fix this before with passt commit
> > d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag").
> > However while that commit masked the bug for some cases, it didn't really
> > address the problem.
> > 
> > Fixes: d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag")
> > Link: https://github.com/containers/podman/issues/20170
> > Link: https://bugs.passt.top/show_bug.cgi?id=74
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp.c      | 65 ++++++++----------------------------------------------
> >  tcp_conn.h |  7 +++---
> >  2 files changed, 12 insertions(+), 60 deletions(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index 13e82ca..cfcd40a 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -46,8 +46,8 @@
> >   *   - avoiding port and address translations whenever possible
> >   *   - mirroring TCP dynamics by observation of socket parameters (TCP_INFO
> >   *     socket option) and TCP headers of packets coming from the tap interface,
> > - *     reapplying those parameters in both flow directions (including TCP_MSS,
> > - *     TCP_WINDOW_CLAMP socket options)
> > + *     reapplying those parameters in both flow directions (including TCP_MSS
> > + *     socket option)
> >   * - simplicity: only a small subset of TCP logic is implemented here and
> >   *   delegated as much as possible to the TCP implementations of guest and host
> >   *   kernel. This is achieved by:
> > @@ -236,12 +236,10 @@
> >   *     - update @seq_ack_from_tap from ack_seq in header
> >   *     - on two duplicated ACKs, reset @seq_to_tap to @seq_ack_from_tap, and
> >   *       resend with steps listed above
> > - *     - set TCP_WINDOW_CLAMP from TCP header from tap
> >   *
> >   * - from tap/guest to socket:
> >   *   - on packet from tap/guest:
> >   *     - set @ts_tap_act
> > - *     - set TCP_WINDOW_CLAMP from TCP header from tap
> >   *     - check seq from header against @seq_from_tap, if data is missing, send
> >   *       two ACKs with number @seq_ack_to_tap, discard packet
> >   *     - otherwise queue data to socket, set @seq_from_tap to seq from header
> > @@ -420,7 +418,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
> >  };
> >  
> >  static const char *tcp_flag_str[] __attribute((__unused__)) = {
> > -	"STALLED", "LOCAL", "WND_CLAMPED", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
> > +	"STALLED", "LOCAL", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
> >  	"ACK_FROM_TAP_DUE",
> >  };
> >  
> > @@ -1790,58 +1788,16 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
> >  
> >  /**
> >   * tcp_tap_window_update() - Process an updated window from tap side
> > - * @c:		Execution context
> >   * @conn:	Connection pointer
> >   * @window:	Window value, host order, unscaled
> >   */
> > -static void tcp_tap_window_update(const struct ctx *c,
> > -				  struct tcp_tap_conn *conn, unsigned wnd)
> > +static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
> >  {
> > -	uint32_t prev_scaled = conn->wnd_from_tap << conn->ws_from_tap;
> > -	int s = conn->sock;
> > -
> >  	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
> >  	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
> >  
> > -	/* TODO: With (at least) Linux kernel versions 6.1 to 6.5, if we end up
> > -	 * with a zero-sized window on a TCP socket, dropping data (once
> > -	 * acknowledged by the guest) with recv() and MSG_TRUNC doesn't appear
> > -	 * to be enough to make the kernel advertise a non-zero window to the
> > -	 * sender. Forcing a TCP_WINDOW_CLAMP setting, even with the existing
> > -	 * value, fixes this.
> > -	 *
> > -	 * The STALLED flag on a connection is a sufficient indication that we
> > -	 * might have a zero-sized window on the socket, because it's set if we
> > -	 * exhausted the tap-side window, or if everything we receive from a
> > -	 * socket is already in flight to the guest.
> > -	 *
> > -	 * So, if STALLED is set, and we received a window value from the tap,
> > -	 * force a TCP_WINDOW_CLAMP setsockopt(). This should be investigated
> > -	 * further and fixed in the kernel instead, if confirmed.
> > -	 */
> > -	if (!(conn->flags & STALLED) && conn->flags & WND_CLAMPED) {
> > -		if (prev_scaled == wnd)
> > -			return;
> > -
> > -		/* Discard +/- 1% updates to spare some syscalls. */
> > -		/* TODO: cppcheck, starting from commit b4d455df487c ("Fix
> > -		 * 11349: FP negativeIndex for clamped array index (#4627)"),
> > -		 * reports wnd > prev_scaled as always being true, see also:
> > -		 *
> > -		 *	https://github.com/danmar/cppcheck/pull/4627
> > -		 *
> > -		 * drop this suppression once that's resolved.
> > -		 */
> > -		/* cppcheck-suppress [knownConditionTrueFalse, unmatchedSuppression] */
> > -		if ((wnd > prev_scaled && wnd * 99 / 100 < prev_scaled) ||
> > -		    (wnd < prev_scaled && wnd * 101 / 100 > prev_scaled))
> > -			return;
> > -	}
> > -
> > -	if (setsockopt(s, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd)))
> > -		trace("TCP: failed to set TCP_WINDOW_CLAMP on socket %i", s);
> > -
> > -	conn_flag(c, conn, WND_CLAMPED);
> > +	/* FIXME: reflect the tap-side receiver's window back to the sock-side
> > +	 * sender by adjusting SO_RCVBUF? */
> 
> If we adjust SO_RCVBUF, we lose the chance to get bigger receive
> buffers by means of auto-tuning, which is especially problematic with
> default values for rmem_max (in turn, the case which is giving us
> issues here).

Yeah, I forgot about that :/.

> I'd rather fix TCP_CLAMP_WINDOW in the kernel and then re-enable this
> in passt (I guess they'll be no obvious way to probe this, so we'll
> have to rely on the kernel version).

I kind of hate checking kernel versions, because it means we can't
make use of fixes backported to distro kernels for example.

Now that we understand the cause of the problem, I can see a way to
probe for it, but it's pretty ugly:

   1. Create a pair of connected local TCP sockets A and B
   2. Set SO_SNDBUF on A and SO_RCVBUF on B as small as we're allowed
   3. Write into A until EAGAIN; since we didn't read B, we can now be
      confident the window is zero
   4. Set TCP_CLAMP_WINDOW on B to a fairly large
   5. Read and discard from B until EAGAIN, we can now be confident
      the buffer is empty
   6. Write to A: if we get EGAIN it looks like we're hitting this
      problem

There might be some edge cases where step 6 seems to succeed because
it goes into A's send buffer, but never reaches B's rcv buffer.  We
can potentially disambiguate by attempting reads on B.


But.. yeah.. it's not simple.  I wonder if we do fix this in the
kernel whether there's anything we can do to publish the fact that
this is now fixed.  Could we tweak the behaviour on
getsockopt(TCP_WINDOW_CLAMP) to advertise it maybe?

> Anyway, this comment is not so fundamental, so I would apply this
> series like it is -- the rest looks good to me.

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

Re: [PATCH 2/2] tcp: Don't use TCP_WINDOW_CLAMP

[Stefano Brivio]

On Fri, 10 Nov 2023 11:23:03 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Nov 09, 2023 at 03:51:36PM +0100, Stefano Brivio wrote:
> > On Thu,  9 Nov 2023 20:54:00 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On the L2 tap side, we see TCP headers and know the TCP window that the
> > > ultimate receiver is advertising.  In order to avoid unnecessary buffering
> > > within passt/pasta (or by the kernel on passt/pasta's behalf) we attempt
> > > to advertise that window back to the original sock-side sender using
> > > TCP_WINDOW_CLAMP.
> > > 
> > > However, TCP_WINDOW_CLAMP just doesn't work like this.  Prior to kernel
> > > commit 3aa7857fe1d7 ("tcp: enable mid stream window clamp"), it simply
> > > had no effect on established sockets.  After that commit, it does affect
> > > established sockets but doesn't behave the way we need:
> > >   * It appears to be designed only to shrink the window, not to allow it to
> > >     re-expand.
> > >   * More importantly, that commit has a serious bug where if the
> > >     setsockopt() is made when the existing kernel advertised window for the
> > >     socket happens to be zero, it will now become locked at zero, stopping
> > >     any further data from being received on the socket.
> > > 
> > > Since this has never worked as intended, simply remove it.  It might be
> > > possible to re-implement the intended behaviour by manipulating SO_RCVBUF,
> > > so we leave a comment to that effect.
> > > 
> > > This kernel bug is the underlying cause of both the linked passt bug and
> > > the linked podman bug.  We attempted to fix this before with passt commit
> > > d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag").
> > > However while that commit masked the bug for some cases, it didn't really
> > > address the problem.
> > > 
> > > Fixes: d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag")
> > > Link: https://github.com/containers/podman/issues/20170
> > > Link: https://bugs.passt.top/show_bug.cgi?id=74
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  tcp.c      | 65 ++++++++----------------------------------------------
> > >  tcp_conn.h |  7 +++---
> > >  2 files changed, 12 insertions(+), 60 deletions(-)
> > > 
> > > diff --git a/tcp.c b/tcp.c
> > > index 13e82ca..cfcd40a 100644
> > > --- a/tcp.c
> > > +++ b/tcp.c
> > > @@ -46,8 +46,8 @@
> > >   *   - avoiding port and address translations whenever possible
> > >   *   - mirroring TCP dynamics by observation of socket parameters (TCP_INFO
> > >   *     socket option) and TCP headers of packets coming from the tap interface,
> > > - *     reapplying those parameters in both flow directions (including TCP_MSS,
> > > - *     TCP_WINDOW_CLAMP socket options)
> > > + *     reapplying those parameters in both flow directions (including TCP_MSS
> > > + *     socket option)
> > >   * - simplicity: only a small subset of TCP logic is implemented here and
> > >   *   delegated as much as possible to the TCP implementations of guest and host
> > >   *   kernel. This is achieved by:
> > > @@ -236,12 +236,10 @@
> > >   *     - update @seq_ack_from_tap from ack_seq in header
> > >   *     - on two duplicated ACKs, reset @seq_to_tap to @seq_ack_from_tap, and
> > >   *       resend with steps listed above
> > > - *     - set TCP_WINDOW_CLAMP from TCP header from tap
> > >   *
> > >   * - from tap/guest to socket:
> > >   *   - on packet from tap/guest:
> > >   *     - set @ts_tap_act
> > > - *     - set TCP_WINDOW_CLAMP from TCP header from tap
> > >   *     - check seq from header against @seq_from_tap, if data is missing, send
> > >   *       two ACKs with number @seq_ack_to_tap, discard packet
> > >   *     - otherwise queue data to socket, set @seq_from_tap to seq from header
> > > @@ -420,7 +418,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
> > >  };
> > >  
> > >  static const char *tcp_flag_str[] __attribute((__unused__)) = {
> > > -	"STALLED", "LOCAL", "WND_CLAMPED", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
> > > +	"STALLED", "LOCAL", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
> > >  	"ACK_FROM_TAP_DUE",
> > >  };
> > >  
> > > @@ -1790,58 +1788,16 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
> > >  
> > >  /**
> > >   * tcp_tap_window_update() - Process an updated window from tap side
> > > - * @c:		Execution context
> > >   * @conn:	Connection pointer
> > >   * @window:	Window value, host order, unscaled
> > >   */
> > > -static void tcp_tap_window_update(const struct ctx *c,
> > > -				  struct tcp_tap_conn *conn, unsigned wnd)
> > > +static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
> > >  {
> > > -	uint32_t prev_scaled = conn->wnd_from_tap << conn->ws_from_tap;
> > > -	int s = conn->sock;
> > > -
> > >  	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
> > >  	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
> > >  
> > > -	/* TODO: With (at least) Linux kernel versions 6.1 to 6.5, if we end up
> > > -	 * with a zero-sized window on a TCP socket, dropping data (once
> > > -	 * acknowledged by the guest) with recv() and MSG_TRUNC doesn't appear
> > > -	 * to be enough to make the kernel advertise a non-zero window to the
> > > -	 * sender. Forcing a TCP_WINDOW_CLAMP setting, even with the existing
> > > -	 * value, fixes this.
> > > -	 *
> > > -	 * The STALLED flag on a connection is a sufficient indication that we
> > > -	 * might have a zero-sized window on the socket, because it's set if we
> > > -	 * exhausted the tap-side window, or if everything we receive from a
> > > -	 * socket is already in flight to the guest.
> > > -	 *
> > > -	 * So, if STALLED is set, and we received a window value from the tap,
> > > -	 * force a TCP_WINDOW_CLAMP setsockopt(). This should be investigated
> > > -	 * further and fixed in the kernel instead, if confirmed.
> > > -	 */
> > > -	if (!(conn->flags & STALLED) && conn->flags & WND_CLAMPED) {
> > > -		if (prev_scaled == wnd)
> > > -			return;
> > > -
> > > -		/* Discard +/- 1% updates to spare some syscalls. */
> > > -		/* TODO: cppcheck, starting from commit b4d455df487c ("Fix
> > > -		 * 11349: FP negativeIndex for clamped array index (#4627)"),
> > > -		 * reports wnd > prev_scaled as always being true, see also:
> > > -		 *
> > > -		 *	https://github.com/danmar/cppcheck/pull/4627
> > > -		 *
> > > -		 * drop this suppression once that's resolved.
> > > -		 */
> > > -		/* cppcheck-suppress [knownConditionTrueFalse, unmatchedSuppression] */
> > > -		if ((wnd > prev_scaled && wnd * 99 / 100 < prev_scaled) ||
> > > -		    (wnd < prev_scaled && wnd * 101 / 100 > prev_scaled))
> > > -			return;
> > > -	}
> > > -
> > > -	if (setsockopt(s, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd)))
> > > -		trace("TCP: failed to set TCP_WINDOW_CLAMP on socket %i", s);
> > > -
> > > -	conn_flag(c, conn, WND_CLAMPED);
> > > +	/* FIXME: reflect the tap-side receiver's window back to the sock-side
> > > +	 * sender by adjusting SO_RCVBUF? */  
> > 
> > If we adjust SO_RCVBUF, we lose the chance to get bigger receive
> > buffers by means of auto-tuning, which is especially problematic with
> > default values for rmem_max (in turn, the case which is giving us
> > issues here).  
> 
> Yeah, I forgot about that :/.
> 
> > I'd rather fix TCP_CLAMP_WINDOW in the kernel and then re-enable this
> > in passt (I guess they'll be no obvious way to probe this, so we'll
> > have to rely on the kernel version).  
> 
> I kind of hate checking kernel versions, because it means we can't
> make use of fixes backported to distro kernels for example.
> 
> Now that we understand the cause of the problem, I can see a way to
> probe for it, but it's pretty ugly:
> 
>    1. Create a pair of connected local TCP sockets A and B
>    2. Set SO_SNDBUF on A and SO_RCVBUF on B as small as we're allowed
>    3. Write into A until EAGAIN; since we didn't read B, we can now be
>       confident the window is zero
>    4. Set TCP_CLAMP_WINDOW on B to a fairly large
>    5. Read and discard from B until EAGAIN, we can now be confident
>       the buffer is empty
>    6. Write to A: if we get EGAIN it looks like we're hitting this
>       problem

I was also thinking of something similar, but if the fix involves not
setting rcv_ssthresh directly (that's my current thought), then we can
just open a TCP socket, read rcv_ssthresh via TCP_INFO, set a different
value via TCP_WINDOW_CLAMP, check that rcv_ssthresh matches that value.

-- 
Stefano

Re: [PATCH 0/2] Avoid bugs related to TCP_WINDOW_CLAMP

[Stefano Brivio]

On Thu,  9 Nov 2023 20:53:58 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> The way we used TCP_WINDOW_CLAMP didn't really correspond to what it
> actually did.  More recent kernels also have a nasty bug in the
> handler which we could trigger, causing TCP stalls.
> 
> David Gibson (2):
>   tcp: Rename and small cleanup to tcp_clamp_window()
>   tcp: Don't use TCP_WINDOW_CLAMP

Applied.

-- 
Stefano