Re: [PATCH v7 3/5] tcp: Resend SYN for inbound connections

On Mon, 3 Nov 2025 20:01:46 +1100 David Gibson wrote:

On Mon, Nov 03, 2025 at 10:31:21AM +0800, Yumei Huang wrote:

...

On Mon, Nov 3, 2025 at 9:10 AM David Gibson wrote:

...

On Fri, Oct 31, 2025 at 01:42:40PM +0800, Yumei Huang wrote:

...

If a client connects while guest is not connected or ready yet, resend SYN instead of just resetting connection after 10 seconds.

Use the same backoff calculation for the timeout as Linux kernel.

Link: https://bugs.passt.top/show_bug.cgi?id=153 Signed-off-by: Yumei Huang

Reviewed-by: David Gibson

There are a couple of tiny style nits and a mostly theoretical bug noted below.

...

--- tcp.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++-------- tcp.h | 4 ++++ 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/tcp.c b/tcp.c index 2ec4b0c..bada88a 100644 --- a/tcp.c +++ b/tcp.c @@ -179,9 +179,11 @@ * * Timeouts are implemented by means of timerfd timers, set based on flags: * - * - SYN_TIMEOUT: if no ACK is received from tap/guest during handshake (flag - * ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, reset the - * connection + * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest during handshake

Nit: I'd maybe say SYN,ACK here to distinguish it from other ACKs.

...

+ * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, resend + * SYN. It's the starting timeout for the first SYN retry. Retry for + * TCP_MAX_RETRIES or (tcp_syn_retries + tcp_syn_linear_timeouts) times, + * reset the connection * * - ACK_TIMEOUT: if no ACK segment was received from tap/guest, after sending * data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re-send data from the @@ -340,7 +342,7 @@ enum { #define WINDOW_DEFAULT 14600 /* RFC 6928 */

#define ACK_INTERVAL 10 /* ms */ -#define SYN_TIMEOUT 10 /* s */ +#define SYN_TIMEOUT_INIT 1 /* s, RFC 6928 */ #define ACK_TIMEOUT 2 #define FIN_TIMEOUT 60 #define ACT_TIMEOUT 7200 @@ -365,6 +367,12 @@ uint8_t tcp_migrate_rcv_queue [TCP_MIGRATE_RCV_QUEUE_MAX];

#define TCP_MIGRATE_RESTORE_CHUNK_MIN 1024 /* Try smaller when above this */

+#define TCP_SYN_RETRIES "/proc/sys/net/ipv4/tcp_syn_retries" +#define TCP_SYN_LINEAR_TIMEOUTS "/proc/sys/net/ipv4/tcp_syn_linear_timeouts" + +#define TCP_SYN_RETRIES_DEFAULT 6 +#define TCP_SYN_LINEAR_TIMEOUTS_DEFAULT 4 + /* "Extended" data (not stored in the flow table) for TCP flow migration */ static struct tcp_tap_transfer_ext migrate_ext[FLOW_MAX];

@@ -581,8 +589,10 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) if (conn->flags & ACK_TO_TAP_DUE) { it.it_value.tv_nsec = (long)ACK_INTERVAL * 1000 * 1000; } else if (conn->flags & ACK_FROM_TAP_DUE) { - if (!(conn->events & ESTABLISHED)) - it.it_value.tv_sec = SYN_TIMEOUT; + if (!(conn->events & ESTABLISHED)) { + int exp = conn->retries - c->tcp.syn_linear_timeouts; + it.it_value.tv_sec = SYN_TIMEOUT_INIT << MAX(exp, 0); + } else

A non-obvious detail of the style we use is that if one branch of an if uses { }, then the other branch should as well, even if it's one line. So above should be "} else {" and the bit below changed to match.

Oh, I didn't know about this style. Will update it later.

Since it's gone in the next patch, it doesn't really matter.

...

...

...

it.it_value.tv_sec = ACK_TIMEOUT; } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { @@ -2409,8 +2419,17 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref) tcp_timer_ctl(c, conn); } else if (conn->flags & ACK_FROM_TAP_DUE) { if (!(conn->events & ESTABLISHED)) { - flow_dbg(conn, "handshake timeout"); - tcp_rst(c, conn); + if (conn->retries >= TCP_MAX_RETRIES || + conn->retries >= (c->tcp.tcp_syn_retries + + c->tcp.syn_linear_timeouts)) {

Here's the theoretical bug. We only clamp tcp_syn_retries and syn_linear_timeouts to a uint8_t, so if the host has these set to strangely large values, this addition could overflow.

Actually, I think it won't, because it's performed as 'unsigned int', see my other email on the subject. But in any case:

...

Just checked net/ipv4/sysctl_net_ipv4.c,

static int tcp_syn_retries_min = 1; static int tcp_syn_retries_max = MAX_TCP_SYNCNT; static int tcp_syn_linear_timeouts_max = MAX_TCP_SYNCNT;

And MAX_TCP_SYNCNT is defined as 127:

#define MAX_TCP_SYNCNT 127

...these limits come and go, so...

...

they have a max value as 127, so there won't be overflows.

Ok. That's not at all obvious from within the passt code, though. Again, this is really only a theoretical problem, but I think ideally we'd clamp to 127 where we read the values out of the kernel, with a comment saying that limit is derived from the kernel's own limit.

that would sound like a good idea anyway, instead of clamping to 255 as done later in this patch. -- Stefano