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

On Thu, Aug 20, 2026 at 6:21 PM Paolo Abeni <[email protected]> wrote:
>
> On 8/12/26 3:38 PM, Zhiling Zou wrote:
> > 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.
> >
> > Limit tcp_synack_retries and the TCP_DEFER_ACCEPT conversion to the
> > range represented by num_timeout, preserving the same defer-accept value
> > for its timer and bare-ACK consumers. Saturate RTO calculation before
> > the shift, and cap the Fast Open extra retry to the same range.
> >
> > 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 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/tcp.h                      | 20 ++++++++++++++++----
> >  net/ipv4/sysctl_net_ipv4.c             |  2 ++
> >  net/ipv4/tcp.c                         |  2 +-
> >  net/ipv4/tcp_timer.c                   |  6 ++++--
> >  5 files changed, 24 insertions(+), 8 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
> >
> >  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
> >       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/tcp.h b/include/net/tcp.h
> > index 2c5b889530b55..8ab2fd368ed3b 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,22 @@ 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;
> > +     u32 rto_max = tcp_rto_max(sk);
> > +     u64 timeout = req->timeout;
> > +
> > +     if (req->num_timeout >= BITS_PER_TYPE(timeout) ||
> > +         timeout > U64_MAX >> req->num_timeout)
> > +             return rto_max;
>
> Sashiko notes the above is racy, and basically makes this patch not
> effective:
>
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/9ec8921d81d6218946e07e8542b1ac41b6e2d205.1786540242.git.zhilinz%40nebusec.ai
>
> note that the solution provided there does not even compile, you
> will need something slightly different.
>
> > @@ -1034,6 +1035,7 @@ static struct ctl_table ipv4_net_table[] = {
> >               .maxlen         = sizeof(u8),
> >               .mode           = 0644,
> >               .proc_handler   = proc_dou8vec_minmax,
> > +             .extra2         = &tcp_synack_retries_max,
>
> I explicitly asked to avoid this in v3. Sashiko suggested updating the
> doc. Why do you add this back?
>

Hi Paolo,

Thanks for the review and for pointing me to the Sashiko report.

I agree that the check/use race in tcp_reqsk_timeout_sk() is valid.
req->num_timeout is a 7-bit bitfield and may be updated by
reqsk_timer_handler() while the receive path is reading it. I will
take a single snapshot of the field and use that value for both the
bounds check and the shift. I will not use READ_ONCE() directly on the
bitfield, since that does not compile; I will use a compile-safe
equivalent and verify it with a build.

I also agree that adding .extra2 changes the accepted sysctl range and
goes against your earlier v3 feedback. I will remove
tcp_synack_retries_max and the .extra2 assignment. The 127 limit will
instead be enforced at the regular and Fast Open SYN-ACK timer use
sites, preserving the existing sysctl behavior.

I will keep the TCP_DEFER_ACCEPT conversion capped at 127, since
rskq_defer_accept is consumed by both the timer and bare-ACK paths and
must have the same representable range.

I will send a v6 with these changes and update the commit message accordingly.

Thanks,
Zhiling

> /P
>