Re: [PATCH v4 3/8] tcp: Rewind sequence when guest shrinks window to zero

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 10 Sep 2025 19:48:05 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> On 2025-09-09 14:16, Stefano Brivio wrote:
> > A window shrunk to zero means by definition that anything else that
> > might be in flight is now out of window. Restart from the currently
> > acknowledged sequence.
> > 
> > We need to do that both in tcp_tap_window_update(), where we already
> > check for zero-window updates, as well as in tcp_data_from_tap(),
> > because we might get one of those updates in a batch of packets that
> > also contains a non-zero window update.
> > 
> > Suggested-by: Jon Maloy <jmaloy@redhat.com>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Reviewed-by: Jon Maloy <jmaloy@redhat.com>
> But see nit below.
> > ---
> >   tcp.c | 34 +++++++++++++++++++++++++---------
> >   1 file changed, 25 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index 86e08f1..12d42e0 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -1268,19 +1268,25 @@ 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
> >    * @wnd:	Window value, host order, unscaled
> >    */
> > -static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
> > +static void tcp_tap_window_update(const struct ctx *c,
> > +				  struct tcp_tap_conn *conn, unsigned wnd)
> >   {
> >   	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
> >   
> >   	/* Work-around for bug introduced in peer kernel code, commit
> > -	 * e2142825c120 ("net: tcp: send zero-window ACK when no memory").
> > -	 * We don't update if window shrank to zero.
> > +	 * e2142825c120 ("net: tcp: send zero-window ACK when no memory"): don't
> > +	 * update the window if it shrank to zero, so that we'll eventually
> > +	 * retry to send data, but rewind the sequence as that obviously implies
> > +	 * that no data beyond the updated window will ever be acknowledged.  
> Agree with David, a strong statement.

Right, see:

  https://archives.passt.top/passt-dev/20250901220746.1b63e515@elisabeth/
  https://archives.passt.top/passt-dev/20250910083748.6b566880@elisabeth/

> >   	 */
> > -	if (!wnd && SEQ_LT(conn->seq_ack_from_tap, conn->seq_to_tap))
> > +	if (!wnd && SEQ_LT(conn->seq_ack_from_tap, conn->seq_to_tap)) {
> > +		tcp_rewind_seq(c, conn);
> >   		return;
> > +	}
> >   
> >   	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
> >   
> > @@ -1709,7 +1715,8 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> >   			tcp_timer_ctl(c, conn);
> >   
> >   			if (p->count == 1) {
> > -				tcp_tap_window_update(conn, ntohs(th->window));
> > +				tcp_tap_window_update(c, conn,
> > +						      ntohs(th->window));
> >   				return 1;
> >   			}
> >   
> > @@ -1728,6 +1735,15 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> >   				       ack_seq == max_ack_seq &&
> >   				       ntohs(th->window) == max_ack_seq_wnd;
> >   
> > +				/* See tcp_tap_window_update() for details. On
> > +				 * top of that, we also need to check here if a
> > +				 * zero-window update is contained in a batch of
> > +				 * packets that includes a non-zero window as
> > +				 * well.
> > +				 */  
> Two pleonasms in the same sentence ;-) Could possibly be shortened.

Which ones exactly? We don't necessarily have a zero-window update
here, and the batch might not include any non-zero window update either.

> > +				if (!ntohs(th->window))
> > +					tcp_rewind_seq(c, conn);
> > +
> >   				max_ack_seq_wnd = ntohs(th->window);
> >   				max_ack_seq = ack_seq;
> >   			}
> > @@ -1791,7 +1807,7 @@ static int tcp_data_from_tap(const 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(conn, max_ack_seq_wnd);
> > +	tcp_tap_window_update(c, conn, max_ack_seq_wnd);
> >   
> >   	if (retr) {
> >   		flow_trace(conn,
> > @@ -1880,7 +1896,7 @@ static void tcp_conn_from_sock_finish(const struct ctx *c,
> >   				      const struct tcphdr *th,
> >   				      const char *opts, size_t optlen)
> >   {
> > -	tcp_tap_window_update(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 */
> > @@ -2085,7 +2101,7 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
> >   		if (!th->ack)
> >   			goto reset;
> >   
> > -		tcp_tap_window_update(conn, ntohs(th->window));
> > +		tcp_tap_window_update(c, conn, ntohs(th->window));
> >   
> >   		tcp_data_from_sock(c, conn);
> >   
> > @@ -2097,7 +2113,7 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
> >   	if (conn->events & TAP_FIN_RCVD) {
> >   		tcp_sock_consume(conn, ntohl(th->ack_seq));
> >   		tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq));
> > -		tcp_tap_window_update(conn, ntohs(th->window));
> > +		tcp_tap_window_update(c, conn, ntohs(th->window));
> >   		tcp_data_from_sock(c, conn);
> >   
> >   		if (conn->events & SOCK_FIN_RCVD &&  

-- 
Stefano