Re: [PATCH net 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message gets the TCP_DEFER_ACCEPT path wrong and
  does not mention a user-visible change.

--

tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers

The patch caps the shift amount in the request_sock SYN-ACK timeout helper
at 30. It also limits max_retries / max_syn_ack_retries to 127, the width
of the num_timeout field, and sends the TFO SYN-ACK timer through the same
bounded helper. A further hunk lowers the TCP_DEFER_ACCEPT conversion cap
in secs_to_retrans() from 255 to 127.

[ ... ]

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 99d7237a623d8..a309fd45ad425 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -358,7 +358,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) {

[Severity: Medium]
This hunk lowers the TCP_DEFER_ACCEPT cap from 255 to 127. It seems to be
the actual fix for the defer-accept trigger, but none of the three bullets
in the commit message describe it. Could the change list mention it?

The commit message also says:

  TCP_DEFER_ACCEPT can derive a comparable count. Both feed the
  max_retries / max_syn_ack_retries that decide how long a request_sock
  keeps retransmitting SYN-ACKs.

Is that accurate? do_tcp_setsockopt() stores the converted value in
icsk_accept_queue.rskq_defer_accept, and syn_ack_recalc() checks it as a
separate condition:

net/ipv4/inet_connection_sock.c:syn_ack_recalc() {
    ...
	*expire = req->num_timeout >= max_syn_ack_retries &&
		  (!inet_rsk(req)->acked || req->num_timeout >= rskq_defer_accept);
    ...
}

tcp_check_req() also compares req->num_timeout against rskq_defer_accept
directly. The value never passes through max_retries or
max_syn_ack_retries, so the new min_t() clamps in reqsk_timer_handler() and
tcp_fastopen_synack_timer() don't apply to it.

Before this patch, take rskq_defer_accept above 127 and a peer that sends a
bare ACK (acked=1) but no data. The 7-bit num_timeout can never reach
rskq_defer_accept, so expire stays false. reqsk_timer_handler() then keeps
re-arming through tcp_reqsk_timeout(), and the shift goes out of bounds once
num_timeout reaches 64.

That path only needs setsockopt(TCP_DEFER_ACCEPT) on the caller's own
listener, with the default tcp_synack_retries=5. The commit message gives
the trigger only as "Set tcp_synack_retries high enough", which needs root.
Would it help stable backporters to say that this is reachable without
privileges?

There is also a user-visible change here. setsockopt(TCP_DEFER_ACCEPT) now
stores at most 127 retransmits, and larger requested values are truncated
without notice. As a result, the largest value getsockopt(TCP_DEFER_ACCEPT)
can return drops from retrans_to_secs(255, 1, 120) = 29887 seconds to
retrans_to_secs(127, 1, 120) = 14527 seconds.

The ip-sysctl.rst update only covers tcp_synack_retries. Should the commit
message or the documentation mention the new TCP_DEFER_ACCEPT limit?

>  			res++;
>  			timeout <<= 1;
>  			if (timeout > rto_max)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023064915.401112-1-jiayuan.chen%40linux.dev