On Wed, 10 Sep 2025 12:20:19 +1000 David Gibson wrote:

On Tue, Sep 09, 2025 at 08:16:50PM +0200, 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 Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

Though a couple of documentation nits 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.

As noted earlier "will ever be acknowledged" might be a bit misleading. Maybe "no data beyond the window will be acknowledged until it is retransmitted".

Sorry, I actually fixed the patch after your same comment on v3 and then for some reason I threw away the changes together with some of your Reviewed-by: tags. Fixed as we discussed on v3 now.

...

*/ - 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.

I'm not 100% convinced of this reasoning. But at worst this should result in some unnecessary but mostly harmless retransmits, and it seems to fix the problem empirically, so I'm not suggesting changing it at this time.

Right, strictly speaking, it *might* be that the peer didn't actually throw data away, which should be indicated by the fact that the same data is acknowledged in another segment of this same batch. But handling that without making this part unreasonably complicated implies a refactor of this whole thing: we should extract a function dealing with sequences and perhaps another one with window updates from here. I plan to file a ticket or two with stuff that emerged from this series. -- Stefano

On Tue, Sep 09, 2025 at 08:16:53PM +0200, Stefano Brivio wrote:

Otherwise, passing signed types causes automatic promotion of the result of the subtractions as well, which is not what we want, as these macros rely on unsigned 32-bit arithmetic.

The next patch introduces a ssize_t operand for SEQ_LE, illustrating the issue.

Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson C macros and promotion rules strike again :/. Personally I'd probably switch to inline functions, but this is fine too.

--- tcp_internal.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tcp_internal.h b/tcp_internal.h index d0009f8..ce6fee2 100644 --- a/tcp_internal.h +++ b/tcp_internal.h @@ -21,10 +21,14 @@ sizeof(struct ipv6hdr), \ sizeof(uint32_t))

-#define SEQ_LE(a, b) ((b) - (a) < MAX_WINDOW) -#define SEQ_LT(a, b) ((b) - (a) - 1 < MAX_WINDOW) -#define SEQ_GE(a, b) ((a) - (b) < MAX_WINDOW) -#define SEQ_GT(a, b) ((a) - (b) - 1 < MAX_WINDOW) +#define SEQ_LE(a, b) \ + ((uint32_t)(b) - (uint32_t)(a) < MAX_WINDOW) +#define SEQ_LT(a, b) \ + ((uint32_t)(b) - (uint32_t)(a) - 1 < MAX_WINDOW) +#define SEQ_GE(a, b) \ + ((uint32_t)(a) - (uint32_t)(b) < MAX_WINDOW) +#define SEQ_GT(a, b) \ + ((uint32_t)(a) - (uint32_t)(b) - 1 < MAX_WINDOW)

#define FIN (1 << 0) #define SYN (1 << 1) -- 2.43.0

-- David Gibson (he or they) | 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

On Tue, Sep 09, 2025 at 08:16:54PM +0200, Stefano Brivio wrote:

We currently have a number of discrepancies in the tcp_tap_handler() path between the half-closed connection path and the regular one, and they are mostly a result of code duplication, which comes in turn from the fact that tcp_data_from_tap() deals with data transfers as well as general connection bookkeeping, so we can't use it for half-closed connections.

This suggests that we should probably rework it into two or more functions, in the long term, but for the moment being I'm just fixing one obvious issue, which is the lack of fast retransmissions in the TAP_FIN_RCVD path, and a potential one, which is the fact we don't handle socket flush failures.

Add fast re-transmit for half-closed connections, and handle the case of socket flush (tcp_sock_consume()) flush failure in the same way as tcp_data_from_tap() handles it.

Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

--- tcp.c | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/tcp.c b/tcp.c index 9c70a25..5163dbf 100644 --- a/tcp.c +++ b/tcp.c @@ -1652,6 +1652,23 @@ static int tcp_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn) return tcp_buf_data_from_sock(c, conn); }

+/** + * tcp_packet_data_len() - Get data (TCP payload) length for a TCP packet + * @th: Pointer to TCP header + * @l4len: TCP packet length, including TCP header + * + * Return: data length of TCP packet, -1 on invalid value of Data Offset field + */ +static ssize_t tcp_packet_data_len(const struct tcphdr *th, size_t l4len) +{ + size_t off = th->doff * 4UL; + + if (off < sizeof(*th) || off > l4len) + return -1; + + return l4len - off; +} + /** * tcp_data_from_tap() - tap/guest data for established connection * @c: Execution context @@ -2113,9 +2130,28 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,

/* Established connections not accepting data from tap */ 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)); - if (tcp_tap_window_update(c, conn, ntohs(th->window))) + bool retr; + + retr = th->ack && !tcp_packet_data_len(th, l4len) && !th->fin &&

