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

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---

--- Patch [1]: [PATCH net v4 resend 1/1] tcp: bound SYN-ACK timers to reqsk timeout range ---

commit 319e164a9bccb22bb455177e690c982f75111c61
Author: Zhiling Zou [off-list ref]

tcp: bound SYN-ACK timers to reqsk timeout range

This adds a new MAX_TCP_SYNACK_RETRIES constant of 63 and applies it as a
runtime clamp on the effective SYN-ACK retry limit and on
rskq_defer_accept in reqsk_timer_handler(), and on max_retries in
tcp_fastopen_synack_timer().  It also converts tcp_reqsk_timeout() into a
saturating helper tcp_reqsk_timeout_sk() that returns rto_max instead of
performing an out-of-range shift.

A few questions below.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org

[Severity: Low]
Is this Fixes tag pointing at the right commit?  git blame on the
baseline attributes tcp_reqsk_timeout() and its shift expression:

  include/net/tcp.h:
	u64 timeout = (u64)req->timeout << req->num_timeout;

	return (unsigned long)min_t(u64, timeout,
				    tcp_rto_max(req->rsk_listener));

to 207ce0f6bc131 (Kuniyuki Iwashima, 2025-11-06), and the rto_max clamp
to 1e9d3005e02cb (2025-11-06).  The new helper also depends on
tcp_rto_max()/icsk_rto_max, which are recent additions too.

With Cc: stable on a patch that changes observable behaviour, could the
tag be adjusted so the affected tree range is clear?

Also, could the changelog state the concrete user-visible impact of the
pre-existing out-of-range shift?  In the reqsk path the result was already
bounded by min_t(u64, ..., tcp_rto_max()), and in the Fast Open path
inet_csk_reset_xmit_timer() clamps 'when' to max_when, so the observable
effect looks like a mis-scaled timer rather than memory unsafety, and it
requires an administrator-configured limit above 63.

[Severity: Low]
Where does 63 come from?  The neighbouring MAX_TCP_* constants are
write-time validation limits for the corresponding interfaces, but this
one is only a hidden runtime clamp, and 63 matches neither the width of
the num_timeout field (u8 num_timeout:7, so 0..127) nor MAX_TCP_SYNCNT
(127), which bounds the very same icsk_syn_retries field.  Could a comment
explain the derivation?

[Severity: Low]
Does the changelog rationale still hold once this helper exists?  The
changelog says:

  "Excessive retry or defer-accept limits can therefore drive these timer
   paths into invalid shift counts before the request expires."

  "Clamp the effective retry and defer-accept limits in the regular
   request socket timer path, clamp the Fast Open retry limit, ..."

But req->num_timeout is a 7-bit field, so it can only hold 0..127, and
this helper already saturates for every one of those values.  Both shift
sites now route through it: tcp_reqsk_timeout() -> tcp_reqsk_timeout_sk()
from reqsk_timer_handler()'s mod_timer(), and tcp_reset_xmit_timer() in
tcp_fastopen_synack_timer().  With that in place, are the three call-site
clamps still needed for shift safety at all?

Conversely, the effect the clamps do have isn't described anywhere.  With
a configured limit above 127 (for example sysctl_tcp_synack_retries=200,
which is writable today), syn_ack_recalc()'s

	*expire = req->num_timeout >= max_syn_ack_retries;

can never become true, req->num_timeout++ wraps 127 -> 0, so the request
socket never expires and keeps retransmitting SYN-ACKs, and each wrap
re-runs:

	if (req->num_timeout++ == 0)
		atomic_dec(&queue->young);

underflowing the young counter used by the pruning heuristic.  Could the
changelog describe that effect instead, and say why 63 was chosen over
helper-only saturation, over 127 (the field width, matching
MAX_TCP_SYNCNT, so no currently legal configuration changes behaviour), or
over write-time validation plus a Documentation update?

[Severity: Medium]
Does this silently truncate limits that are currently reachable and
honoured?

The write side is unchanged by this patch.  In
net/ipv4/sysctl_net_ipv4.c, tcp_synack_retries is registered with
proc_dou8vec_minmax and no .extra1/.extra2, so 0..255 stays writable, and
Documentation/networking/ip-sysctl.rst still says the value "should not be
higher than 255".  tcp_sock_set_syncnt() accepts icsk_syn_retries up to
MAX_TCP_SYNCNT (127), and do_tcp_getsockopt(TCP_SYNCNT) returns the stored
value verbatim.

Since req->num_timeout can reach 127, limits in 64..127 were previously
fully reachable, so a listener configured with tcp_synack_retries=100 (or
TCP_SYNCNT=100) now expires request sockets at 63 timeouts while
userspace still reads back 100.  Is that divergence between reported
configuration and effective behaviour intended?

The changelog says:

  "This keeps sysctl writes unchanged while bounding the timer
   calculations."

The write does still succeed, but its effect changes.  Could the changelog
and ip-sysctl.rst be updated to describe the new effective ceiling?

[ ... ]

[Severity: Medium]
Can clamping rskq_defer_accept only here make TCP_DEFER_ACCEPT unable to
ever complete?  The other consumer of the same field, tcp_check_req() in
net/ipv4/tcp_minisocks.c, still compares against the unclamped value:

	if (req->num_timeout < READ_ONCE(inet_csk(sk)->icsk_accept_queue.rskq_defer_accept) &&
	    TCP_SKB_CB(skb)->end_seq == tcp_rsk(req)->rcv_isn + 1) {
		inet_rsk(req)->acked = 1;
		...
		return NULL;
	}

secs_to_retrans() can legitimately store 64..255 in rskq_defer_accept
(its loop is "while (seconds > period && res < 255)"), so with, say,
rskq_defer_accept == 100 the two deferral windows disagree.
syn_ack_recalc() sees 63 and sets:

	*expire = req->num_timeout >= max_syn_ack_retries &&
		  (!inet_rsk(req)->acked || req->num_timeout >= rskq_defer_accept);

so the request expires at num_timeout == 63, and the documented "last
chance for data or ACK to create established socket" resend moves to
num_timeout == 62.  Meanwhile any bare ACK arriving in that window is
still swallowed by tcp_check_req() (62 and 63 are both < 100), so the
request socket gets dropped by __inet_csk_reqsk_queue_drop() instead of
being promoted.  Before this patch both comparisons used the same value.

getsockopt(TCP_DEFER_ACCEPT) also still converts the unclamped value back
to seconds in do_tcp_getsockopt():

	val = READ_ONCE(icsk->icsk_accept_queue.rskq_defer_accept);
	val = retrans_to_secs(val, TCP_TIMEOUT_INIT / HZ,
			      TCP_RTO_MAX / HZ);

Should both sites be clamped consistently, or the clamp dropped here?

[Severity: Low]
Does the clamp cancel out the preceding comment?  The comment just above
reads "Add one more retry for fastopen", but at
sysctl_tcp_synack_retries == 63 the computed 64 is clamped straight back
to 63, so the extra Fast Open retry disappears, and for sysctl values
above 63 the "+ 1" has no effect at all.

	if (req->num_timeout >= max_retries) {
		tcp_write_err(sk);
		return;
	}

means the TFO child then hits tcp_write_err() one retry earlier than the
comment describes.  Would MAX_TCP_SYNACK_RETRIES + 1 be the intended
bound here, or should the comment be updated?

 
 	if (req->num_timeout >= max_retries) {
 		tcp_write_err(sk);

[ ... ]