linux-kernel - Re: [PATCH net-next] tcp: fix error handling of tcp_retransmit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANn89i+A1FDBt70NG_VDExQTp5fzJpVMSAwngdbv-dwRPPfCqQ@mail.gmail.com>
Date: Mon, 5 Jan 2026 19:13:01 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: 刘聪聪(方勿) <fangwu.lcc@...group.com>
Cc: ncardwell@...gle.com, davem@...emloft.net, kuba@...nel.org, 
	netdev@...r.kernel.org, kuniyu@...gle.com, dsahern@...nel.org, 
	pabeni@...hat.com, horms@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] tcp: fix error handling of tcp_retransmit_skb

On Mon, Jan 5, 2026 at 6:53 PM 刘聪聪(方勿) <fangwu.lcc@...group.com> wrote:
>
> The tcp_retransmit_timer() function checks if tcp_retransmit_skb()
> returns a value greater than 0, but tcp_retransmit_skb() returns
> 0 on success and negative error codes on failure.

This seems like a bogus claim to me.

tcp_retransmit_skb() can and should return >0 in some cases.

Time to provide a packetdrill test I guess.

   0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

   +0 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>

 +.02 < . 1:1(0) ack 1 win 257
   +0 accept(3, ..., ...) = 4
   // Set a 5s timeout
   +0 setsockopt(4, SOL_TCP, TCP_USER_TIMEOUT, [5000], 4) = 0
   +0 write(4, ..., 11000) = 11000
   +0 > P. 1:10001(10000) ack 1

// TLP
 +.04~+.05 > P. 10001:11001(1000) ack 1
   +0 %{ assert tcpi_retransmits == 0, tcpi_retransmits;\
         assert tcpi_backoff     == 0, tcpi_backoff }%

   // Emulate a congestion  - no packets can get out for 3 seconds
   // Check that we retry every 500ms (6 rounds) w/o backoff
   +0 `tc qdisc replace dev tun0 root pfifo limit 0`
   +3 %{ assert tcpi_retransmits == 6, tcpi_retransmits;\
         assert tcpi_backoff     == 0, tcpi_backoff }%

   // Congestion is now relieved - the next retry should show up some time
   // Hopefully qdisc in the future can inform TCP right away to retry.
   +0 `tc qdisc replace dev tun0 root pfifo limit 1000`
 +.21~+.26 > . 1:1001(1000) ack 1
 +.02 < . 1:1(0) ack 1001 win 257
   +0 > . 1001:3001(2000) ack 1
   // Test the recurring timeout counter is reset. The backoff counter
   // remains one until a new RTT sample is acquired. We do not get a new
   // RTT sample b/c the ACK acks a rtx w/o TS options
   +0 %{ assert tcpi_retransmits == 0, tcpi_retransmits;\
         assert tcpi_backoff     == 1, tcpi_backoff }%

   // Emulate a longer local congestion - the next ACK should trigger
   // more transmission but none can succeed.
   +0 `tc qdisc replace dev tun0 root pfifo limit 0`
   +.02 < . 1:1(0) ack 3001 win 257

   // Socket has timed out after +5s of lack of progress as specified above
   +5.1 write(4, ..., 100) = -1 (ETIMEDOUT)


> This means the
> error handling branch is never executed when retransmission fails.
>
> Fix this by changing the condition to check for != 0 instead of > 0.
>
> Signed-off-by: Liu Congcong <fangwu.lcc@...group.com>
> ---
>  net/ipv4/tcp_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 160080c9021d..4fbb387e7e7b 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -624,7 +624,7 @@ void tcp_retransmit_timer(struct sock *sk)
>         tcp_enter_loss(sk);
>
>         tcp_update_rto_stats(sk);
> -       if (tcp_retransmit_skb(sk, tcp_rtx_queue_head(sk), 1) > 0) {
> +       if (tcp_retransmit_skb(sk, tcp_rtx_queue_head(sk), 1)) {
>                 /* Retransmission failed because of local congestion,
>                  * Let senders fight for local resources conservatively.
>                  */
> --
> 2.17.0
>