[PATCH v4 0/4] Retry SYNs for inbound connections

older
[PATCH v5 3/4] tcp: Resend SYN for...

Yumei Huang

16 Oct 2025 16 Oct '25

4:34 a.m.

When a client connects, SYN would be sent to guest only once. If the guest is not connected or ready at that time, the connection will be reset in 10s. These patches introduce the SYN retry mechanism using the similar backoff timeout as linux kernel. Also update the data retransmission timeout using the backoff timeout. Yumei Huang (4): tcp: Rename "retrans" to "retries" util: Introduce read_file() and read_file_integer() function tcp: Resend SYN for inbound connections tcp: Update data retransmission timeout tcp.c | 74 ++++++++++++++++++++++++++++++++++------------- tcp.h | 2 ++ tcp_conn.h | 12 ++++---- util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 3 ++ 5 files changed, 149 insertions(+), 26 deletions(-) -- 2.47.0

Show replies by date

Yumei Huang

16 Oct 16 Oct

4:34 a.m.

New subject: [PATCH v4 1/4] tcp: Rename "retrans" to "retries"

Rename "retrans" to "retries" so it can be used for SYN retries. Signed-off-by: Yumei Huang Reviewed-by: David Gibson --- tcp.c | 12 ++++++------ tcp_conn.h | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tcp.c b/tcp.c index 0f9e9b3..2ec4b0c 100644 --- a/tcp.c +++ b/tcp.c @@ -186,7 +186,7 @@ * - 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 * socket and reset sequence to what was acknowledged. If this persists for - * more than TCP_MAX_RETRANS times in a row, reset the connection + * more than TCP_MAX_RETRIES times in a row, reset the connection * * - FIN_TIMEOUT: if a FIN segment was sent to tap/guest (flag ACK_FROM_TAP_DUE * with TAP_FIN_SENT event), and no ACK is received within this time, reset @@ -1127,7 +1127,7 @@ static void tcp_update_seqack_from_tap(const struct ctx *c, if (SEQ_LT(seq, conn->seq_to_tap)) conn_flag(c, conn, ACK_FROM_TAP_DUE); - conn->retrans = 0; + conn->retries = 0; conn->seq_ack_from_tap = seq; } } @@ -2414,7 +2414,7 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref) } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { flow_dbg(conn, "FIN timeout"); tcp_rst(c, conn); - } else if (conn->retrans == TCP_MAX_RETRANS) { + } else if (conn->retries == TCP_MAX_RETRIES) { flow_dbg(conn, "retransmissions count exceeded"); tcp_rst(c, conn); } else { @@ -2423,7 +2423,7 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref) if (!conn->wnd_from_tap) conn->wnd_from_tap = 1; /* Zero-window probe */ - conn->retrans++; + conn->retries++; if (tcp_rewind_seq(c, conn)) return; @@ -3382,7 +3382,7 @@ static int tcp_flow_repair_opt(const struct tcp_tap_conn *conn, int tcp_flow_migrate_source(int fd, struct tcp_tap_conn *conn) { struct tcp_tap_transfer t = { - .retrans = conn->retrans, + .retries = conn->retries, .ws_from_tap = conn->ws_from_tap, .ws_to_tap = conn->ws_to_tap, .events = conn->events, @@ -3662,7 +3662,7 @@ int tcp_flow_migrate_target(struct ctx *c, int fd) memcpy(&flow->f.side, &t.side, sizeof(flow->f.side)); conn = FLOW_SET_TYPE(flow, FLOW_TCP, tcp); - conn->retrans = t.retrans; + conn->retries = t.retries; conn->ws_from_tap = t.ws_from_tap; conn->ws_to_tap = t.ws_to_tap; conn->events = t.events; diff --git a/tcp_conn.h b/tcp_conn.h index 38b5c54..e5c8146 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -13,7 +13,7 @@ * struct tcp_tap_conn - Descriptor for a TCP connection (not spliced) * @f: Generic flow information * @in_epoll: Is the connection in the epoll set? - * @retrans: Number of retransmissions occurred due to ACK_TIMEOUT + * @retries: Number of retries occurred due to timeouts * @ws_from_tap: Window scaling factor advertised from tap/guest * @ws_to_tap: Window scaling factor advertised to tap/guest * @tap_mss: MSS advertised by tap/guest, rounded to 2 ^ TCP_MSS_BITS @@ -38,9 +38,9 @@ struct tcp_tap_conn { bool in_epoll :1; -#define TCP_RETRANS_BITS 3 - unsigned int retrans :TCP_RETRANS_BITS; -#define TCP_MAX_RETRANS MAX_FROM_BITS(TCP_RETRANS_BITS) +#define TCP_RETRIES_BITS 3 + unsigned int retries :TCP_RETRIES_BITS; +#define TCP_MAX_RETRIES MAX_FROM_BITS(TCP_RETRIES_BITS) #define TCP_WS_BITS 4 /* RFC 7323 */ #define TCP_WS_MAX 14 @@ -102,7 +102,7 @@ struct tcp_tap_conn { * struct tcp_tap_transfer - Migrated TCP data, flow table part, network order * @pif: Interfaces for each side of the flow * @side: Addresses and ports for each side of the flow - * @retrans: Number of retransmissions occurred due to ACK_TIMEOUT + * @retries: Number of retries occurred due to timeouts * @ws_from_tap: Window scaling factor advertised from tap/guest * @ws_to_tap: Window scaling factor advertised to tap/guest * @events: Connection events, implying connection states @@ -122,7 +122,7 @@ struct tcp_tap_transfer { uint8_t pif[SIDES]; struct flowside side[SIDES]; - uint8_t retrans; + uint8_t retries; uint8_t ws_from_tap; uint8_t ws_to_tap; uint8_t events; -- 2.47.0

Yumei Huang

4:34 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

Signed-off-by: Yumei Huang --- util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 3 +++ 2 files changed, 87 insertions(+) diff --git a/util.c b/util.c index c492f90..197677e 100644 --- a/util.c +++ b/util.c @@ -579,6 +579,90 @@ int write_file(const char *path, const char *buf) return len == 0 ? 0 : -1; } +/** + * read_file() - Read contents of file into a buffer + * @path: File to read + * @buf: Buffer to store file contents + * @buf_size: Size of buffer + * + * Return: number of bytes read on success, -1 on any error, -2 on truncation +*/ +int read_file(const char *path, char *buf, size_t buf_size) +{ + int fd = open(path, O_RDONLY | O_CLOEXEC); + size_t total_read = 0; + ssize_t rc; + + if (fd < 0) { + warn_perror("Could not open %s", path); + return -1; + } + + while (total_read < buf_size) { + rc = read(fd, buf + total_read, buf_size - total_read); + + if (rc < 0) { + warn_perror("Couldn't read from %s", path); + close(fd); + return -1; + } + + if (rc == 0) + break; + + total_read += rc; + } + + close(fd); + + if (total_read == buf_size) { + warn_perror("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return (int)total_read; +} + +/** + * read_file_integer() - Read an integer value from a file + * @path: File to read + * @fallback: Default value if file can't be read + * + * Return: Integer value, fallback on failure +*/ +intmax_t read_file_integer(const char *path, intmax_t fallback) +{ + char buf[INTMAX_STRLEN]; + char *end; + intmax_t value; + int bytes_read; + + bytes_read = read_file(path, buf, sizeof(buf)); + + if (bytes_read < 0) + return fallback; + + if (bytes_read == 0) { + debug("Empty file %s", path); + return fallback; + } + + errno = 0; + value = strtoimax(buf, &end, 10); + if (*end && *end != '\n') { + debug("Invalid format in %s", path); + return fallback; + } + if (errno) { + debug("Invalid value in %s: %s", path, buf); + return fallback; + } + + return value; +} + #ifdef __ia64__ /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(), * use the description from clone(2). diff --git a/util.h b/util.h index 22eaac5..887d795 100644 --- a/util.h +++ b/util.h @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +int read_file(const char *path, char *buf, size_t buf_size); +intmax_t read_file_integer(const char *path, intmax_t fallback); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) } #define UINT16_STRLEN (sizeof("65535")) +#define INTMAX_STRLEN (sizeof("-9223372036854775808")) /* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */ #define SOCKADDR_INET_STRLEN \ -- 2.47.0

Yumei Huang

4:34 a.m.

New subject: [PATCH v4 3/4] tcp: Resend SYN for inbound connections

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. Signed-off-by: Yumei Huang --- tcp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-------- tcp.h | 2 ++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/tcp.c b/tcp.c index 2ec4b0c..3003333 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 + * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, resend + * SYN. It's the starting timeout for the first SYN retry. If this persists + * for more than TCP_MAX_RETRIES or (tcp_syn_retries + + * tcp_syn_linear_timeouts) times in a row, 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 */ #define ACK_TIMEOUT 2 #define FIN_TIMEOUT 60 #define ACT_TIMEOUT 7200 @@ -365,6 +367,10 @@ 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_SYSCTL "/proc/sys/net/ipv4/tcp_syn_retries" +#define TCP_SYN_LINEAR_TIMEOUTS_SYSCTL \ + "/proc/sys/net/ipv4/tcp_syn_linear_timeouts" + /* "Extended" data (not stored in the flow table) for TCP flow migration */ static struct tcp_tap_transfer_ext migrate_ext[FLOW_MAX]; @@ -581,8 +587,13 @@ 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)) { + if (conn->retries < c->tcp.syn_linear_timeouts) + it.it_value.tv_sec = SYN_TIMEOUT_INIT; + else + it.it_value.tv_sec = SYN_TIMEOUT_INIT << + (conn->retries - c->tcp.syn_linear_timeouts); + } else it.it_value.tv_sec = ACK_TIMEOUT; } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { @@ -2409,8 +2420,16 @@ 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 >= MIN(TCP_MAX_RETRIES, + (c->tcp.tcp_syn_retries + c->tcp.syn_linear_timeouts))) { + flow_dbg(conn, "handshake timeout"); + tcp_rst(c, conn); + } else { + flow_trace(conn, "SYN timeout, retry"); + tcp_send_flag(c, conn, SYN); + conn->retries++; + tcp_timer_ctl(c, conn); + } } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { flow_dbg(conn, "FIN timeout"); tcp_rst(c, conn); @@ -2766,6 +2785,24 @@ static socklen_t tcp_probe_tcp_info(void) return sl; } +/** + * tcp_syn_params_init() - Get initial syn params for inbound connection + * @c: Execution context +*/ +void tcp_syn_params_init(struct ctx *c) +{ + long tcp_syn_retries, syn_linear_timeouts; + + tcp_syn_retries = read_file_integer(TCP_SYN_RETRIES_SYSCTL, 8); + syn_linear_timeouts = read_file_integer(TCP_SYN_LINEAR_TIMEOUTS_SYSCTL, 1); + + c->tcp.tcp_syn_retries = (uint8_t)MIN(tcp_syn_retries, UINT8_MAX); + c->tcp.syn_linear_timeouts = (uint8_t)MIN(syn_linear_timeouts, UINT8_MAX); + + debug("TCP SYN parameters: retries=%"PRIu8", linear_timeouts=%"PRIu8, + c->tcp.tcp_syn_retries, c->tcp.syn_linear_timeouts); +} + /** * tcp_init() - Get initial sequence, hash secret, initialise per-socket data * @c: Execution context @@ -2776,6 +2813,8 @@ int tcp_init(struct ctx *c) { ASSERT(!c->no_tcp); + tcp_syn_params_init(c); + tcp_sock_iov_init(c); memset(init_sock_pool4, 0xff, sizeof(init_sock_pool4)); diff --git a/tcp.h b/tcp.h index 234a803..df699a4 100644 --- a/tcp.h +++ b/tcp.h @@ -65,6 +65,8 @@ struct tcp_ctx { struct fwd_ports fwd_out; struct timespec timer_run; size_t pipe_size; + uint8_t tcp_syn_retries; + uint8_t syn_linear_timeouts; }; #endif /* TCP_H */ -- 2.47.0

Yumei Huang

4:34 a.m.

New subject: [PATCH v4 4/4] tcp: Update data retransmission timeout

Use an exponential backoff timeout for data retransmission according to RFC 2988 and RFC 6298. Set the initial RTO to one second as discussed in Appendix A of RFC 6298. Also combine the macros defining the initial RTO for both SYN and ACK. Signed-off-by: Yumei Huang --- tcp.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/tcp.c b/tcp.c index 3003333..3254d67 100644 --- a/tcp.c +++ b/tcp.c @@ -179,16 +179,12 @@ * * Timeouts are implemented by means of timerfd timers, set based on flags: * - * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest during handshake - * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, resend - * SYN. It's the starting timeout for the first SYN retry. If this persists - * for more than TCP_MAX_RETRIES or (tcp_syn_retries + - * tcp_syn_linear_timeouts) times in a row, 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 - * socket and reset sequence to what was acknowledged. If this persists for - * more than TCP_MAX_RETRIES times in a row, reset the connection + * - RTO_INIT: if no ACK segment was received from tap/guest, either during + * handshake (flag ACK_FROM_TAP_DUE without ESTABLISHED event) or after + * sending data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re-send data + * from the socket and reset sequence to what was acknowledged. It's the + * starting timeout for the first retry. If this persists for more than + * allowed times in a row, reset the connection * * - FIN_TIMEOUT: if a FIN segment was sent to tap/guest (flag ACK_FROM_TAP_DUE * with TAP_FIN_SENT event), and no ACK is received within this time, reset @@ -342,8 +338,7 @@ enum { #define WINDOW_DEFAULT 14600 /* RFC 6928 */ #define ACK_INTERVAL 10 /* ms */ -#define SYN_TIMEOUT_INIT 1 /* s */ -#define ACK_TIMEOUT 2 +#define RTO_INIT 1 /* s, RFC 6298 */ #define FIN_TIMEOUT 60 #define ACT_TIMEOUT 7200 @@ -589,13 +584,13 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) } else if (conn->flags & ACK_FROM_TAP_DUE) { if (!(conn->events & ESTABLISHED)) { if (conn->retries < c->tcp.syn_linear_timeouts) - it.it_value.tv_sec = SYN_TIMEOUT_INIT; + it.it_value.tv_sec = RTO_INIT; else - it.it_value.tv_sec = SYN_TIMEOUT_INIT << + it.it_value.tv_sec = RTO_INIT << (conn->retries - c->tcp.syn_linear_timeouts); } else - it.it_value.tv_sec = ACK_TIMEOUT; + it.it_value.tv_sec = RTO_INIT << conn->retries; } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { it.it_value.tv_sec = FIN_TIMEOUT; } else { -- 2.47.0

David Gibson

8:30 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote:

...

Signed-off-by: Yumei Huang --- util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 3 +++ 2 files changed, 87 insertions(+)

diff --git a/util.c b/util.c index c492f90..197677e 100644 --- a/util.c +++ b/util.c @@ -579,6 +579,90 @@ int write_file(const char *path, const char *buf) return len == 0 ? 0 : -1; }

+/** + * read_file() - Read contents of file into a buffer + * @path: File to read + * @buf: Buffer to store file contents + * @buf_size: Size of buffer + * + * Return: number of bytes read on success, -1 on any error, -2 on truncation +*/ +int read_file(const char *path, char *buf, size_t buf_size) +{ + int fd = open(path, O_RDONLY | O_CLOEXEC); + size_t total_read = 0; + ssize_t rc; + + if (fd < 0) { + warn_perror("Could not open %s", path); + return -1; + } + + while (total_read < buf_size) { + rc = read(fd, buf + total_read, buf_size - total_read); + + if (rc < 0) { + warn_perror("Couldn't read from %s", path); + close(fd); + return -1; + } + + if (rc == 0) + break; + + total_read += rc; + } + + close(fd); + + if (total_read == buf_size) { + warn_perror("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return (int)total_read;

Probably makes more sense for total_read and the return type to be ssize_t.

...

+} + +/** + * read_file_integer() - Read an integer value from a file + * @path: File to read + * @fallback: Default value if file can't be read + * + * Return: Integer value, fallback on failure +*/ +intmax_t read_file_integer(const char *path, intmax_t fallback) +{ + char buf[INTMAX_STRLEN]; + char *end;

passt coding style is to list (where possible) local variables in reverse order of line length, so this should go after bytes_read.

...

+ intmax_t value; + int bytes_read; + + bytes_read = read_file(path, buf, sizeof(buf)); + + if (bytes_read < 0) + return fallback; + + if (bytes_read == 0) { + debug("Empty file %s", path); + return fallback; + } + + errno = 0; + value = strtoimax(buf, &end, 10); + if (*end && *end != '\n') { + debug("Invalid format in %s", path); + return fallback; + } + if (errno) { + debug("Invalid value in %s: %s", path, buf); + return fallback; + } + + return value; +} + #ifdef __ia64__ /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(), * use the description from clone(2). diff --git a/util.h b/util.h index 22eaac5..887d795 100644 --- a/util.h +++ b/util.h @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +int read_file(const char *path, char *buf, size_t buf_size); +intmax_t read_file_integer(const char *path, intmax_t fallback); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) }

#define UINT16_STRLEN (sizeof("65535")) +#define INTMAX_STRLEN (sizeof("-9223372036854775808"))

It's correct for now, and probably for any systems we're likely to run on, but I dislike hard-assuming the size of intmax_t here. I feel like there must be a better way to derive the correct string length, but I haven't figured out what it is yet :(.

...

/* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */ #define SOCKADDR_INET_STRLEN \ -- 2.47.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

Yumei Huang

9:49 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Thu, Oct 16, 2025 at 2:30 PM David Gibson wrote:

...

On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote:

...
Signed-off-by: Yumei Huang --- util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 3 +++ 2 files changed, 87 insertions(+)

diff --git a/util.c b/util.c index c492f90..197677e 100644 --- a/util.c +++ b/util.c @@ -579,6 +579,90 @@ int write_file(const char *path, const char *buf) return len == 0 ? 0 : -1; }

+/** + * read_file() - Read contents of file into a buffer + * @path: File to read + * @buf: Buffer to store file contents + * @buf_size: Size of buffer + * + * Return: number of bytes read on success, -1 on any error, -2 on truncation +*/ +int read_file(const char *path, char *buf, size_t buf_size) +{ + int fd = open(path, O_RDONLY | O_CLOEXEC); + size_t total_read = 0; + ssize_t rc; + + if (fd < 0) { + warn_perror("Could not open %s", path); + return -1; + } + + while (total_read < buf_size) { + rc = read(fd, buf + total_read, buf_size - total_read); + + if (rc < 0) { + warn_perror("Couldn't read from %s", path); + close(fd); + return -1; + } + + if (rc == 0) + break; + + total_read += rc; + } + + close(fd); + + if (total_read == buf_size) { + warn_perror("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return (int)total_read;

Probably makes more sense for total_read and the return type to be ssize_t.

Just tried to be consistent with write_file(). I can change it to ssize_t if needed.

...

...
+} + +/** + * read_file_integer() - Read an integer value from a file + * @path: File to read + * @fallback: Default value if file can't be read + * + * Return: Integer value, fallback on failure +*/ +intmax_t read_file_integer(const char *path, intmax_t fallback) +{ + char buf[INTMAX_STRLEN]; + char *end;

passt coding style is to list (where possible) local variables in reverse order of line length, so this should go after bytes_read.

Oh, I didn't notice that. Will update later.

...

...
+ intmax_t value; + int bytes_read; + + bytes_read = read_file(path, buf, sizeof(buf)); + + if (bytes_read < 0) + return fallback; + + if (bytes_read == 0) { + debug("Empty file %s", path); + return fallback; + } + + errno = 0; + value = strtoimax(buf, &end, 10); + if (*end && *end != '\n') { + debug("Invalid format in %s", path); + return fallback; + } + if (errno) { + debug("Invalid value in %s: %s", path, buf); + return fallback; + } + + return value; +} + #ifdef __ia64__ /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(), * use the description from clone(2). diff --git a/util.h b/util.h index 22eaac5..887d795 100644 --- a/util.h +++ b/util.h @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +int read_file(const char *path, char *buf, size_t buf_size); +intmax_t read_file_integer(const char *path, intmax_t fallback); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) }

#define UINT16_STRLEN (sizeof("65535")) +#define INTMAX_STRLEN (sizeof("-9223372036854775808"))

It's correct for now, and probably for any systems we're likely to run on, but I dislike hard-assuming the size of intmax_t here. I feel like there must be a better way to derive the correct string length, but I haven't figured out what it is yet :(.

How about this: #define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2) Each byte can represent about 2.4 decimal digits as below, sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null terminator. 1 bit = log₁₀(2) ≈ 0.30103 decimal digits 1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

...

...
/* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */ #define SOCKADDR_INET_STRLEN \ -- 2.47.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

-- Thanks, Yumei Huang

Stefano Brivio

17 Oct 17 Oct

12:22 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Thu, 16 Oct 2025 15:49:39 +0800 Yumei Huang wrote:

...

On Thu, Oct 16, 2025 at 2:30 PM David Gibson wrote:

...
On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote:

...
Signed-off-by: Yumei Huang --- util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 3 +++ 2 files changed, 87 insertions(+)

diff --git a/util.c b/util.c index c492f90..197677e 100644 --- a/util.c +++ b/util.c @@ -579,6 +579,90 @@ int write_file(const char *path, const char *buf) return len == 0 ? 0 : -1; }

+/** + * read_file() - Read contents of file into a buffer + * @path: File to read + * @buf: Buffer to store file contents + * @buf_size: Size of buffer + * + * Return: number of bytes read on success, -1 on any error, -2 on truncation +*/ +int read_file(const char *path, char *buf, size_t buf_size) +{ + int fd = open(path, O_RDONLY | O_CLOEXEC); + size_t total_read = 0; + ssize_t rc; + + if (fd < 0) { + warn_perror("Could not open %s", path); + return -1; + } + + while (total_read < buf_size) { + rc = read(fd, buf + total_read, buf_size - total_read); + + if (rc < 0) { + warn_perror("Couldn't read from %s", path); + close(fd); + return -1; + } + + if (rc == 0) + break; + + total_read += rc; + } + + close(fd); + + if (total_read == buf_size) { + warn_perror("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return (int)total_read;

Probably makes more sense for total_read and the return type to be ssize_t.

Just tried to be consistent with write_file(). I can change it to ssize_t if needed.

ssize_t is the type designed for this, if write_file() has it wrong (I didn't check), we should fix that as well.

...

...
...
+} + +/** + * read_file_integer() - Read an integer value from a file + * @path: File to read + * @fallback: Default value if file can't be read + * + * Return: Integer value, fallback on failure +*/ +intmax_t read_file_integer(const char *path, intmax_t fallback) +{ + char buf[INTMAX_STRLEN]; + char *end;

passt coding style is to list (where possible) local variables in reverse order of line length, so this should go after bytes_read.

Oh, I didn't notice that. Will update later.

Rationale (added to my further list for CONTRIBUTING.md): https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/ and see also https://lwn.net/Articles/758552/.

...

...
...
+ intmax_t value; + int bytes_read; + + bytes_read = read_file(path, buf, sizeof(buf)); + + if (bytes_read < 0) + return fallback; + + if (bytes_read == 0) { + debug("Empty file %s", path); + return fallback; + } + + errno = 0; + value = strtoimax(buf, &end, 10); + if (*end && *end != '\n') { + debug("Invalid format in %s", path); + return fallback; + } + if (errno) { + debug("Invalid value in %s: %s", path, buf); + return fallback; + } + + return value; +} + #ifdef __ia64__ /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(), * use the description from clone(2). diff --git a/util.h b/util.h index 22eaac5..887d795 100644 --- a/util.h +++ b/util.h @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +int read_file(const char *path, char *buf, size_t buf_size); +intmax_t read_file_integer(const char *path, intmax_t fallback); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) }

#define UINT16_STRLEN (sizeof("65535")) +#define INTMAX_STRLEN (sizeof("-9223372036854775808"))

It's correct for now, and probably for any systems we're likely to run on, but I dislike hard-assuming the size of intmax_t here. I feel like there must be a better way to derive the correct string length, but I haven't figured out what it is yet :(.

How about this:

#define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2)

Each byte can represent about 2.4 decimal digits as below, sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null terminator.

1 bit = log₁₀(2) ≈ 0.30103 decimal digits 1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

If it's sourced from https://stackoverflow.com/a/10536254 and comment, don't forget to mention that in whatever implementation / commit message. But I was thinking... what if we keep it much simpler, use BUFSIZ, and error out if the buffer is too small? It would be good to be robust against any potential kernel issue anyway, so I think we need a mechanism like that in any case. It's not a security matter, because if the kernel was compromised, we're compromised too, simply a matter of robustness. -- Stefano

Stefano Brivio

12:22 a.m.

New subject: [PATCH v4 3/4] tcp: Resend SYN for inbound connections

On Thu, 16 Oct 2025 10:34:22 +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.

Signed-off-by: Yumei Huang --- tcp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-------- tcp.h | 2 ++ 2 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/tcp.c b/tcp.c index 2ec4b0c..3003333 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 + * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, resend + * SYN. It's the starting timeout for the first SYN retry. If this persists + * for more than TCP_MAX_RETRIES or (tcp_syn_retries + + * tcp_syn_linear_timeouts) times in a row, 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 */ #define ACK_TIMEOUT 2 #define FIN_TIMEOUT 60 #define ACT_TIMEOUT 7200 @@ -365,6 +367,10 @@ 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_SYSCTL "/proc/sys/net/ipv4/tcp_syn_retries" +#define TCP_SYN_LINEAR_TIMEOUTS_SYSCTL \ + "/proc/sys/net/ipv4/tcp_syn_linear_timeouts"

It's quite obvious those are names of sysctl entries, I think we can drop the _SYSCTL suffix and keep this shorter without losing any information/indication.

...

+ /* "Extended" data (not stored in the flow table) for TCP flow migration */ static struct tcp_tap_transfer_ext migrate_ext[FLOW_MAX];

@@ -581,8 +587,13 @@ 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)) { + if (conn->retries < c->tcp.syn_linear_timeouts) + it.it_value.tv_sec = SYN_TIMEOUT_INIT; + else + it.it_value.tv_sec = SYN_TIMEOUT_INIT << + (conn->retries - c->tcp.syn_linear_timeouts); + } else it.it_value.tv_sec = ACK_TIMEOUT; } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { @@ -2409,8 +2420,16 @@ 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 >= MIN(TCP_MAX_RETRIES, + (c->tcp.tcp_syn_retries + c->tcp.syn_linear_timeouts))) {

That doesn't seem to match the sysctl documentation for tcp_syn_retries, which should be the *total* number of retries, not excluding the ones with "linear timeouts". This is pretty hard to read, by the way. It could be: if (conn->retries >= TCP_MAX_RETRIES || conn->retries >= ...) {

...

+ flow_dbg(conn, "handshake timeout"); + tcp_rst(c, conn); + } else { + flow_trace(conn, "SYN timeout, retry"); + tcp_send_flag(c, conn, SYN); + conn->retries++; + tcp_timer_ctl(c, conn); + } } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { flow_dbg(conn, "FIN timeout"); tcp_rst(c, conn); @@ -2766,6 +2785,24 @@ static socklen_t tcp_probe_tcp_info(void) return sl; }

+/** + * tcp_syn_params_init() - Get initial syn params for inbound connection

SYN, parameters

...

+ * @c: Execution context +*/ +void tcp_syn_params_init(struct ctx *c) +{ + long tcp_syn_retries, syn_linear_timeouts; + + tcp_syn_retries = read_file_integer(TCP_SYN_RETRIES_SYSCTL, 8); + syn_linear_timeouts = read_file_integer(TCP_SYN_LINEAR_TIMEOUTS_SYSCTL, 1); + + c->tcp.tcp_syn_retries = (uint8_t)MIN(tcp_syn_retries, UINT8_MAX); + c->tcp.syn_linear_timeouts = (uint8_t)MIN(syn_linear_timeouts, UINT8_MAX);

I don't think you need those casts, MIN(..., UINT8_MAX) already guarantees that the number is <= UINT8_MAX, and in any case the cast won't fix anything here.

...

+ + debug("TCP SYN parameters: retries=%"PRIu8", linear_timeouts=%"PRIu8, + c->tcp.tcp_syn_retries, c->tcp.syn_linear_timeouts); +} + /** * tcp_init() - Get initial sequence, hash secret, initialise per-socket data * @c: Execution context @@ -2776,6 +2813,8 @@ int tcp_init(struct ctx *c) { ASSERT(!c->no_tcp);

+ tcp_syn_params_init(c); + tcp_sock_iov_init(c);

memset(init_sock_pool4, 0xff, sizeof(init_sock_pool4)); diff --git a/tcp.h b/tcp.h index 234a803..df699a4 100644 --- a/tcp.h +++ b/tcp.h @@ -65,6 +65,8 @@ struct tcp_ctx { struct fwd_ports fwd_out; struct timespec timer_run; size_t pipe_size; + uint8_t tcp_syn_retries; + uint8_t syn_linear_timeouts;

These should be added to the documentation for struct tcp_ctx, above.

...

};

#endif /* TCP_H */

-- Stefano

David Gibson

1:16 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Fri, Oct 17, 2025 at 12:22:14AM +0200, Stefano Brivio wrote:

...

On Thu, 16 Oct 2025 15:49:39 +0800 Yumei Huang wrote:

...
On Thu, Oct 16, 2025 at 2:30 PM David Gibson wrote:

...
On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote:

...
Signed-off-by: Yumei Huang

[snip]

...
...
+ if (total_read == buf_size) { + warn_perror("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return (int)total_read;

Probably makes more sense for total_read and the return type to be ssize_t.

Just tried to be consistent with write_file(). I can change it to ssize_t if needed.

ssize_t is the type designed for this, if write_file() has it wrong (I didn't check), we should fix that as well.

It does, and we should :).

...

...
...
...
+} + +/** + * read_file_integer() - Read an integer value from a file + * @path: File to read + * @fallback: Default value if file can't be read + * + * Return: Integer value, fallback on failure +*/ +intmax_t read_file_integer(const char *path, intmax_t fallback) +{ + char buf[INTMAX_STRLEN]; + char *end;

passt coding style is to list (where possible) local variables in reverse order of line length, so this should go after bytes_read.

Oh, I didn't notice that. Will update later.

Rationale (added to my further list for CONTRIBUTING.md):

https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

and see also https://lwn.net/Articles/758552/.

If you want to update CONTRIBUTING.md to cover this, Yumei, that would be much appreciated.

...

...
...
...
+ intmax_t value; + int bytes_read; + + bytes_read = read_file(path, buf, sizeof(buf)); + + if (bytes_read < 0) + return fallback; + + if (bytes_read == 0) { + debug("Empty file %s", path); + return fallback; + } + + errno = 0; + value = strtoimax(buf, &end, 10); + if (*end && *end != '\n') { + debug("Invalid format in %s", path); + return fallback; + } + if (errno) { + debug("Invalid value in %s: %s", path, buf); + return fallback; + } + + return value; +} + #ifdef __ia64__ /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(), * use the description from clone(2). diff --git a/util.h b/util.h index 22eaac5..887d795 100644 --- a/util.h +++ b/util.h @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +int read_file(const char *path, char *buf, size_t buf_size); +intmax_t read_file_integer(const char *path, intmax_t fallback); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) }

#define UINT16_STRLEN (sizeof("65535")) +#define INTMAX_STRLEN (sizeof("-9223372036854775808"))

It's correct for now, and probably for any systems we're likely to run on, but I dislike hard-assuming the size of intmax_t here. I feel like there must be a better way to derive the correct string length, but I haven't figured out what it is yet :(.

How about this:

#define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2)

Each byte can represent about 2.4 decimal digits as below, sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null terminator.

1 bit = log₁₀(2) ≈ 0.30103 decimal digits 1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

Works for me.

...

If it's sourced from https://stackoverflow.com/a/10536254 and comment, don't forget to mention that in whatever implementation / commit message.

Good point.

...

But I was thinking... what if we keep it much simpler, use BUFSIZ, and error out if the buffer is too small? It would be good to be robust against any potential kernel issue anyway, so I think we need a mechanism like that in any case.

It already handles the case where the buffer isn't big enough (in read_file()). We could use BUFSIZ, but it's massive overkill for reading a single integer: 8192 versus ~21 bytes (or ~42 bytes if intmax_t were 128-bit).

...

It's not a security matter, because if the kernel was compromised, we're compromised too, simply a matter of robustness.

-- Stefano

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

David Gibson

1:34 a.m.

New subject: [PATCH v4 3/4] tcp: Resend SYN for inbound connections

On Fri, Oct 17, 2025 at 12:22:46AM +0200, Stefano Brivio wrote:

...

On Thu, 16 Oct 2025 10:34:22 +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.

Signed-off-by: Yumei Huang --- tcp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-------- tcp.h | 2 ++ 2 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/tcp.c b/tcp.c index 2ec4b0c..3003333 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 + * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, resend + * SYN. It's the starting timeout for the first SYN retry. If this persists + * for more than TCP_MAX_RETRIES or (tcp_syn_retries + + * tcp_syn_linear_timeouts) times in a row, 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 */ #define ACK_TIMEOUT 2 #define FIN_TIMEOUT 60 #define ACT_TIMEOUT 7200 @@ -365,6 +367,10 @@ 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_SYSCTL "/proc/sys/net/ipv4/tcp_syn_retries" +#define TCP_SYN_LINEAR_TIMEOUTS_SYSCTL \ + "/proc/sys/net/ipv4/tcp_syn_linear_timeouts"

It's quite obvious those are names of sysctl entries, I think we can drop the _SYSCTL suffix and keep this shorter without losing any information/indication.

...
+ /* "Extended" data (not stored in the flow table) for TCP flow migration */ static struct tcp_tap_transfer_ext migrate_ext[FLOW_MAX];

@@ -581,8 +587,13 @@ 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)) { + if (conn->retries < c->tcp.syn_linear_timeouts) + it.it_value.tv_sec = SYN_TIMEOUT_INIT; + else + it.it_value.tv_sec = SYN_TIMEOUT_INIT << + (conn->retries - c->tcp.syn_linear_timeouts); + } else it.it_value.tv_sec = ACK_TIMEOUT; } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { @@ -2409,8 +2420,16 @@ 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 >= MIN(TCP_MAX_RETRIES, + (c->tcp.tcp_syn_retries + c->tcp.syn_linear_timeouts))) {

That doesn't seem to match the sysctl documentation for tcp_syn_retries, which should be the *total* number of retries, not excluding the ones with "linear timeouts".

No, but it does match the actual kernel behaviour (Yumei already experimented and discovered this). Confirmed it in the kernel code, here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... -- 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

David Gibson

1:49 a.m.

New subject: [PATCH v4 3/4] tcp: Resend SYN for inbound connections

On Thu, Oct 16, 2025 at 10:34:22AM +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.

Signed-off-by: Yumei Huang

LGTM. Stefano's suggestions make sense, plus one more nit below: [snip]

...

+/** + * tcp_syn_params_init() - Get initial syn params for inbound connection + * @c: Execution context +*/ +void tcp_syn_params_init(struct ctx *c) +{ + long tcp_syn_retries, syn_linear_timeouts;

These should be intmax_t to match read_file_integer(). -- 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

David Gibson

1:59 a.m.

New subject: [PATCH v4 4/4] tcp: Update data retransmission timeout

On Thu, Oct 16, 2025 at 10:34:23AM +0800, Yumei Huang wrote:

...

Use an exponential backoff timeout for data retransmission according to RFC 2988 and RFC 6298. Set the initial RTO to one second as discussed in Appendix A of RFC 6298.

Also combine the macros defining the initial RTO for both SYN and ACK.

Signed-off-by: Yumei Huang

Reviewed-by: David Gibson Except for some minor English clarity notes below.

...

--- tcp.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/tcp.c b/tcp.c index 3003333..3254d67 100644 --- a/tcp.c +++ b/tcp.c @@ -179,16 +179,12 @@ * * Timeouts are implemented by means of timerfd timers, set based on flags: * - * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest during handshake - * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, resend - * SYN. It's the starting timeout for the first SYN retry. If this persists - * for more than TCP_MAX_RETRIES or (tcp_syn_retries + - * tcp_syn_linear_timeouts) times in a row, 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 - * socket and reset sequence to what was acknowledged. If this persists for - * more than TCP_MAX_RETRIES times in a row, reset the connection + * - RTO_INIT: if no ACK segment was received from tap/guest, either during + * handshake (flag ACK_FROM_TAP_DUE without ESTABLISHED event) or after + * sending data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re-send data + * from the socket and reset sequence to what was acknowledged. It's the + * starting timeout for the first retry.

For clarity, I'd suggest: This is the timeout for the first retry, in seconds.

...

If this persists for more than + * allowed times in a row, reset the connection

I'd suggest: If this persists too many times in a row, reset the connection: TCP_MAX_RETRIES for established connections, or (tcp_syn_retries + tcp_syn_linear_timeouts) during the handshake.

...

* - FIN_TIMEOUT: if a FIN segment was sent to tap/guest (flag ACK_FROM_TAP_DUE * with TAP_FIN_SENT event), and no ACK is received within this time, reset @@ -342,8 +338,7 @@ enum { #define WINDOW_DEFAULT 14600 /* RFC 6928 */

#define ACK_INTERVAL 10 /* ms */ -#define SYN_TIMEOUT_INIT 1 /* s */ -#define ACK_TIMEOUT 2 +#define RTO_INIT 1 /* s, RFC 6298 */ #define FIN_TIMEOUT 60 #define ACT_TIMEOUT 7200

@@ -589,13 +584,13 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) } else if (conn->flags & ACK_FROM_TAP_DUE) { if (!(conn->events & ESTABLISHED)) { if (conn->retries < c->tcp.syn_linear_timeouts) - it.it_value.tv_sec = SYN_TIMEOUT_INIT; + it.it_value.tv_sec = RTO_INIT; else - it.it_value.tv_sec = SYN_TIMEOUT_INIT << + it.it_value.tv_sec = RTO_INIT << (conn->retries - c->tcp.syn_linear_timeouts); } else - it.it_value.tv_sec = ACK_TIMEOUT; + it.it_value.tv_sec = RTO_INIT << conn->retries; } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { it.it_value.tv_sec = FIN_TIMEOUT; } else { -- 2.47.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

Yumei Huang

4:11 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Fri, Oct 17, 2025 at 7:16 AM David Gibson wrote:

...

On Fri, Oct 17, 2025 at 12:22:14AM +0200, Stefano Brivio wrote:

...
On Thu, 16 Oct 2025 15:49:39 +0800 Yumei Huang wrote:

...
On Thu, Oct 16, 2025 at 2:30 PM David Gibson wrote:

...
On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote:

...
Signed-off-by: Yumei Huang

[snip]

...
...
+ if (total_read == buf_size) { + warn_perror("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return (int)total_read;

Probably makes more sense for total_read and the return type to be ssize_t.

Just tried to be consistent with write_file(). I can change it to ssize_t if needed.

ssize_t is the type designed for this, if write_file() has it wrong (I didn't check), we should fix that as well.

It does, and we should :).

Checked write_file(), seems the return value is not the same to read_file(). It returns 0 or 1 depending on success or failure. So int works fine here. I will update read_file() only.

...

...
...
...
...
+} + +/** + * read_file_integer() - Read an integer value from a file + * @path: File to read + * @fallback: Default value if file can't be read + * + * Return: Integer value, fallback on failure +*/ +intmax_t read_file_integer(const char *path, intmax_t fallback) +{ + char buf[INTMAX_STRLEN]; + char *end;

passt coding style is to list (where possible) local variables in reverse order of line length, so this should go after bytes_read.

Oh, I didn't notice that. Will update later.

Rationale (added to my further list for CONTRIBUTING.md):

https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

and see also https://lwn.net/Articles/758552/.

If you want to update CONTRIBUTING.md to cover this, Yumei, that would be much appreciated.

Sure, will do it.

...

...
...
...
...
+ intmax_t value; + int bytes_read; + + bytes_read = read_file(path, buf, sizeof(buf)); + + if (bytes_read < 0) + return fallback; + + if (bytes_read == 0) { + debug("Empty file %s", path); + return fallback; + } + + errno = 0; + value = strtoimax(buf, &end, 10); + if (*end && *end != '\n') { + debug("Invalid format in %s", path); + return fallback; + } + if (errno) { + debug("Invalid value in %s: %s", path, buf); + return fallback; + } + + return value; +} + #ifdef __ia64__ /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(), * use the description from clone(2). diff --git a/util.h b/util.h index 22eaac5..887d795 100644 --- a/util.h +++ b/util.h @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +int read_file(const char *path, char *buf, size_t buf_size); +intmax_t read_file_integer(const char *path, intmax_t fallback); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) }

#define UINT16_STRLEN (sizeof("65535")) +#define INTMAX_STRLEN (sizeof("-9223372036854775808"))

It's correct for now, and probably for any systems we're likely to run on, but I dislike hard-assuming the size of intmax_t here. I feel like there must be a better way to derive the correct string length, but I haven't figured out what it is yet :(.

How about this:

#define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2)

Each byte can represent about 2.4 decimal digits as below, sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null terminator.

1 bit = log₁₀(2) ≈ 0.30103 decimal digits 1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

Works for me.

...
If it's sourced from https://stackoverflow.com/a/10536254 and comment, don't forget to mention that in whatever implementation / commit message.

Actually, it's a suggestion from Claude and I double checked the logic and math. Maybe I should mention Claude and the logic in the commit message instead?

...

Good point.

...
But I was thinking... what if we keep it much simpler, use BUFSIZ, and error out if the buffer is too small? It would be good to be robust against any potential kernel issue anyway, so I think we need a mechanism like that in any case.

It already handles the case where the buffer isn't big enough (in read_file()). We could use BUFSIZ, but it's massive overkill for reading a single integer: 8192 versus ~21 bytes (or ~42 bytes if intmax_t were 128-bit).

...
It's not a security matter, because if the kernel was compromised, we're compromised too, simply a matter of robustness.

-- Stefano

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

-- Thanks, Yumei Huang

David Gibson

4:29 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Fri, Oct 17, 2025 at 10:11:21AM +0800, Yumei Huang wrote:

...

On Fri, Oct 17, 2025 at 7:16 AM David Gibson wrote:

...
On Fri, Oct 17, 2025 at 12:22:14AM +0200, Stefano Brivio wrote:

...
On Thu, 16 Oct 2025 15:49:39 +0800 Yumei Huang wrote:

...
On Thu, Oct 16, 2025 at 2:30 PM David Gibson wrote:

...
On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote:

...
Signed-off-by: Yumei Huang

[snip]

...
...
+ if (total_read == buf_size) { + warn_perror("File %s truncated, buffer too small", path); + return -2; + } + + buf[total_read] = '\0'; + + return (int)total_read;

Probably makes more sense for total_read and the return type to be ssize_t.

Just tried to be consistent with write_file(). I can change it to ssize_t if needed.

ssize_t is the type designed for this, if write_file() has it wrong (I didn't check), we should fix that as well.

It does, and we should :).

Checked write_file(), seems the return value is not the same to read_file(). It returns 0 or 1 depending on success or failure. So int works fine here. I will update read_file() only.

Ah, good point. [snip]

...

...
...
Rationale (added to my further list for CONTRIBUTING.md):

https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

and see also https://lwn.net/Articles/758552/.

If you want to update CONTRIBUTING.md to cover this, Yumei, that would be much appreciated.

Sure, will do it.

Thanks. [snip]

...

...
...
...
...
...
diff --git a/util.h b/util.h index 22eaac5..887d795 100644 --- a/util.h +++ b/util.h @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); +int read_file(const char *path, char *buf, size_t buf_size); +intmax_t read_file_integer(const char *path, intmax_t fallback); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); int read_all_buf(int fd, void *buf, size_t len); @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) }

#define UINT16_STRLEN (sizeof("65535")) +#define INTMAX_STRLEN (sizeof("-9223372036854775808"))

It's correct for now, and probably for any systems we're likely to run on, but I dislike hard-assuming the size of intmax_t here. I feel like there must be a better way to derive the correct string length, but I haven't figured out what it is yet :(.

How about this:

#define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2)

Each byte can represent about 2.4 decimal digits as below, sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null terminator.

1 bit = log₁₀(2) ≈ 0.30103 decimal digits 1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

Works for me.

...
If it's sourced from https://stackoverflow.com/a/10536254 and comment, don't forget to mention that in whatever implementation / commit message.

Actually, it's a suggestion from Claude and I double checked the logic and math. Maybe I should mention Claude and the logic in the commit message instead?

Heh, so Claude got it from that stackoverflow page or one like it, probably. It's not correctness we're concerned about, but proper attribution. This is small enough that it's probably not copyrightable, but that would be a concern for larger segments as well. -- 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

Yumei Huang

4:44 a.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Fri, Oct 17, 2025 at 10:30 AM David Gibson wrote:

...

On Fri, Oct 17, 2025 at 10:11:21AM +0800, Yumei Huang wrote:

...
On Fri, Oct 17, 2025 at 7:16 AM David Gibson wrote:

...
On Fri, Oct 17, 2025 at 12:22:14AM +0200, Stefano Brivio wrote:

...
On Thu, 16 Oct 2025 15:49:39 +0800 Yumei Huang wrote:

...
On Thu, Oct 16, 2025 at 2:30 PM David Gibson wrote:

...
On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote: > Signed-off-by: Yumei Huang

[snip]

...
> + if (total_read == buf_size) { > + warn_perror("File %s truncated, buffer too small", path); > + return -2; > + } > + > + buf[total_read] = '\0'; > + > + return (int)total_read;

Probably makes more sense for total_read and the return type to be ssize_t.

Just tried to be consistent with write_file(). I can change it to ssize_t if needed.

ssize_t is the type designed for this, if write_file() has it wrong (I didn't check), we should fix that as well.

It does, and we should :).

Checked write_file(), seems the return value is not the same to read_file(). It returns 0 or 1 depending on success or failure. So int works fine here. I will update read_file() only.

Ah, good point.

[snip]

...
...
...
Rationale (added to my further list for CONTRIBUTING.md):

https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

and see also https://lwn.net/Articles/758552/.

If you want to update CONTRIBUTING.md to cover this, Yumei, that would be much appreciated.

Sure, will do it.

Thanks.

[snip]

...
...
...
...
...
> diff --git a/util.h b/util.h > index 22eaac5..887d795 100644 > --- a/util.h > +++ b/util.h > @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); > int __daemon(int pidfile_fd, int devnull_fd); > int fls(unsigned long x); > int write_file(const char *path, const char *buf); > +int read_file(const char *path, char *buf, size_t buf_size); > +intmax_t read_file_integer(const char *path, intmax_t fallback); > int write_all_buf(int fd, const void *buf, size_t len); > int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); > int read_all_buf(int fd, void *buf, size_t len); > @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) > } > > #define UINT16_STRLEN (sizeof("65535")) > +#define INTMAX_STRLEN (sizeof("-9223372036854775808"))

