Re: [PATCH net v6 1/1] tcp: bound SYN-ACK timers to reqsk timeout range

On Sat, Aug 22, 2026 at 4:20 PM Zhiling Zou <[email protected]> wrote:
>
> From: Zhiling Zou <[email protected]>
>
> request_sock::num_timeout is a 7-bit counter. Commit e6c022a4fa2d
> ("tcp: better retrans tracking for defer-accept") split this counter
> out of an 8-bit field, but tcp_synack_retries still accepts an 8-bit
> value and TCP_DEFER_ACCEPT can still derive a retry count up to 255.
>
> If these settings exceed 127, the regular request timer cannot reach
> its expiration threshold and num_timeout wraps to zero. After the wrap,
> the request can keep timing out instead of expiring, and the next
> zero-to-one transition repeats the young-queue accounting decrement.
>
> Both request timer paths can also shift req->timeout by 64 or more while
> calculating the next RTO. UBSAN reports that invalid shift, and systems
> with panic_on_warn=1 panic before the later cap can take effect.
>
> Keep the tcp_synack_retries sysctl range unchanged, but cap its effective
> value in both SYN-ACK timer paths. Cap the TCP_DEFER_ACCEPT conversion at
> the same range so its timer and bare-ACK consumers agree. Saturate the RTO
> calculation before shifting, and cap the Fast Open extra retry as well.
>
> Snapshot the request timeout and num_timeout once before the bounds check
> and shift. num_timeout is a bitfield, so add a raw view of its existing
> storage byte for a compile-safe READ_ONCE() snapshot. This keeps the check
> and shift consistent when the timer concurrently increments num_timeout.
>
> Fixes: e6c022a4fa2d ("tcp: better retrans tracking for defer-accept")
> Cc: [email protected]
> Reported-by: Vega <[email protected]>
> Signed-off-by: Zhiling Zou <[email protected]>
> ---
> changes in v6:
> - Keep the tcp_synack_retries sysctl range unchanged and cap the effective
>   retry count only in the regular and Fast Open timer paths.
> - Snapshot num_timeout and timeout once before the bounds check and shift.
> - Use a raw view of num_timeout's bitfield storage for the compile-safe
>   READ_ONCE() snapshot.
> - v5 Link: https://lore.kernel.org/all/9ec8921d81d6218946e07e8542b1ac41b6e2d205.1786540242.git.zhilinz@nebusec.ai/
>
> changes in v5:
> - Limit tcp_synack_retries and TCP_DEFER_ACCEPT at their configuration
>   paths, so all users of each value see the same 7-bit range.
> - Use 127, matching request_sock::num_timeout, instead of the previous
>   runtime limit of 63, and document the sysctl limit.
> - Spell out the 7-bit range mismatch, the repeated young-queue accounting,
>   and the UBSAN panic_on_warn failure mode in the commit message.
> - Keep the timeout calculation saturating before either SYN-ACK timer
>   shifts it.
> - Drop a no-op reqsk_timer_handler() formatting hunk.
> - Correct the Fixes tag and update the reporter and sign-off trailers.
> - v4 Link: https://lore.kernel.org/all/891a220c362e3266efdf6b1aa9dc3e52f6825c00.1784735392.git.zhilinz@nebusec.ai/
>
> Changes in v4:
>   - Drop the tcp_synack_retries sysctl maximum to preserve existing
>     user-space behavior, and keep the runtime clamps at the timer usage
>     sites.
>   - v3 Link: https://lore.kernel.org/all/[email protected]/
>
> Changes in v3:
>   - Order local variables in tcp_reqsk_timeout_sk() by reverse Christmas
>     tree.
>   - v2 Link: https://lore.kernel.org/all/[email protected]/
>
> Changes in v2:
>   - Keep the existing max_retries calculation in
>     tcp_fastopen_synack_timer() and only add the clamp, avoiding code
>     churn.
>   - v1 Link: https://lore.kernel.org/all/02e24eb83639e9d7ecc623f000c60254bb5c40a5.1782643946.git.roxy520tt@gmail.com/
>
>  Documentation/networking/ip-sysctl.rst |  2 +-
>  include/net/request_sock.h             | 20 ++++++++++++++++++--
>  include/net/tcp.h                      | 21 +++++++++++++++++----
>  net/ipv4/inet_connection_sock.c        |  2 ++
>  net/ipv4/tcp.c                         |  2 +-
>  net/ipv4/tcp_timer.c                   |  6 ++++--
>  6 files changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 208f46967ee59..fd5038b6cab9f 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -954,7 +954,7 @@ tcp_stdurg - BOOLEAN
>

Hi Sashiko,

Thanks for the review.

>  tcp_synack_retries - INTEGER
>         Number of times SYNACKs for a passive TCP connection attempt will
> -       be retransmitted. Should not be higher than 255. Default value
> +       be retransmitted. Should not be higher than 127. Default value

The observation about the sysctl readback is correct. Values above 127
remain accepted and stored intentionally to preserve the existing
user-space behavior, following Paolo's feedback. Adding an .extra2 limit
would make previously accepted writes fail, so I will not add it.

I agree that the current documentation is ambiguous. I will update it to
state explicitly that values above 127 are accepted and reported back,
but are treated as 127 by the SYN-ACK timer paths.

