Re: [PATCH net-next] tcp: fix error handling of tcp_retransmit_skb
flat view
From: Eric Dumazet <hidden>
Date: 2026-01-05 18:13:13
Also in:
lkml
On Mon, Jan 5, 2026 at 6:53 PM 刘聪聪(方勿) [off-list ref] 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)
quoted hunk ↗ jump to hunk
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 <redacted> --- 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