It's correct for now, and probably for any systems we're likely to run on, but I dislike hard-assuming the size of intmax_t here. I feel like there must be a better way to derive the correct string length, but I haven't figured out what it is yet :(.

How about this:

#define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2)

Each byte can represent about 2.4 decimal digits as below, sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null terminator.

1 bit = log₁₀(2) ≈ 0.30103 decimal digits 1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

Works for me.

...
If it's sourced from https://stackoverflow.com/a/10536254 and comment, don't forget to mention that in whatever implementation / commit message.

Actually, it's a suggestion from Claude and I double checked the logic and math. Maybe I should mention Claude and the logic in the commit message instead?

Heh, so Claude got it from that stackoverflow page or one like it, probably. It's not correctness we're concerned about, but proper attribution. This is small enough that it's probably not copyrightable, but that would be a concern for larger segments as well.

Yeah, I understand. For most of the time, I only asked claude to explain the code instead of writing the code directly. For this piece, I will add a comment explaining the logic before the define line. Thanks.

...

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

-- Thanks, Yumei Huang

Stefano Brivio

19 Oct 19 Oct

12:07 p.m.

New subject: [PATCH v4 2/4] util: Introduce read_file() and read_file_integer() function

On Fri, 17 Oct 2025 10:44:42 +0800 Yumei Huang wrote:

...

On Fri, Oct 17, 2025 at 10:30 AM David Gibson wrote:

...
On Fri, Oct 17, 2025 at 10:11:21AM +0800, Yumei Huang wrote:

...
On Fri, Oct 17, 2025 at 7:16 AM David Gibson wrote:

...
On Fri, Oct 17, 2025 at 12:22:14AM +0200, Stefano Brivio wrote:

...
On Thu, 16 Oct 2025 15:49:39 +0800 Yumei Huang wrote:

...
On Thu, Oct 16, 2025 at 2:30 PM David Gibson wrote: > > On Thu, Oct 16, 2025 at 10:34:21AM +0800, Yumei Huang wrote: > > Signed-off-by: Yumei Huang [snip] > > + if (total_read == buf_size) { > > + warn_perror("File %s truncated, buffer too small", path); > > + return -2; > > + } > > + > > + buf[total_read] = '\0'; > > + > > + return (int)total_read; > > Probably makes more sense for total_read and the return type to be ssize_t.

Just tried to be consistent with write_file(). I can change it to ssize_t if needed.

ssize_t is the type designed for this, if write_file() has it wrong (I didn't check), we should fix that as well.

It does, and we should :).

Checked write_file(), seems the return value is not the same to read_file(). It returns 0 or 1 depending on success or failure. So int works fine here. I will update read_file() only.

