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

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?

/P