Not really in scope here, but I wonder if we should log an error and/or RST if we get a non-zero data length in this situation.

+ ntohl(th->ack_seq) == conn->seq_ack_from_tap && + ntohs(th->window) == conn->wnd_from_tap; + + /* On socket flush failure, pretend there was no ACK, try again + * later + */ + if (th->ack && !tcp_sock_consume(conn, ntohl(th->ack_seq))) + tcp_update_seqack_from_tap(c, conn, ntohl(th->ack_seq)); + + if (retr) { + flow_trace(conn, + "fast re-transmit, ACK: %u, previous sequence: %u", + ntohl(th->ack_seq), conn->seq_to_tap); + + if (tcp_rewind_seq(c, conn)) + return -1; + } + + if (tcp_tap_window_update(c, conn, ntohs(th->window)) || retr) tcp_data_from_sock(c, conn);

if (conn->seq_ack_from_tap == conn->seq_to_tap) { -- 2.43.0

-- David Gibson (he or they) | 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

On Tue, Sep 09, 2025 at 08:16:55PM +0200, Stefano Brivio wrote:

For some reason, tcp_vu_data_from_sock() already takes care of this, but the non-vhost-user version ignores this possibility and just sends out a FIN segment whenever we infer we received one host-side, regardless of the fact that we might have unacknowledged data still to send.

Somewhat surprisingly, this didn't cause any issue to be reported yet, until 6.17-rc1 and 1d2fbaad7cd8 ("tcp: stronger sk_rcvbuf checks") came around, leading to the following report from Paul, who hit this running Podman tests:

439 0.033032 169.254.1.2 → 192.168.122.100 65540 TCP 56602 → 5789 [PSH, ACK] Seq=10336361 Ack=1 Win=65536 Len=65480 440 0.033055 169.254.1.2 → 192.168.122.100 30324 TCP [TCP Window Full] 56602 → 5789 [PSH, ACK] Seq=10401841 Ack=1 Win=65536 Len=30264

we're sending data to the container, up to the edge of the window

441 0.033059 192.168.122.100 → 169.254.1.2 60 TCP 5789 → 56602 [ACK] Seq=1 Ack=10401841 Win=83968 Len=0

and the container acknowledges it

442 0.033091 169.254.1.2 → 192.168.122.100 53716 TCP 56602 → 5789 [PSH, ACK] Seq=10432105 Ack=1 Win=65536 Len=53656

we send more data, all we possibly can, in window

443 0.033126 192.168.122.100 → 169.254.1.2 60 TCP [TCP ZeroWindow] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=0 Len=0

and the container shrinks the window due to the issue introduced by kernel commit e2142825c120 ("net: tcp: send zero-window ACK when no memory"). With a previous patch from this series, we rewind the sequence, meaning that we assign conn->seq_to_tap from conn->seq_ack_from_tap, so that we'll retransmit this segment, by reading again from the socket, and increasing conn->seq_to_tap once more.

However:

444 0.033144 169.254.1.2 → 192.168.122.100 60 TCP 56602 → 5789 [FIN, PSH, ACK] Seq=10485761 Ack=1 Win=65536 Len=0

we eventually get a zero-length read from the socket and we miss the fact that conn->seq_to_tap != conn->seq_ack_from_tap, so we send a FIN flag with the most recent sequence. The kernel insists:

445 0.033147 192.168.122.100 → 169.254.1.2 60 TCP [TCP ZeroWindow] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=0 Len=0

with its buggy zero-window update, but:

446 0.033152 192.168.122.100 → 169.254.1.2 60 TCP [TCP Window Update] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=69120 Len=0 447 0.033202 192.168.122.100 → 169.254.1.2 60 TCP [TCP Window Update] 5789 → 56602 [ACK] Seq=1 Ack=10432105 Win=142848 Len=0

we don't reset the TAP_FIN_SENT flag anymore, and don't resend the FIN segment (nor data), as we already rewound the sequence earlier.

To solve this, hold off the FIN segment until we get a zero-length read from the socket *and* we know that there's no unacknowledged pending data, also without vhost-user, in tcp_buf_data_from_sock().

Reported-by: Paul Holzinger Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson At least, I think this makes sense. As always, I find the semantics of the STALLED flag difficult to wrap my head around.

--- tcp_buf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tcp_buf.c b/tcp_buf.c index 4ebb013..49bddbe 100644 --- a/tcp_buf.c +++ b/tcp_buf.c @@ -363,7 +363,10 @@ int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn) }

if (!len) { - if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == SOCK_FIN_RCVD) { + if (already_sent) { + 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); if (ret) { tcp_rst(c, conn); -- 2.43.0

-- David Gibson (he or they) | 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