Ah, good point.

[snip]

...
...
...
Rationale (added to my further list for CONTRIBUTING.md):

https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

and see also https://lwn.net/Articles/758552/.

If you want to update CONTRIBUTING.md to cover this, Yumei, that would be much appreciated.

Sure, will do it.

Thanks.

[snip]

...
...
...
...
> > diff --git a/util.h b/util.h > > index 22eaac5..887d795 100644 > > --- a/util.h > > +++ b/util.h > > @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid); > > int __daemon(int pidfile_fd, int devnull_fd); > > int fls(unsigned long x); > > int write_file(const char *path, const char *buf); > > +int read_file(const char *path, char *buf, size_t buf_size); > > +intmax_t read_file_integer(const char *path, intmax_t fallback); > > int write_all_buf(int fd, const void *buf, size_t len); > > int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); > > int read_all_buf(int fd, void *buf, size_t len); > > @@ -249,6 +251,7 @@ static inline const char *af_name(sa_family_t af) > > } > > > > #define UINT16_STRLEN (sizeof("65535")) > > +#define INTMAX_STRLEN (sizeof("-9223372036854775808")) > > It's correct for now, and probably for any systems we're likely to run > on, but I dislike hard-assuming the size of intmax_t here. I feel > like there must be a better way to derive the correct string length, > but I haven't figured out what it is yet :(.

