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

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 3 Nov 2025 20:01:46 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <yuhuang@redhat.com>  
> > >
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > >
> > > 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