Re: [PATCH 1/4] tcp: Fix ACK sequence on FIN to tap

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 3 Oct 2025 13:19:17 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Oct 02, 2025 at 01:58:41PM +0200, Stefano Brivio wrote:
> > On Thu, 2 Oct 2025 12:41:08 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Oct 02, 2025 at 02:06:43AM +0200, Stefano Brivio wrote:  
> > > > If we reach end-of-file on a socket (or get EPOLLRDHUP / EPOLLHUP) and
> > > > send a FIN segment to the guest / container acknowledging a sequence
> > > > number that's behind what we received so far, we won't have any
> > > > further trigger to send an updated ACK segment, as we are now
> > > > switching the epoll socket monitoring to edge-triggered mode.
> > > > 
> > > > To avoid this situation, in tcp_update_seqack_wnd(), we set the next
> > > > acknowledgement sequence to the current observed sequence, regardless
> > > > of what was acknowledged socket-side.    
> > > 
> > > To double check my understanding: things should work if we always
> > > acknowledged everything we've received.  Acknowledging only what the
> > > peer has acked is a refinement to give the guest a view that's closer
> > > to what it would be end-to-end with the peer (which might improve the
> > > operation of flow control).  
> > 
> > Right.
> >   
> > > We can't use that refined mechanism when the socket is closing
> > > (amongst other cases), because while we can get the peer acked bytes
> > > from TCP_INFO, we can't get events when that changes, so we have no
> > > mechanism to provide updates to the guest at the right time.  So we
> > > fall back to the simpler method.
> > > 
> > > Is that correct?  
> > 
> > Also correct, yes. If you have a better idea to summarise this in the
> > comment in tcp_buf_data_from_sock() let me know.  
> 
> Hm, I might.  Or actually a way to reorganise the code that I think
> will be a bit clearer and probably allow a clearer comment too.

I would keep reworks for a later moment. Right now, it's already taking
me long enough to find a moment to investigate these issues, write these
fixes, and test them.

> > Maybe I could mention
> > EPOLLET explicitly there?  
> 
> I don't think EPOLLET is actually relevant.  Even if we had level
> triggered events, a change in bytes_acked doesn't count as an event
> (AFAIK).

It doesn't count, but with level-triggered events, we would be busy
polling bytes_acked, as you noted. I was mentioning EPOLLET because it
could be taken, intuitively, as a "stop listening for events" (almost)
step. I'll leave that out then.

> So either some other event is on, in which case we'd
> effectively be busy polling bytes_acked, or it's not in which case we
> don't get updates, just like now.
> 
> I principle we could implement some sort of timer based polling, but
> that sounds like way more trouble than it's worth.

We already have something similar, based on ACK_INTERVAL and
ACK_TO_TAP_DUE, and it shouldn't be overly complicated to extend that
to a new FIN_TO_TAP_DUE flag. But indeed beyond the scope of this
series.

> > > > However, we don't necessarily call tcp_update_seqack_wnd() before
> > > > sending the FIN segment, which might potentially lead to a situation,
> > > > not observed in practice, where we unnecessarily cause a
> > > > retransmission at some point after our FIN segment.
> > > > 
> > > > Avoid that by setting the ACK sequence to whatever we received from
> > > > the container / guest, before sending a FIN segment and switching to
> > > > EPOLLET.
> > > > 
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>    
> > > 
> > > Based on my understanding above, this looks correct to me, so,
> > > 
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > 
> > > My only concern is whether we could instead insert an extra call to
> > > tcp_update_seqack_wnd() to reduce duplicated logic.  
> > 
> > Hmm, maybe, but on the other hand we're closing the connection. Should
> > we really spend time querying TCP_INFO to recalculate the window at
> > this point? I wouldn't.  
> 
> Good point.  I mean tcp_update_seqack_wnd() could skip the TCP_INFO in
> that case, but that does look a bit fiddly.
> 
> On the other hand, in favour of not duplicating logic...
> 
> [snip]
> > > > @@ -368,7 +368,19 @@ int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
> > > >  			conn_flag(c, conn, STALLED);
> > > >  		} else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) ==
> > > >  			   SOCK_FIN_RCVD) {
> > > > -			int ret = tcp_buf_send_flag(c, conn, FIN | ACK);
> > > > +			int ret;
> > > > +
> > > > +			/* On TAP_FIN_SENT, we won't get further data events
> > > > +			 * from the socket, and this might be the last ACK
> > > > +			 * segment we send to the tap, so update its sequence to
> > > > +			 * include everything we received until now.
> > > > +			 *
> > > > +			 * See also the special handling on CONN_IS_CLOSING() in
> > > > +			 * tcp_update_seqack_wnd().
> > > > +			 */
> > > > +			conn->seq_ack_to_tap = conn->seq_from_tap;  
> 
> ... the equivalent bits in tcp_update_seqack_wnd() have after them:
> 	if (SEQ_LT(conn->seq_ack_to_tap, prev_ack_to_tap))
> 		conn->seq_ack_to_tap = prev_ack_to_tap;
> 
> Don't we need that here as well, in case the guest is retransmitting
> when we get the sock side FIN?

Not really, because we don't rewind conn->seq_from_tap, so we don't
risk jumping back here.

In tcp_update_seqack_wnd(), we might jump back (that should be double
checked eventually, I'm not sure it's still the case) if we happened to
acknowledge more than acknowledged socket-side while handling some
particular condition, and then we switch back to acknowledging only
bytes_acked.

It should happen if the destination is/was a low RTT one, but we run
out of slots in low_rtt_dst in favour of other entries. I don't
remember any other case.

> > > > +
> > > > +			ret = tcp_buf_send_flag(c, conn, FIN | ACK);
> > > >  			if (ret) {
> > > >  				tcp_rst(c, conn);
> > > >  				return ret;
> > > > diff --git a/tcp_vu.c b/tcp_vu.c
> > > > index ebd3a1e..3ec3538 100644
> > > > --- a/tcp_vu.c
> > > > +++ b/tcp_vu.c
> > > > @@ -410,7 +410,12 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
> > > >  			conn_flag(c, conn, STALLED);
> > > >  		} else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) ==
> > > >  			   SOCK_FIN_RCVD) {
> > > > -			int ret = tcp_vu_send_flag(c, conn, FIN | ACK);
> > > > +			int ret;
> > > > +
> > > > +			/* See tcp_buf_data_from_sock() */
> > > > +			conn->seq_ack_to_tap = conn->seq_from_tap;
> > > > +
> > > > +			ret = tcp_vu_send_flag(c, conn, FIN | ACK);
> > > >  			if (ret) {
> > > >  				tcp_rst(c, conn);
> > > >  				return ret;
> > > > -- 
> > > > 2.43.0
> > > >     

-- 
Stefano