How about this:

#define INTMAX_STRLEN (sizeof(intmax_t) * 3 + 2)

Each byte can represent about 2.4 decimal digits as below, sizeof(intmax_t) * 3 gives us a safe upper bound, +2 for sign and null terminator.

1 bit = log₁₀(2) ≈ 0.30103 decimal digits 1 byte = 8 bits = 8 × 0.30103 ≈ 2.408 decimal digits

Works for me.

...
If it's sourced from https://stackoverflow.com/a/10536254 and comment, don't forget to mention that in whatever implementation / commit message.

Actually, it's a suggestion from Claude and I double checked the logic and math.

Please mention that the next time instead of making it look like it's yours.

...

...
...
Maybe I should mention Claude and the logic in the commit message instead?

Using whatever tool to copy code or descriptions of techniques doesn't make you exempt from licensing requirements, see: https://stackoverflow.com/help/licensing

...

...
Heh, so Claude got it from that stackoverflow page or one like it, probably. It's not correctness we're concerned about, but proper attribution. This is small enough that it's probably not copyrightable, but that would be a concern for larger segments as well.

Yeah, I understand. For most of the time, I only asked claude to explain the code instead of writing the code directly. For this piece, I will add a comment explaining the logic before the define line.

That's not enough. Just to be very clear, I'm not going to merge anything that violates licensing requirements, no matter if some tool hides them from you. -- Stefano

Age (days ago)

Last active (days ago)

List overview

Download

16 comments

3 participants

participants (3)

David Gibson
Stefano Brivio
Yumei Huang

[PATCH v4 0/4] Retry SYNs for inbound connections

tags

participants (3)