mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers
@ 2026-10-26 12:32 Jiayuan Chen
  2026-10-26 12:32 ` [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff() Jiayuan Chen
  2026-10-26 13:27 ` [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Neal Cardwell
  0 siblings, 2 replies; 4+ messages in thread
From: Jiayuan Chen @ 2026-10-26 12:32 UTC (permalink / raw)
  To: netdev
  Cc: Jiayuan Chen, stable, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
	Shuah Khan, Randy Dunlap, Neal Cardwell, Kuniyuki Iwashima,
	linux-doc, linux-kernel

req->num_timeout counts the SYN-ACK timeouts of a request and is used as
a shift amount:

	/* tcp_reqsk_timeout() */
	u64 timeout = (u64)req->timeout << req->num_timeout;
	/* tcp_fastopen_synack_timer() */
	req->timeout << req->num_timeout

Once it climbs past the width of the shifted type the shift is UB. Seen
with tcp_synack_retries=255 and a peer that never completes the
handshake:

  UBSAN: shift-out-of-bounds in ./include/net/tcp.h:917:34
  shift exponent 64 is too large for 64-bit type 'long long unsigned int'
  Call Trace:
   <TASK>
   dump_stack_lvl+0xc1/0xf0
   dump_stack+0x10/0x20
   __ubsan_handle_shift_out_of_bounds+0x1d1/0x390
   reqsk_timer_handler.cold+0x1c/0x5d
   call_timer_fn+0x17d/0x510
   __run_timers+0x662/0xab0
   run_timer_softirq+0xdf/0x260
   handle_softirqs+0x204/0x9b0
   __irq_exit_rcu+0x13c/0x220
   irq_exit_rcu+0xe/0x20
   sysvec_apic_timer_interrupt+0x40/0xd0
   asm_sysvec_apic_timer_interrupt+0x1b/0x20

num_timeout is a 7-bit field. Once a retransmit limit is above 127 the
"num_timeout >= limit" check can never be true, the request is never
expired by that limit, and after the field wraps to 0
reqsk_timer_handler() decrements queue->young again on the next expiry.

 - split tcp_reqsk_timeout() into tcp_reqsk_timeout_sk() and bound the
   shift there; a num_timeout well under 30 already saturates the
   tcp_rto_max() clamp, so nothing reachable changes for the u64 path.
 - cap max_retries / max_syn_ack_retries at 127, the largest value the
   field can hold, instead of narrowing the sysctl; 128..255 are still
   accepted and act as 127.
 - cap the TCP_DEFER_ACCEPT conversion in secs_to_retrans() at 127 too;
   larger values kept an acked request from ever expiring. getsockopt()
   now reports at most 14527 instead of 29887 seconds.
 - route the TFO path through the same helper; its u32 shift could also
   wrap to 0 before reaching UB.
 - document that tcp_synack_retries values above 127 act as 127.

Fixes: e6c022a4fa2d ("tcp: better retrans tracking for defer-accept")
Cc: stable@vger.kernel.org
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
v1 -> v2: reworded the commit message, no code change.
v1: https://lore.kernel.org/netdev/20261023064915.401112-1-jiayuan.chen@linux.dev/
---
 Documentation/networking/ip-sysctl.rst |  4 +++-
 include/net/tcp.h                      | 16 ++++++++++++----
 net/ipv4/inet_connection_sock.c        |  2 ++
 net/ipv4/tcp.c                         |  2 +-
 net/ipv4/tcp_timer.c                   |  3 ++-
 5 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 4b19c8d3efda..eac33072be2d 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -956,7 +956,9 @@ 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. The sysctl accepts 0-255, but values above 127 are
+	silently capped by the SYN-ACK timers to match the 7-bit
+	request_sock::num_timeout field. 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 0265c27a5fd7..986b7ea05e4a 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.
@@ -912,12 +914,18 @@ 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,
+						 const struct request_sock *req)
 {
-	u64 timeout = (u64)req->timeout << req->num_timeout;
+	u64 timeout = (u64)READ_ONCE(req->timeout) <<
+		      min_t(u8, req->num_timeout, 30);
+
+	return (unsigned long)min_t(u64, timeout, tcp_rto_max(sk));
+}
 
-	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 8151c638a24c..ee7b64f3141c 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -1072,6 +1072,8 @@ static void reqsk_timer_handler(struct timer_list *t)
 	net = sock_net(sk_listener);
 	max_syn_ack_retries = READ_ONCE(icsk->icsk_syn_retries) ? :
 		READ_ONCE(net->ipv4.sysctl_tcp_synack_retries);