>         is 5, which corresponds to 31seconds till the last retransmission
>         with the current initial RTO of 1second. With this the final timeout
>         for a passive TCP connection will happen after 63seconds.
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 5a9c826a7092d..a9781fab774aa 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -58,12 +58,17 @@ struct request_sock {
>         struct request_sock             *dl_next;
>         u16                             mss;
>         u8                              num_retrans; /* number of retransmits */
> -       u8                              syncookie:1; /* True if
> +       union {
> +               struct {
> +                       u8              syncookie:1; /* True if
>                                                       * 1) tcpopts needs to be encoded in
>                                                       *    TS of SYN+ACK
>                                                       * 2) ACK is validated by BPF kfunc.
>                                                       */
> -       u8                              num_timeout:7; /* number of timeouts */
> +                       u8              num_timeout:7; /* number of timeouts */
> +               };
> +               u8              num_timeout_syncookie;
> +       };
>         u32                             ts_recent;
>         struct timer_list               rsk_timer;
>         const struct request_sock_ops   *rsk_ops;
> @@ -74,6 +79,17 @@ struct request_sock {
>         u32                             timeout;
>  };
>
> +static inline u8 reqsk_num_timeout(const struct request_sock *req)
> +{
> +       u8 num_timeout = READ_ONCE(req->num_timeout_syncookie);
> +
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +       return num_timeout >> 1;
> +#else
> +       return num_timeout & 0x7f;
> +#endif
> +}
> +
>  static inline struct request_sock *inet_reqsk(const struct sock *sk)
>  {
>         return (struct request_sock *)sk;
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 2c5b889530b55..b4054aa53b7c7 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -183,6 +183,8 @@ static_assert((1 << ATO_BITS) > TCP_DELACK_MAX);
>  #define MAX_TCP_KEEPINTVL      32767
>  #define MAX_TCP_KEEPCNT                127
>  #define MAX_TCP_SYNCNT         127
> +/* request_sock::num_timeout is a 7-bit field. */
> +#define MAX_TCP_SYNACK_RETRIES 127
>
>  /* Ensure that TCP PAWS checks are relaxed after ~2147 seconds
>   * to avoid overflows. This assumes a clock smaller than 1 Mhz.
> @@ -882,12 +884,23 @@ static inline u32 __tcp_set_rto(const struct tcp_sock *tp)
>         return usecs_to_jiffies((tp->srtt_us >> 3) + tp->rttvar_us);
>  }
>
> -static inline unsigned long tcp_reqsk_timeout(struct request_sock *req)
> +static inline unsigned long tcp_reqsk_timeout_sk(const struct sock *sk,
> +                                                struct request_sock *req)
>  {
> -       u64 timeout = (u64)req->timeout << req->num_timeout;
> +       u64 timeout = READ_ONCE(req->timeout);
> +       u32 rto_max = tcp_rto_max(sk);
> +       u8 num_timeout = reqsk_num_timeout(req);
> +
> +       if (num_timeout >= BITS_PER_TYPE(timeout) ||
> +           timeout > U64_MAX >> num_timeout)
> +               return rto_max;
> +
> +       return (unsigned long)min_t(u64, timeout << num_timeout, rto_max);
> +}
>
> -       return (unsigned long)min_t(u64, timeout,
> -                                   tcp_rto_max(req->rsk_listener));
> +static inline unsigned long tcp_reqsk_timeout(struct request_sock *req)
> +{
> +       return tcp_reqsk_timeout_sk(req->rsk_listener, req);
>  }
>
>  u32 tcp_delack_max(const struct sock *sk);
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 56902bba54838..a5f6ad0f0ea1f 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -1085,6 +1085,8 @@ static void reqsk_timer_handler(struct timer_list *t)
>                         young <<= 1;
>                 }
>         }
> +       max_syn_ack_retries = min_t(int, max_syn_ack_retries,
> +                                   MAX_TCP_SYNACK_RETRIES);
>

The finding about the clamp placement is also correct. Applying the clamp
after the queue-pressure reduction can hide that reduction when the
tcp_synack_retries sysctl is above 127. I will move the clamp to
immediately after selecting the per-socket or sysctl value, before the
reduction loop.

TCP_SYNCNT cannot set icsk_syn_retries above 127 because
tcp_sock_set_syncnt() already enforces MAX_TCP_SYNCNT, but the sysctl case
is sufficient to expose the ordering issue.

I will address both points in the next revision.

Thanks,
Zhiling

>         syn_ack_recalc(req, max_syn_ack_retries, READ_ONCE(queue->rskq_defer_accept),
>                        &expire, &resend);
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 455441f1b6949..24896fa08a5d8 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -357,7 +357,7 @@ static u8 secs_to_retrans(int seconds, int timeout, int rto_max)
>                 int period = timeout;
>
>                 res = 1;
> -               while (seconds > period && res < 255) {
> +               while (seconds > period && res < MAX_TCP_SYNACK_RETRIES) {
>                         res++;
>                         timeout <<= 1;
>                         if (timeout > rto_max)
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index bf171b5e1eb30..daa4eac072dd9 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -462,11 +462,13 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
>
>         tcp_syn_ack_timeout(req);
>
> -       /* Add one more retry for fastopen.
> +       /* Add one more retry for fastopen when the timeout counter can
> +        * represent it.
>          * Paired with WRITE_ONCE() in tcp_sock_set_syncnt()
>          */
>         max_retries = READ_ONCE(icsk->icsk_syn_retries) ? :
>                 READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_synack_retries) + 1;
> +       max_retries = min_t(int, max_retries, MAX_TCP_SYNACK_RETRIES);
>
>         if (req->num_timeout >= max_retries) {
>                 tcp_write_err(sk);
> @@ -488,7 +490,7 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
>         if (!tp->retrans_stamp)
>                 tp->retrans_stamp = tcp_time_stamp_ts(tp);
>         tcp_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
> -                         req->timeout << req->num_timeout, false);
> +                            tcp_reqsk_timeout_sk(sk, req), false);
>  }
>
>  static bool tcp_rtx_probe0_timed_out(const struct sock *sk,
> --
> 2.43.0