+	max_syn_ack_retries = min_t(int, max_syn_ack_retries,
+				    MAX_TCP_SYNACK_RETRIES);
 	/* Normally all the openreqs are young and become mature
 	 * (i.e. converted to established socket) for first timeout.
 	 * If synack was not acknowledged for 1 second, it means
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 99d7237a623d..a309fd45ad42 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) {
 			res++;
 			timeout <<= 1;
 			if (timeout > rto_max)
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index e56eae4bc341..f7b100334a89 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -469,6 +469,7 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
 	 */
 	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);
@@ -490,7 +491,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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff()
  2026-10-26 12:32 [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Jiayuan Chen
@ 2026-10-26 12:32 ` Jiayuan Chen
  2026-10-26 13:36   ` Neal Cardwell
  2026-10-26 13:27 ` [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Neal Cardwell
  1 sibling, 1 reply; 4+ messages in thread
From: Jiayuan Chen @ 2026-10-26 12:32 UTC (permalink / raw)
  To: netdev
  Cc: Jiayuan Chen, stable, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
	Shuah Khan, Randy Dunlap, Neal Cardwell, Kuniyuki Iwashima,
	linux-doc, linux-kernel

icsk_backoff is incremented by tcp_retransmit_timer() on every backed-off
RTO with no cap of its own, and by tcp_send_probe0() up to tcp_retries2.
It is used as a shift amount in inet_csk_rto_backoff():

	u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff;

Once it climbs past 63 the shift is UB. Seen with tcp_retries2=255 and a
silent peer, when an ICMP host unreachable goes through
tcp_ld_RTO_revert():

  UBSAN: shift-out-of-bounds in ./include/net/inet_connection_sock.h:258:40
  shift exponent 73 is too large for 64-bit type 'long long unsigned int'
  Call Trace:
   <IRQ>
   dump_stack_lvl+0xc1/0xf0
   dump_stack+0x10/0x20
   __ubsan_handle_shift_out_of_bounds+0x1d1/0x390
   tcp_ld_RTO_revert.cold+0x16/0x92
   tcp_v4_err+0xdb3/0x13e0
   icmp_socket_deliver+0x1a7/0x390
   icmp_unreach+0x2dd/0xe50
   icmp_rcv+0x93e/0xe30
   ip_protocol_deliver_rcu+0x36b/0x3e0
   ip_local_deliver_finish+0x37d/0x620
   ip_local_deliver+0x165/0x1d0
   ip_rcv+0x284/0x320
   __netif_receive_skb_one_core+0x16b/0x1c0
   __netif_receive_skb+0x21/0x150
   process_backlog+0x37e/0x1200
   __napi_poll.constprop.0+0xa5/0x5e0
   net_rx_action+0x4c0/0xf70
   handle_softirqs+0x204/0x9b0
   __irq_exit_rcu+0x13c/0x220
   irq_exit_rcu+0xe/0x20
   sysvec_apic_timer_interrupt+0x92/0xd0
   </IRQ>

The orphan check in tcp_probe_timer() calls the same helper. On x86-64
the oversized shift count is taken modulo 64, so an orphan that should
be dead can be seen as alive, and tcp_ld_RTO_revert() can end up with a
smaller RTO than intended.

Cap the shift at 32: any non-zero icsk_rto << 32 exceeds TCP_RTO_MAX, so
the min_t() result is what the unbounded math would give. The
ilog2(TCP_RTO_MAX / TCP_RTO_MIN) + 1 bound of tcp_probe0_when() does not
fit here, as icsk_rto has no TCP_RTO_MIN floor the way tcp_probe0_base()
has.

Fixes: fcdd1cf4dd63 ("tcp: avoid possible arithmetic overflows")
Cc: stable@vger.kernel.org
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 include/net/inet_connection_sock.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 903499581db7..562e12033a5d 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -255,9 +255,10 @@ static inline unsigned long
 inet_csk_rto_backoff(const struct inet_connection_sock *icsk,
 		     unsigned long max_when)
 {
-        u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff;
+	u8 backoff = min_t(u8, 32U, icsk->icsk_backoff);
+	u64 when = (u64)icsk->icsk_rto << backoff;
 
-        return (unsigned long)min_t(u64, when, max_when);
+	return (unsigned long)min_t(u64, when, max_when);
 }
 
 struct sock *inet_csk_accept(struct sock *sk, struct proto_accept_arg *arg);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers
  2026-10-26 12:32 [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Jiayuan Chen
  2026-10-26 12:32 ` [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff() Jiayuan Chen
@ 2026-10-26 13:27 ` Neal Cardwell
  1 sibling, 0 replies; 4+ messages in thread
From: Neal Cardwell @ 2026-10-26 13:27 UTC (permalink / raw)
  To: Jiayuan Chen
  Cc: netdev, stable, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Kuniyuki Iwashima, linux-doc, linux-kernel

On Mon, Oct 26, 2026 at 8:32 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
> req->num_timeout counts the SYN-ACK timeouts of a request and is used as
> a shift amount:
>
>         /* tcp_reqsk_timeout() */
>         u64 timeout = (u64)req->timeout << req->num_timeout;
>         /* tcp_fastopen_synack_timer() */
>         req->timeout << req->num_timeout
>
> Once it climbs past the width of the shifted type the shift is UB. Seen
> with tcp_synack_retries=255 and a peer that never completes the
> handshake:
>
>   UBSAN: shift-out-of-bounds in ./include/net/tcp.h:917:34
>   shift exponent 64 is too large for 64-bit type 'long long unsigned int'
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0xc1/0xf0
>    dump_stack+0x10/0x20
>    __ubsan_handle_shift_out_of_bounds+0x1d1/0x390
>    reqsk_timer_handler.cold+0x1c/0x5d
>    call_timer_fn+0x17d/0x510
>    __run_timers+0x662/0xab0
>    run_timer_softirq+0xdf/0x260
>    handle_softirqs+0x204/0x9b0
>    __irq_exit_rcu+0x13c/0x220
>    irq_exit_rcu+0xe/0x20
>    sysvec_apic_timer_interrupt+0x40/0xd0
>    asm_sysvec_apic_timer_interrupt+0x1b/0x20
>
> num_timeout is a 7-bit field. Once a retransmit limit is above 127 the
> "num_timeout >= limit" check can never be true, the request is never
> expired by that limit, and after the field wraps to 0
> reqsk_timer_handler() decrements queue->young again on the next expiry.
>
>  - split tcp_reqsk_timeout() into tcp_reqsk_timeout_sk() and bound the
>    shift there; a num_timeout well under 30 already saturates the
>    tcp_rto_max() clamp, so nothing reachable changes for the u64 path.
>  - cap max_retries / max_syn_ack_retries at 127, the largest value the
>    field can hold, instead of narrowing the sysctl; 128..255 are still
>    accepted and act as 127.
>  - cap the TCP_DEFER_ACCEPT conversion in secs_to_retrans() at 127 too;
>    larger values kept an acked request from ever expiring. getsockopt()
>    now reports at most 14527 instead of 29887 seconds.
>  - route the TFO path through the same helper; its u32 shift could also
>    wrap to 0 before reaching UB.
>  - document that tcp_synack_retries values above 127 act as 127.
>
> Fixes: e6c022a4fa2d ("tcp: better retrans tracking for defer-accept")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
> v1 -> v2: reworded the commit message, no code change.
> v1: https://lore.kernel.org/netdev/20261023064915.401112-1-jiayuan.chen@linux.dev/
> ---
>  Documentation/networking/ip-sysctl.rst |  4 +++-
>  include/net/tcp.h                      | 16 ++++++++++++----
>  net/ipv4/inet_connection_sock.c        |  2 ++
>  net/ipv4/tcp.c                         |  2 +-
>  net/ipv4/tcp_timer.c                   |  3 ++-
>  5 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 4b19c8d3efda..eac33072be2d 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -956,7 +956,9 @@ 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. The sysctl accepts 0-255, but values above 127 are
> +       silently capped by the SYN-ACK timers to match the 7-bit
> +       request_sock::num_timeout field. 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 0265c27a5fd7..986b7ea05e4a 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.
> @@ -912,12 +914,18 @@ 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,
> +                                                const struct request_sock *req)
>  {
> -       u64 timeout = (u64)req->timeout << req->num_timeout;
> +       u64 timeout = (u64)READ_ONCE(req->timeout) <<
> +                     min_t(u8, req->num_timeout, 30);
> +
> +       return (unsigned long)min_t(u64, timeout, tcp_rto_max(sk));
> +}
>
> -       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 8151c638a24c..ee7b64f3141c 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -1072,6 +1072,8 @@ static void reqsk_timer_handler(struct timer_list *t)
>         net = sock_net(sk_listener);
>         max_syn_ack_retries = READ_ONCE(icsk->icsk_syn_retries) ? :
>                 READ_ONCE(net->ipv4.sysctl_tcp_synack_retries);
> +       max_syn_ack_retries = min_t(int, max_syn_ack_retries,
> +                                   MAX_TCP_SYNACK_RETRIES);
>         /* Normally all the openreqs are young and become mature
>          * (i.e. converted to established socket) for first timeout.
>          * If synack was not acknowledged for 1 second, it means
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 99d7237a623d..a309fd45ad42 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) {
>                         res++;
>                         timeout <<= 1;
>                         if (timeout > rto_max)
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index e56eae4bc341..f7b100334a89 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -469,6 +469,7 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
>          */
>         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);
> @@ -490,7 +491,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

The following review comments are from an LLM, look legitimate, and
IMHO are worth addressing.

Please either incorporate each piece of feedback or respond to
indicate why you think it's not appropriate.

---

Thanks for the fix. The logic looks right to me, and I could reproduce
both splats and confirm the patch fixes them. A few comments:

1) The changelog suggests tcp_synack_retries (or TCP_DEFER_ACCEPT) must be
   raised. But a listener can also set TCP_SYNCNT up to MAX_TCP_SYNCNT
   (127), and that also bounds its SYN-ACK retries. So num_timeout reaches
   the UB range with default sysctls. With TCP_SYNCNT=127 plus
   TCP_RTO_MAX_MS=1000 on the listener, the tcp.h:917 splat (shift
   exponent 64) shows up after about 64 seconds. For TFO it fires already
   at num_timeout 32, because "req->timeout << req->num_timeout" is a
   32-bit shift (req->timeout is u32). Please mention TCP_SYNCNT. It also
   shows the shift clamp is needed independently of the 0..255 sysctl
   range.

2) Before the shift amount goes out of range, the shifted value already
   wraps. With HZ=1000 and req->timeout = 1000:
     (u64)1000 << 61..63 == 0, and (u32)1000 << 29..31 == 0.
   Both give zero-length timers. On an unpatched kernel I see bursts of
   three back-to-back SYN-ACK retransmits about 1 ms apart:
     - regular path: at num_timeout 61-63, and again at 125-127 once x86
       masks the shift count;
     - TFO path: at num_timeout 29-31, 61-63, 93-95 and 125-127.
   That is user-visible on production (non-UBSAN) kernels and is also
   fixed by your clamp. It may be worth a sentence in the changelog.

3) READ_ONCE(req->timeout): as far as I can see, req->timeout is only
   written in reqsk_queue_hash_req() and tcp_fastopen_create_child(),
   both before the request is visible to other CPUs. If this is meant as
   a data-race annotation, please explain it in the changelog (and pair
   it with WRITE_ONCE()). Otherwise I would drop it from this fix.

4) Nit: this patch caps the shift at 30 and patch 2 caps it at 32, for the
   same reason. Maybe use one bound in both, plus a one-line comment on
   why it cannot change the clamped result. 32 also works here, since
   (u64)U32_MAX << 32 still fits in a u64.

5) Nit: MAX_TCP_SYNACK_RETRIES duplicates MAX_TCP_SYNCNT (127), which
   already limits the listener's SYN-ACK retries via TCP_SYNCNT. Maybe
   reuse it, or define one in terms of the other, with the 7-bit
   num_timeout comment.

6) Have you considered limiting the sysctl itself, as 651e92716aaa
   ("sysctl net: Keep tcp_syn_retries inside the boundary") did for
   tcp_syn_retries? That would make the limit visible to admins instead
   of silently capping it. The consumer-side caps would still be needed
   for TCP_DEFER_ACCEPT and for the TFO "+ 1" (128 can never be reached
   by a 7-bit counter), so your approach is reasonable. It would be good
   to say in the changelog why the silent cap was preferred.

7) Minor: once num_timeout wraps to 0, the
   "if (req->num_timeout++ == 0) atomic_dec(&queue->young)" in
   reqsk_timer_handler() and reqsk_queue_removed() decrement queue->young
   again. That is one more symptom fixed by making the request expire at
   127.

8) The documentation text exposes an internal field name
   ("request_sock::num_timeout"). Maybe simply: "Values larger than 127
   are treated as 127."

9) Small user-visible change worth a note in the changelog: for large
   values, getsockopt(TCP_DEFER_ACCEPT) now reports at most
   retrans_to_secs(127) = 14527 seconds instead of 29887.

10) Per Documentation/process/maintainer-netdev.rst ("Bug fixes"), please
    state how the issue was found, how it was validated (the reproducer
    steps are short enough to include), and how the fix was tested.

---

Thanks,
neal

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff()
  2026-10-26 12:32 ` [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff() Jiayuan Chen
@ 2026-10-26 13:36   ` Neal Cardwell
  0 siblings, 0 replies; 4+ messages in thread
From: Neal Cardwell @ 2026-10-26 13:36 UTC (permalink / raw)
  To: Jiayuan Chen
  Cc: netdev, stable, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, Kuniyuki Iwashima, linux-doc, linux-kernel

On Mon, Oct 26, 2026 at 8:33 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
> icsk_backoff is incremented by tcp_retransmit_timer() on every backed-off
> RTO with no cap of its own, and by tcp_send_probe0() up to tcp_retries2.
> It is used as a shift amount in inet_csk_rto_backoff():
>
>         u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff;
>
> Once it climbs past 63 the shift is UB. Seen with tcp_retries2=255 and a
> silent peer, when an ICMP host unreachable goes through
> tcp_ld_RTO_revert():
>
>   UBSAN: shift-out-of-bounds in ./include/net/inet_connection_sock.h:258:40
>   shift exponent 73 is too large for 64-bit type 'long long unsigned int'
>   Call Trace:
>    <IRQ>
>    dump_stack_lvl+0xc1/0xf0
>    dump_stack+0x10/0x20
>    __ubsan_handle_shift_out_of_bounds+0x1d1/0x390
>    tcp_ld_RTO_revert.cold+0x16/0x92
>    tcp_v4_err+0xdb3/0x13e0
>    icmp_socket_deliver+0x1a7/0x390
>    icmp_unreach+0x2dd/0xe50
>    icmp_rcv+0x93e/0xe30
>    ip_protocol_deliver_rcu+0x36b/0x3e0
>    ip_local_deliver_finish+0x37d/0x620
>    ip_local_deliver+0x165/0x1d0
>    ip_rcv+0x284/0x320
>    __netif_receive_skb_one_core+0x16b/0x1c0
>    __netif_receive_skb+0x21/0x150
>    process_backlog+0x37e/0x1200
>    __napi_poll.constprop.0+0xa5/0x5e0
>    net_rx_action+0x4c0/0xf70
>    handle_softirqs+0x204/0x9b0
>    __irq_exit_rcu+0x13c/0x220
>    irq_exit_rcu+0xe/0x20
>    sysvec_apic_timer_interrupt+0x92/0xd0
>    </IRQ>
>
> The orphan check in tcp_probe_timer() calls the same helper. On x86-64
> the oversized shift count is taken modulo 64, so an orphan that should
> be dead can be seen as alive, and tcp_ld_RTO_revert() can end up with a
> smaller RTO than intended.
>
> Cap the shift at 32: any non-zero icsk_rto << 32 exceeds TCP_RTO_MAX, so
> the min_t() result is what the unbounded math would give. The
> ilog2(TCP_RTO_MAX / TCP_RTO_MIN) + 1 bound of tcp_probe0_when() does not
> fit here, as icsk_rto has no TCP_RTO_MIN floor the way tcp_probe0_base()
> has.
>
> Fixes: fcdd1cf4dd63 ("tcp: avoid possible arithmetic overflows")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
>  include/net/inet_connection_sock.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> index 903499581db7..562e12033a5d 100644
> --- a/include/net/inet_connection_sock.h
> +++ b/include/net/inet_connection_sock.h
> @@ -255,9 +255,10 @@ static inline unsigned long
>  inet_csk_rto_backoff(const struct inet_connection_sock *icsk,
>                      unsigned long max_when)
>  {
> -        u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff;
> +       u8 backoff = min_t(u8, 32U, icsk->icsk_backoff);
> +       u64 when = (u64)icsk->icsk_rto << backoff;
>
> -        return (unsigned long)min_t(u64, when, max_when);
> +       return (unsigned long)min_t(u64, when, max_when);
>  }
>
>  struct sock *inet_csk_accept(struct sock *sk, struct proto_accept_arg *arg);
> --
> 2.43.0

The following review comments are from an LLM, look legitimate, and
IMHO are worth addressing.

Please either incorporate each piece of feedback or respond to
indicate why you think it's not appropriate.

---

This is exactly the diff Eric suggested in 2020:
https://lore.kernel.org/netdev/CANn89i+=QRibXsm6tw-eVwhB3wjOtgF-MtVu0G--K5=oWP97+A@mail.gmail.com/
Please add "Suggested-by: Eric Dumazet <edumazet@google.com>" and a Link:
to that message.

As for 1/2, please add the discovery/validation/testing statement
required by maintainer-netdev.rst.

---

Thanks,
neal

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-10-26 13:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-26 12:32 [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Jiayuan Chen
2026-10-26 12:32 ` [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff() Jiayuan Chen
2026-10-26 13:36   ` Neal Cardwell
2026-10-26 13:27 ` [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Neal Cardwell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®