Netdev List
 help / color / mirror / Atom feed
* [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.
@ 2026-10-22  8:49 Guillaume Nault
  2026-10-22  9:46 ` Eric Dumazet
  2026-10-23 16:10 ` Neal Cardwell
  0 siblings, 2 replies; 7+ messages in thread
From: Guillaume Nault @ 2026-10-22  8:49 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet
  Cc: netdev, Simon Horman, Neal Cardwell, Kuniyuki Iwashima,
	Tom Williams

When receiving a pure ack from a peer that also sends data, we can
have SEG.SEQ > SND.WL1, even though the segment contains no data. This
makes tcp_may_update_window() return true (because of the
"after(ack_seq, tp->snd_wl1)" condition), and tcp_ack_update_window()
then adds FLAG_WIN_UPDATE to the segment set of flags, even when
there's no window update.

Since FLAG_WIN_UPDATE prevents the segment from being considered as
dup-ack (as it's part of FLAG_NOT_DUP), this can inhibit fast
retransmit on non-SACK connections, making recovery very slow as it
now entirely relies on RTO.

Avoid this scenario by setting FLAG_WIN_UPDATE only when a window
update actually occurs. Pure acks with SEG.SEQ > SND.WL1 can now
be detected as dup-ack and trigger fast retransmission.

Add packetdrill selftest. Before this patch, the 1001:2001 segment was
retransmitted upon RTO, now it's sent right after reception of the
third duplicate ack.

Thanks to Tom Williams for the investigation help and initial
reproducer.

Reported-by: Tom Williams <Tom.D.Williams@MorganStanley.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 net/ipv4/tcp_input.c                          |  2 +-
 .../tcp_bidir_fast_retransmit_nosack.pkt      | 42 +++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 99baf14afdfd..ef20bb5a0523 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3959,11 +3959,11 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32
 		nwin <<= tp->rx_opt.snd_wscale;
 
 	if (tcp_may_update_window(tp, ack, ack_seq, nwin)) {
-		flag |= FLAG_WIN_UPDATE;
 		tcp_update_wl(tp, ack_seq);
 
 		if (tp->snd_wnd != nwin) {
 			tp->snd_wnd = nwin;
+			flag |= FLAG_WIN_UPDATE;
 
 			/* Note, it is the only place, where
 			 * fast path is recovered for sending TCP.
diff --git a/tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt b/tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt
new file mode 100644
index 000000000000..5c66b7171d22
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Verify that non-SACK connections properly detect duplicate acks when mixed
+// with other data segments from peer.
+
+`./defaults.sh`
+
+    0     socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
+   +0...0 connect(4, ..., ...) = 0
+
+   // Three-way handshake, SACK not supported by peer
+   +0 > S  0:0(0) <mss 1460,sackOK,TS val 0 ecr 0,nop,wscale 8>
+   +0 < S. 0:0(0) ack 1 win 65535 <mss 1000>
+   +0 > .  1:1(0) ack 1
+
+   // Send 1000 bytes, peer acks everything
+   +0 send(4, ..., 1000, 0) = 1000
+   +0 > P. 1:1001(1000) ack 1
+   +0 < .  1:1(0) ack 1001 win 65535
+
+   // Send 4000 more bytes
+   +0 send(4, ..., 4000, 0) = 4000
+   +0 > P. 1001:5001(4000) ack 1
+
+   // Receive first dup-ack (considering segment 1001-2001 was lost)
+   +0 < .  1:1(0) ack 1001 win 65535
+
+   // Receive unrelated data segment from peer and ack it
+   +0 < P. 1:11(10) ack 1001 win 65535
+   +0 > .  5001:5001(0) ack 11
+
+   // Receive second dup-ack
+   +0 < .  11:11(0) ack 1001 win 65535
+
+   // Receive another data segment from peer and ack it
+   +0 < P. 11:21(10) ack 1001 win 65535
+   +0 > .  5001:5001(0) ack 21
+
+   // Receive third dup-ack, which should trigger fast retransmit (without
+   // waiting for RTO)
+   +0 < .  21:21(0) ack 1001 win 65535
+   +0 > .  1001:2001(1000) ack 21
-- 
2.47.3


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

* Re: [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.
  2026-10-22  8:49 [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer Guillaume Nault
@ 2026-10-22  9:46 ` Eric Dumazet
  2026-10-23 11:53   ` Guillaume Nault
  2026-10-23 16:17   ` Neal Cardwell
  2026-10-23 16:10 ` Neal Cardwell
  1 sibling, 2 replies; 7+ messages in thread
From: Eric Dumazet @ 2026-10-22  9:46 UTC (permalink / raw)
  To: Guillaume Nault
  Cc: David Miller, Jakub Kicinski, Paolo Abeni, netdev, Simon Horman,
	Neal Cardwell, Kuniyuki Iwashima, Tom Williams

Le jeu. 22 oct. 2026 à 10:49, Guillaume Nault <gnault@redhat.com> a écrit :
>
> When receiving a pure ack from a peer that also sends data, we can
> have SEG.SEQ > SND.WL1, even though the segment contains no data. This
> makes tcp_may_update_window() return true (because of the
> "after(ack_seq, tp->snd_wl1)" condition), and tcp_ack_update_window()
> then adds FLAG_WIN_UPDATE to the segment set of flags, even when
> there's no window update.

Nice catch. This also matches RFC 5681 dupack definition, (e):
"the advertised window in the incoming acknowledgment equals the
advertised window in the last incoming acknowledgment".
Maybe worth quoting in the changelog.

>
> Since FLAG_WIN_UPDATE prevents the segment from being considered as
> dup-ack (as it's part of FLAG_NOT_DUP), this can inhibit fast
> retransmit on non-SACK connections, making recovery very slow as it
> now entirely relies on RTO.
>
> Avoid this scenario by setting FLAG_WIN_UPDATE only when a window
> update actually occurs. Pure acks with SEG.SEQ > SND.WL1 can now
> be detected as dup-ack and trigger fast retransmission.
>
> Add packetdrill selftest. Before this patch, the 1001:2001 segment was
> retransmitted upon RTO, now it's sent right after reception of the
> third duplicate ack.
>
> Thanks to Tom Williams for the investigation help and initial
> reproducer.
>
> Reported-by: Tom Williams <Tom.D.Williams@MorganStanley.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> ---
>  net/ipv4/tcp_input.c                          |  2 +-
>  .../tcp_bidir_fast_retransmit_nosack.pkt      | 42 +++++++++++++++++++
>  2 files changed, 43 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 99baf14afdfd..ef20bb5a0523 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -3959,11 +3959,11 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32
>                 nwin <<= tp->rx_opt.snd_wscale;
>
>         if (tcp_may_update_window(tp, ack, ack_seq, nwin)) {
> -               flag |= FLAG_WIN_UPDATE;
>                 tcp_update_wl(tp, ack_seq);
>
>                 if (tp->snd_wnd != nwin) {
>                         tp->snd_wnd = nwin;
> +                       flag |= FLAG_WIN_UPDATE;

One behavior change to keep in mind: because of the "|| !nwin" clause
in tcp_may_update_window(), every zero-window ACK used to carry
FLAG_WIN_UPDATE and was never considered as a dupack.
After this patch, repeated "win 0" ACKs at snd_una with outstanding
data (receiver dropping in-flight packets because its queue is full)
will be counted as dupacks and can trigger fast retransmit + cwnd
reduction while the peer is simply stalled.
This is RFC compliant, I think this is fine, but a packetdrill test
covering this case would be nice.

Also, the header-prediction branch in tcp_ack() still sets
FLAG_WIN_UPDATE unconditionally, so CA_ACK_WIN_UPDATE (only visible to
BPF CC) is now inconsistent between fast and slow paths.

It is quite sad that some TCP stacks do not use SACK in 2026.
They will still be dealing with decade old bugs.

I am unsure if this is a net candidate, perhaps Neal and Kuniyuki can
give their thoughts.

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

* Re: [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.
  2026-10-22  9:46 ` Eric Dumazet
@ 2026-10-23 11:53   ` Guillaume Nault
  2026-10-23 16:17   ` Neal Cardwell
  1 sibling, 0 replies; 7+ messages in thread
From: Guillaume Nault @ 2026-10-23 11:53 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, Jakub Kicinski, Paolo Abeni, netdev, Simon Horman,
	Neal Cardwell, Kuniyuki Iwashima, Tom Williams

On Thu, Oct 22, 2026 at 11:46:48AM +0200, Eric Dumazet wrote:
> Le jeu. 22 oct. 2026 à 10:49, Guillaume Nault <gnault@redhat.com> a écrit :
> >
> > When receiving a pure ack from a peer that also sends data, we can
> > have SEG.SEQ > SND.WL1, even though the segment contains no data. This
> > makes tcp_may_update_window() return true (because of the
> > "after(ack_seq, tp->snd_wl1)" condition), and tcp_ack_update_window()
> > then adds FLAG_WIN_UPDATE to the segment set of flags, even when
> > there's no window update.
> 
> Nice catch. This also matches RFC 5681 dupack definition, (e):
> "the advertised window in the incoming acknowledgment equals the
> advertised window in the last incoming acknowledgment".
> Maybe worth quoting in the changelog.

Good idea, I'll add the quote in v2.

> > Since FLAG_WIN_UPDATE prevents the segment from being considered as
> > dup-ack (as it's part of FLAG_NOT_DUP), this can inhibit fast
> > retransmit on non-SACK connections, making recovery very slow as it
> > now entirely relies on RTO.
> >
> > Avoid this scenario by setting FLAG_WIN_UPDATE only when a window
> > update actually occurs. Pure acks with SEG.SEQ > SND.WL1 can now
> > be detected as dup-ack and trigger fast retransmission.
> >
> > Add packetdrill selftest. Before this patch, the 1001:2001 segment was
> > retransmitted upon RTO, now it's sent right after reception of the
> > third duplicate ack.
> >
> > Thanks to Tom Williams for the investigation help and initial
> > reproducer.
> >
> > Reported-by: Tom Williams <Tom.D.Williams@MorganStanley.com>
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > ---
> >  net/ipv4/tcp_input.c                          |  2 +-
> >  .../tcp_bidir_fast_retransmit_nosack.pkt      | 42 +++++++++++++++++++
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 99baf14afdfd..ef20bb5a0523 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -3959,11 +3959,11 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32
> >                 nwin <<= tp->rx_opt.snd_wscale;
> >
> >         if (tcp_may_update_window(tp, ack, ack_seq, nwin)) {
> > -               flag |= FLAG_WIN_UPDATE;
> >                 tcp_update_wl(tp, ack_seq);
> >
> >                 if (tp->snd_wnd != nwin) {
> >                         tp->snd_wnd = nwin;
> > +                       flag |= FLAG_WIN_UPDATE;
> 
> One behavior change to keep in mind: because of the "|| !nwin" clause
> in tcp_may_update_window(), every zero-window ACK used to carry
> FLAG_WIN_UPDATE and was never considered as a dupack.
> After this patch, repeated "win 0" ACKs at snd_una with outstanding
> data (receiver dropping in-flight packets because its queue is full)
> will be counted as dupacks and can trigger fast retransmit + cwnd
> reduction while the peer is simply stalled.
> This is RFC compliant, I think this is fine, but a packetdrill test
> covering this case would be nice.

Indeed, I also think this new behaviour is valid.

> Also, the header-prediction branch in tcp_ack() still sets
> FLAG_WIN_UPDATE unconditionally, so CA_ACK_WIN_UPDATE (only visible to
> BPF CC) is now inconsistent between fast and slow paths.

I feel I'm missing something about this header prediction code. The
comment says "Window is constant, pure forward advance.", so why does it
set "flag |= FLAG_WIN_UPDATE;" right afterwards? Do I misread the
comment?

My (theoritical) understanding of header prediction is that one of the
precoditions for it to happen is that SEQ.WND == SND.WND. In that case
we could just drop the "flag |= FLAG_WIN_UPDATE;" line. However, I
can't see any such guarantee in the code (but I may have missed
something).

In the end, should I just add a chunk like the following?

 		tcp_update_wl(tp, ack_seq);
 		tcp_snd_una_update(tp, ack);
-		flag |= FLAG_WIN_UPDATE;
+		if (tp->snd_wnd != ntohs(tcp_hdr(skb)->window))
+			flag |= FLAG_WIN_UPDATE;
 
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPHPACKS);

> It is quite sad that some TCP stacks do not use SACK in 2026.
> They will still be dealing with decade old bugs.

Yes, we have what we have :(.

> I am unsure if this is a net candidate, perhaps Neal and Kuniyuki can
> give their thoughts.

I posted it to net because I considered it's a bug in the duplicate ack
detection. But if maintainers think this should be for net-next, that's
okay too.


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

* Re: [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.
  2026-10-22  8:49 [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer Guillaume Nault
  2026-10-22  9:46 ` Eric Dumazet
@ 2026-10-23 16:10 ` Neal Cardwell
  2026-10-23 17:04   ` Neal Cardwell
  1 sibling, 1 reply; 7+ messages in thread
From: Neal Cardwell @ 2026-10-23 16:10 UTC (permalink / raw)
  To: gnault
  Cc: Tom.D.Williams, davem, edumazet, horms, kuba, kuniyu, ncardwell,
	netdev, pabeni, ycheng

From: Neal Cardwell <ncardwell@google.com>

On Fri, Oct 23, 2026 at 7:54#AM Guillaume Nault <gnault@redhat.com> wrote:
>
> On Thu, Oct 22, 2026 at 11:46:48AM +0200, Eric Dumazet wrote:
...
> > >
> > > When receiving a pure ack from a peer that also sends data, we can
> > > have SEG.SEQ > SND.WL1, even though the segment contains no data. This
> > > makes tcp_may_update_window() return true (because of the
> > > "after(ack_seq, tp->snd_wl1)" condition), and tcp_ack_update_window()
> > > then adds FLAG_WIN_UPDATE to the segment set of flags, even when
> > > there's no window update.
> >
> > Nice catch. This also matches RFC 5681 dupack definition, (e):
> > "the advertised window in the incoming acknowledgment equals the
> > advertised window in the last incoming acknowledgment".
> > Maybe worth quoting in the changelog.
>
> Good idea, I'll add the quote in v2.

Thanks. I agree this is worth mentioning in the changelog.

> > > Since FLAG_WIN_UPDATE prevents the segment from being considered as
> > > dup-ack (as it's part of FLAG_NOT_DUP), this can inhibit fast
> > > retransmit on non-SACK connections, making recovery very slow as it
> > > now entirely relies on RTO.
> > >
> > > Avoid this scenario by setting FLAG_WIN_UPDATE only when a window
> > > update actually occurs. Pure acks with SEG.SEQ > SND.WL1 can now
> > > be detected as dup-ack and trigger fast retransmission.
> > >
> > > Add packetdrill selftest. Before this patch, the 1001:2001 segment was
> > > retransmitted upon RTO, now it's sent right after reception of the
> > > third duplicate ack.
> > >
> > > Thanks to Tom Williams for the investigation help and initial
> > > reproducer.
> > >
> > > Reported-by: Tom Williams <Tom.D.Williams@MorganStanley.com>
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > > ---
> > >  net/ipv4/tcp_input.c                          |  2 +-
> > >  .../tcp_bidir_fast_retransmit_nosack.pkt      | 42 +++++++++++++++++++
> > >  2 files changed, 43 insertions(+), 1 deletion(-)
> > >  create mode 100644 tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt
> > >
> > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > > index 99baf14afdfd..ef20bb5a0523 100644
> > > --- a/net/ipv4/tcp_input.c
> > > +++ b/net/ipv4/tcp_input.c
> > > @@ -3959,11 +3959,11 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32
> > >                 nwin <<= tp->rx_opt.snd_wscale;
> > >
> > >         if (tcp_may_update_window(tp, ack, ack_seq, nwin)) {
> > > -               flag |= FLAG_WIN_UPDATE;
> > >                 tcp_update_wl(tp, ack_seq);
> > >
> > >                 if (tp->snd_wnd != nwin) {
> > >                         tp->snd_wnd = nwin;
> > > +                       flag |= FLAG_WIN_UPDATE;

Thanks for the fix!

> > One behavior change to keep in mind: because of the "|| !nwin" clause
> > in tcp_may_update_window(), every zero-window ACK used to carry
> > FLAG_WIN_UPDATE and was never considered as a dupack.
> > After this patch, repeated "win 0" ACKs at snd_una with outstanding
> > data (receiver dropping in-flight packets because its queue is full)
> > will be counted as dupacks and can trigger fast retransmit + cwnd
> > reduction while the peer is simply stalled.
> > This is RFC compliant, I think this is fine, but a packetdrill test
> > covering this case would be nice.
>
> Indeed, I also think this new behaviour is valid.
>
> > Also, the header-prediction branch in tcp_ack() still sets
> > FLAG_WIN_UPDATE unconditionally, so CA_ACK_WIN_UPDATE (only visible to
> > BPF CC) is now inconsistent between fast and slow paths.
>
> I feel I'm missing something about this header prediction code. The
> comment says "Window is constant, pure forward advance.", so why does it
> set "flag |= FLAG_WIN_UPDATE;" right afterwards? Do I misread the
> comment?
>
> My (theoritical) understanding of header prediction is that one of the
> precoditions for it to happen is that SEQ.WND == SND.WND. In that case
> we could just drop the "flag |= FLAG_WIN_UPDATE;" line. However, I
> can't see any such guarantee in the code (but I may have missed
> something).

I agree that setting the FLAG_WIN_UPDATE bit in the "Window is constant, pure
forward advance" code path looks incorrect.

> In the end, should I just add a chunk like the following?
>
>                 tcp_update_wl(tp, ack_seq);
>                 tcp_snd_una_update(tp, ack);
> -               flag |= FLAG_WIN_UPDATE;
> +               if (tp->snd_wnd != ntohs(tcp_hdr(skb)->window))
> +                       flag |= FLAG_WIN_UPDATE;
>
>                 NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPHPACKS);

Keep in mind that tp->snd_wnd is after shifting for window scaling, and
ntohs(tcp_hdr(skb)->window) is before shifting for window scaling, so they
probably should not be compared.

But in any case AFAICT that code should not be needed, because if we get to
that block header prediction succeeded, meaning there is no window update.

> I posted it to net because I considered it's a bug in the duplicate ack
> detection. But if maintainers think this should be for net-next, that's
> okay too.

I agree it seems lke a bug fix, so "net" seems reasonable IMHO.

I asked two different frontier LLMs to take a look at this thread, and they
both pointed out an intertwined issue we haven't noted yet, which is that when
an ACK advances tp->snd_una by less than 1 whole segment of the head skb in the
retransmit queue, tcp_clean_rtx_queue() breaks out early without setting
FLAG_ACKED. Previously, such a pure ACK relied on FLAG_WIN_UPDATE being set in
order to have FLAG_NOT_DUP set and avoid being treated as dubious by
tcp_ack_is_dubious() in TCP_CA_Open, and thus avoid calling
tcp_fastretrans_alert(). If we fix the FLAG_WIN_UPDATE code in this way, then
FLAG_WIN_UPDATE does not get set on sub-MSS ACKs, and such cases will make
tcp_ack_is_dubious() true and we'll call tcp_fastretrans_alert().  AFAICT
nothing serious should happen, since FLAG_SND_UNA_ADVANCED should be true and
so num_dupack should be 0 and nothing should happen to the connection state,
and nothing spurious should be sent. But it's performance overhead, and a
confusing dynamic that might lead to bugs down the road.

Below is a proposed patch, cooked by a frontier LLM and lightly edited by me,
that should factor in all these considerations. This is compiled but not yet
tested. I will kick off some tests for it.

~~~~ possible patch:

Currently, FLAG_WIN_UPDATE (/* Incoming ACK was a window update. */) is set
in two places even when tp->snd_wnd does not change:

1. In tcp_ack_update_window(), flag |= FLAG_WIN_UPDATE is set whenever
   tcp_may_update_window() returns true, outside the if (tp->snd_wnd != nwin)
   check.
2. In the tcp_ack() header-prediction fast path, where tp->pred_flags has
   already verified that the advertised window is unchanged
   (ntohs(th->window) == (tp->snd_wnd >> tp->rx_opt.snd_wscale)), line 4357
   unconditionally sets flag |= FLAG_WIN_UPDATE.

This causes two issues:

First, on the slow path in tcp_ack_update_window(), a pure duplicate ACK
(ack == tp->snd_una) with an unchanged window (nwin == tp->snd_wnd) still
satisfies tcp_may_update_window() whenever after(ack_seq, tp->snd_wl1) is true
(or when ack_seq == tp->snd_wl1 && !nwin). In particular, when the peer sends
an in-order data segment of length L > 0 with sequence number S, tcp_update_wl()
sets tp->snd_wl1 = S while tp->rcv_nxt advances to S + L. If the peer's next
packet is a pure duplicate ACK with unchanged window, its ack_seq is S + L >
tp->snd_wl1. tcp_ack_update_window() therefore sets FLAG_WIN_UPDATE (which is
part of FLAG_NOT_DUP), preventing the ACK from being counted as a duplicate ACK
(RFC 5681, Section 2, condition (e)) and inhibiting Fast Retransmit on non-SACK
connections.

Second, tcp_in_ack_event() maps FLAG_WIN_UPDATE to CA_ACK_WIN_UPDATE and passes
it to icsk_ca_ops->in_ack_event(). Because both the fast path and slow path set
FLAG_WIN_UPDATE whenever snd_una or snd_wl1 advances, CA_ACK_WIN_UPDATE is
falsely reported on every cumulative ACK even when the peer's advertised receive
window is unchanged.

Note that when an ACK advances tp->snd_una by less than 1 whole segment of the
head skb in the retransmit queue, tcp_clean_rtx_queue() breaks out early without
setting FLAG_ACKED. Previously, such a pure ACK relied on FLAG_WIN_UPDATE being
set in order to have FLAG_NOT_DUP set and avoid being treated as dubious by
tcp_ack_is_dubious() in TCP_CA_Open, and thus avoid calling
tcp_fastretrans_alert().

Fix this by:
1. Including FLAG_SND_UNA_ADVANCED in FLAG_NOT_DUP, since an ACK that advances
   SND.UNA is by definition not a duplicate ACK.
2. Moving flag |= FLAG_WIN_UPDATE inside if (tp->snd_wnd != nwin) in
   tcp_ack_update_window(), so FLAG_WIN_UPDATE is only set when tp->snd_wnd
   actually changes.
3. Removing flag |= FLAG_WIN_UPDATE from the constant-window header-prediction
   fast path in tcp_ack().

This updates FLAG_WIN_UPDATE to match part (e) of the RFC 5681 dupack
definition: "the advertised window in the incoming acknowledgment equals the
advertised window in the last incoming acknowledgment".

This updates FLAG_NOT_DUP to include part (d) of the RFC 5681 dupack
definition: "the acknowledgment number is equal to the greatest acknowledgment
received on the given connection (TCP.UNA from [RFC793])"

---
 net/ipv4/tcp_input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 99baf14afdfd..1feb370804b1 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -107,7 +107,8 @@ int sysctl_tcp_max_orphans __read_mostly = NR_FILE;
 #define FLAG_TS_PROGRESS	0x40000 /* Positive timestamp delta */
 
 #define FLAG_ACKED		(FLAG_DATA_ACKED|FLAG_SYN_ACKED)
-#define FLAG_NOT_DUP		(FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED)
+#define FLAG_NOT_DUP		(FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED|\
+				 FLAG_SND_UNA_ADVANCED)
 #define FLAG_CA_ALERT		(FLAG_DATA_SACKED|FLAG_ECE|FLAG_DSACKING_ACK)
 #define FLAG_FORWARD_PROGRESS	(FLAG_ACKED|FLAG_DATA_SACKED)
 
@@ -3959,10 +3960,10 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32
 		nwin <<= tp->rx_opt.snd_wscale;
 
 	if (tcp_may_update_window(tp, ack, ack_seq, nwin)) {
-		flag |= FLAG_WIN_UPDATE;
 		tcp_update_wl(tp, ack_seq);
 
 		if (tp->snd_wnd != nwin) {
+			flag |= FLAG_WIN_UPDATE;
 			tp->snd_wnd = nwin;
 
 			/* Note, it is the only place, where
@@ -4354,7 +4355,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 		 */
 		tcp_update_wl(tp, ack_seq);
 		tcp_snd_una_update(tp, ack);
-		flag |= FLAG_WIN_UPDATE;
 
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPHPACKS);
 	} else {
-- 
2.47.3

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

* Re: [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.
  2026-10-22  9:46 ` Eric Dumazet
  2026-10-23 11:53   ` Guillaume Nault
@ 2026-10-23 16:17   ` Neal Cardwell
  1 sibling, 0 replies; 7+ messages in thread
From: Neal Cardwell @ 2026-10-23 16:17 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Guillaume Nault, David Miller, Jakub Kicinski, Paolo Abeni,
	netdev, Simon Horman, Kuniyuki Iwashima, Tom Williams

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On Thu, Oct 22, 2026 at 5:47 AM Eric Dumazet <edumazet@kernel.org> wrote:
> One behavior change to keep in mind: because of the "|| !nwin" clause
> in tcp_may_update_window(), every zero-window ACK used to carry
> FLAG_WIN_UPDATE and was never considered as a dupack.
> After this patch, repeated "win 0" ACKs at snd_una with outstanding
> data (receiver dropping in-flight packets because its queue is full)
> will be counted as dupacks and can trigger fast retransmit + cwnd
> reduction while the peer is simply stalled.
> This is RFC compliant, I think this is fine, but a packetdrill test
> covering this case would be nice.

I agree with Eric's point, and agree it should be acceptable.

I asked an LLM to cook a packetdrill test based on Eric's suggestion,
and it LGTM. I've attached it. I can post it officially if folks agree
it's useful.

thanks,
neal

[-- Attachment #2: tcp_zero_wnd_dupack_nosack.pkt --]
[-- Type: application/octet-stream, Size: 3936 bytes --]

// SPDX-License-Identifier: GPL-2.0
//
// Test a non-SACK sender that receives repeated zero-window ACKs at SND.UNA
// while data is outstanding: the receiver's queue is full, so it drops the
// in-flight data and sends a zero-window ACK for each dropped segment.
//
// Only the first zero-window ACK changes the window (snd_wnd goes to 0).
// The following identical ACKs meet every condition in the RFC 5681
// definition of a duplicate ACK, so the third one triggers a NewReno fast
// retransmit and a cwnd reduction. With snd_wnd == 0, that retransmit also
// acts as a zero window probe (see __tcp_retransmit_skb()).
//
// Before "tcp: Fix dup-ack detection on non-SACK bidirectional data
// transfer.", all zero-window ACKs got FLAG_WIN_UPDATE (because of the
// "|| !nwin" clause in tcp_may_update_window()), so none was counted as a
// dupack. Instead, the first retransmit was the zero window probe sent by
// the RTO timer, at the same time as the second retransmit below, with the
// same cwnd and ssthresh.

`./defaults.sh`

// Establish a connection. The peer supports neither SACK nor timestamps.
    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,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <mss 1460,nop,wscale 8>
  +.1 < . 1:1(0) ack 1 win 320
   +0 accept(3, ..., ...) = 4

// Send 10 MSS.
   +0 write(4, ..., 10000) = 10000
   +0 > P. 1:10001(10000) ack 1

// The peer acks 1:2001, so cwnd grows to 12. Then the peer's receive queue is
// full: it drops 2001:3001 and advertises a zero window. Not a dupack, because
// the window changed.
  +.1 < . 1:1(0) ack 2001 win 320
   +0 < . 1:1(0) ack 2001 win 0
   +0 %{
assert tcpi_snd_wnd == 0, tcpi_snd_wnd
assert tcpi_ca_state == TCP_CA_Open, tcpi_ca_state
assert tcpi_sacked == 0, tcpi_sacked
assert tcpi_snd_cwnd == 12, tcpi_snd_cwnd
}%

// The peer also drops 3001:4001 and 4001:5001, sending an identical
// zero-window ACK for each of them: dupacks.
   +0 < . 1:1(0) ack 2001 win 0
   +0 < . 1:1(0) ack 2001 win 0
   +0 %{
assert tcpi_ca_state == TCP_CA_Disorder, tcpi_ca_state
assert tcpi_sacked == 2, tcpi_sacked
}%

// The third dupack (for 5001:6001) triggers a fast retransmit and a cwnd
// reduction, although snd_wnd is 0.
   +0 < . 1:1(0) ack 2001 win 0
   +0 > . 2001:3001(1000) ack 1
   +0 %{
assert tcpi_ca_state == TCP_CA_Recovery, tcpi_ca_state
assert tcpi_snd_ssthresh == 8, tcpi_snd_ssthresh
assert tcpi_snd_cwnd == 5, tcpi_snd_cwnd
assert tcpi_lost == 1, tcpi_lost
assert tcpi_retrans == 1, tcpi_retrans
}%

// Zero-window ACKs for the other dropped segments, 6001:10001.
   +0 < . 1:1(0) ack 2001 win 0
   +0 < . 1:1(0) ack 2001 win 0
   +0 < . 1:1(0) ack 2001 win 0
   +0 < . 1:1(0) ack 2001 win 0
   +0 %{
assert tcpi_ca_state == TCP_CA_Recovery, tcpi_ca_state
assert tcpi_sacked == 7, tcpi_sacked
assert tcpi_reordering == 3, tcpi_reordering
}%

// The peer is still stalled, so it drops the fast retransmit as well. That
// is one dupack more than packets_out - 1, which NewReno takes as a sign of
// reordering (tcp_check_reno_reordering()).
 +.05 < . 1:1(0) ack 2001 win 0
   +0 %{
assert tcpi_sacked == 7, tcpi_sacked
assert tcpi_reordering == 8, tcpi_reordering
assert tcpi_reord_seen == 1, tcpi_reord_seen
}%

// RTO fires 300ms after the fast retransmit. snd_wnd is still 0, so
// tcp_retransmit_timer() retransmits the head as a zero window probe.
 +.25 > . 2001:3001(1000) ack 1
   +0 %{
assert tcpi_ca_state == TCP_CA_Loss, tcpi_ca_state
assert tcpi_snd_cwnd == 1, tcpi_snd_cwnd
assert tcpi_snd_ssthresh == 8, tcpi_snd_ssthresh
assert tcpi_lost == 8, tcpi_lost
assert tcpi_total_retrans == 2, tcpi_total_retrans
}%

// The peer application reads: the peer accepts the probe and opens its
// window. Repair continues in slow start.
  +.1 < . 1:1(0) ack 3001 win 320
   +0 > . 3001:5001(2000) ack 1

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

* Re: [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.
  2026-10-23 16:10 ` Neal Cardwell
@ 2026-10-23 17:04   ` Neal Cardwell
  2026-10-23 20:31     ` Neal Cardwell
  0 siblings, 1 reply; 7+ messages in thread
From: Neal Cardwell @ 2026-10-23 17:04 UTC (permalink / raw)
  To: Neal Cardwell
  Cc: gnault, Tom.D.Williams, davem, edumazet, horms, kuba, kuniyu,
	netdev, pabeni, ycheng

On Fri, Oct 23, 2026 at 12:10 PM Neal Cardwell <ncardwell.sw@gmail.com> wrote:
...
> Below is a proposed patch, cooked by a frontier LLM and lightly edited by me,
> that should factor in all these considerations. This is compiled but not yet
> tested. I will kick off some tests for it.
>
> ~~~~ possible patch:
>
> Currently, FLAG_WIN_UPDATE (/* Incoming ACK was a window update. */) is set
> in two places even when tp->snd_wnd does not change:
>
> 1. In tcp_ack_update_window(), flag |= FLAG_WIN_UPDATE is set whenever
>    tcp_may_update_window() returns true, outside the if (tp->snd_wnd != nwin)
>    check.
> 2. In the tcp_ack() header-prediction fast path, where tp->pred_flags has
>    already verified that the advertised window is unchanged
>    (ntohs(th->window) == (tp->snd_wnd >> tp->rx_opt.snd_wscale)), line 4357
>    unconditionally sets flag |= FLAG_WIN_UPDATE.
>
> This causes two issues:
>
> First, on the slow path in tcp_ack_update_window(), a pure duplicate ACK
> (ack == tp->snd_una) with an unchanged window (nwin == tp->snd_wnd) still
> satisfies tcp_may_update_window() whenever after(ack_seq, tp->snd_wl1) is true
> (or when ack_seq == tp->snd_wl1 && !nwin). In particular, when the peer sends
> an in-order data segment of length L > 0 with sequence number S, tcp_update_wl()
> sets tp->snd_wl1 = S while tp->rcv_nxt advances to S + L. If the peer's next
> packet is a pure duplicate ACK with unchanged window, its ack_seq is S + L >
> tp->snd_wl1. tcp_ack_update_window() therefore sets FLAG_WIN_UPDATE (which is
> part of FLAG_NOT_DUP), preventing the ACK from being counted as a duplicate ACK
> (RFC 5681, Section 2, condition (e)) and inhibiting Fast Retransmit on non-SACK
> connections.
>
> Second, tcp_in_ack_event() maps FLAG_WIN_UPDATE to CA_ACK_WIN_UPDATE and passes
> it to icsk_ca_ops->in_ack_event(). Because both the fast path and slow path set
> FLAG_WIN_UPDATE whenever snd_una or snd_wl1 advances, CA_ACK_WIN_UPDATE is
> falsely reported on every cumulative ACK even when the peer's advertised receive
> window is unchanged.
>
> Note that when an ACK advances tp->snd_una by less than 1 whole segment of the
> head skb in the retransmit queue, tcp_clean_rtx_queue() breaks out early without
> setting FLAG_ACKED. Previously, such a pure ACK relied on FLAG_WIN_UPDATE being
> set in order to have FLAG_NOT_DUP set and avoid being treated as dubious by
> tcp_ack_is_dubious() in TCP_CA_Open, and thus avoid calling
> tcp_fastretrans_alert().
>
> Fix this by:
> 1. Including FLAG_SND_UNA_ADVANCED in FLAG_NOT_DUP, since an ACK that advances
>    SND.UNA is by definition not a duplicate ACK.
> 2. Moving flag |= FLAG_WIN_UPDATE inside if (tp->snd_wnd != nwin) in
>    tcp_ack_update_window(), so FLAG_WIN_UPDATE is only set when tp->snd_wnd
>    actually changes.
> 3. Removing flag |= FLAG_WIN_UPDATE from the constant-window header-prediction
>    fast path in tcp_ack().
>
> This updates FLAG_WIN_UPDATE to match part (e) of the RFC 5681 dupack
> definition: "the advertised window in the incoming acknowledgment equals the
> advertised window in the last incoming acknowledgment".
>
> This updates FLAG_NOT_DUP to include part (d) of the RFC 5681 dupack
> definition: "the acknowledgment number is equal to the greatest acknowledgment
> received on the given connection (TCP.UNA from [RFC793])"
>
> ---
>  net/ipv4/tcp_input.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 99baf14afdfd..1feb370804b1 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -107,7 +107,8 @@ int sysctl_tcp_max_orphans __read_mostly = NR_FILE;
>  #define FLAG_TS_PROGRESS       0x40000 /* Positive timestamp delta */
>
>  #define FLAG_ACKED             (FLAG_DATA_ACKED|FLAG_SYN_ACKED)
> -#define FLAG_NOT_DUP           (FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED)
> +#define FLAG_NOT_DUP           (FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED|\
> +                                FLAG_SND_UNA_ADVANCED)
>  #define FLAG_CA_ALERT          (FLAG_DATA_SACKED|FLAG_ECE|FLAG_DSACKING_ACK)
>  #define FLAG_FORWARD_PROGRESS  (FLAG_ACKED|FLAG_DATA_SACKED)
>
> @@ -3959,10 +3960,10 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32
>                 nwin <<= tp->rx_opt.snd_wscale;
>
>         if (tcp_may_update_window(tp, ack, ack_seq, nwin)) {
> -               flag |= FLAG_WIN_UPDATE;
>                 tcp_update_wl(tp, ack_seq);
>
>                 if (tp->snd_wnd != nwin) {
> +                       flag |= FLAG_WIN_UPDATE;
>                         tp->snd_wnd = nwin;
>
>                         /* Note, it is the only place, where
> @@ -4354,7 +4355,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
>                  */
>                 tcp_update_wl(tp, ack_seq);
>                 tcp_snd_una_update(tp, ack);
> -               flag |= FLAG_WIN_UPDATE;
>
>                 NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPHPACKS);
>         } else {
> --
> 2.47.3

This patch passes all of our internal tests, as well as the new
tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt
test that Guillaume Nault posted in this thread.

neal

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

* Re: [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.
  2026-10-23 17:04   ` Neal Cardwell
@ 2026-10-23 20:31     ` Neal Cardwell
  0 siblings, 0 replies; 7+ messages in thread
From: Neal Cardwell @ 2026-10-23 20:31 UTC (permalink / raw)
  To: Neal Cardwell
  Cc: gnault, Tom.D.Williams, davem, edumazet, horms, kuba, kuniyu,
	netdev, pabeni, ycheng

[-- Attachment #1: Type: text/plain, Size: 6018 bytes --]

On Fri, Oct 23, 2026 at 1:04 PM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Fri, Oct 23, 2026 at 12:10 PM Neal Cardwell <ncardwell.sw@gmail.com> wrote:
> ...
> > Below is a proposed patch, cooked by a frontier LLM and lightly edited by me,
> > that should factor in all these considerations. This is compiled but not yet
> > tested. I will kick off some tests for it.
> >
> > ~~~~ possible patch:
> >
> > Currently, FLAG_WIN_UPDATE (/* Incoming ACK was a window update. */) is set
> > in two places even when tp->snd_wnd does not change:
> >
> > 1. In tcp_ack_update_window(), flag |= FLAG_WIN_UPDATE is set whenever
> >    tcp_may_update_window() returns true, outside the if (tp->snd_wnd != nwin)
> >    check.
> > 2. In the tcp_ack() header-prediction fast path, where tp->pred_flags has
> >    already verified that the advertised window is unchanged
> >    (ntohs(th->window) == (tp->snd_wnd >> tp->rx_opt.snd_wscale)), line 4357
> >    unconditionally sets flag |= FLAG_WIN_UPDATE.
> >
> > This causes two issues:
> >
> > First, on the slow path in tcp_ack_update_window(), a pure duplicate ACK
> > (ack == tp->snd_una) with an unchanged window (nwin == tp->snd_wnd) still
> > satisfies tcp_may_update_window() whenever after(ack_seq, tp->snd_wl1) is true
> > (or when ack_seq == tp->snd_wl1 && !nwin). In particular, when the peer sends
> > an in-order data segment of length L > 0 with sequence number S, tcp_update_wl()
> > sets tp->snd_wl1 = S while tp->rcv_nxt advances to S + L. If the peer's next
> > packet is a pure duplicate ACK with unchanged window, its ack_seq is S + L >
> > tp->snd_wl1. tcp_ack_update_window() therefore sets FLAG_WIN_UPDATE (which is
> > part of FLAG_NOT_DUP), preventing the ACK from being counted as a duplicate ACK
> > (RFC 5681, Section 2, condition (e)) and inhibiting Fast Retransmit on non-SACK
> > connections.
> >
> > Second, tcp_in_ack_event() maps FLAG_WIN_UPDATE to CA_ACK_WIN_UPDATE and passes
> > it to icsk_ca_ops->in_ack_event(). Because both the fast path and slow path set
> > FLAG_WIN_UPDATE whenever snd_una or snd_wl1 advances, CA_ACK_WIN_UPDATE is
> > falsely reported on every cumulative ACK even when the peer's advertised receive
> > window is unchanged.
> >
> > Note that when an ACK advances tp->snd_una by less than 1 whole segment of the
> > head skb in the retransmit queue, tcp_clean_rtx_queue() breaks out early without
> > setting FLAG_ACKED. Previously, such a pure ACK relied on FLAG_WIN_UPDATE being
> > set in order to have FLAG_NOT_DUP set and avoid being treated as dubious by
> > tcp_ack_is_dubious() in TCP_CA_Open, and thus avoid calling
> > tcp_fastretrans_alert().
> >
> > Fix this by:
> > 1. Including FLAG_SND_UNA_ADVANCED in FLAG_NOT_DUP, since an ACK that advances
> >    SND.UNA is by definition not a duplicate ACK.
> > 2. Moving flag |= FLAG_WIN_UPDATE inside if (tp->snd_wnd != nwin) in
> >    tcp_ack_update_window(), so FLAG_WIN_UPDATE is only set when tp->snd_wnd
> >    actually changes.
> > 3. Removing flag |= FLAG_WIN_UPDATE from the constant-window header-prediction
> >    fast path in tcp_ack().
> >
> > This updates FLAG_WIN_UPDATE to match part (e) of the RFC 5681 dupack
> > definition: "the advertised window in the incoming acknowledgment equals the
> > advertised window in the last incoming acknowledgment".
> >
> > This updates FLAG_NOT_DUP to include part (d) of the RFC 5681 dupack
> > definition: "the acknowledgment number is equal to the greatest acknowledgment
> > received on the given connection (TCP.UNA from [RFC793])"
> >
> > ---
> >  net/ipv4/tcp_input.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 99baf14afdfd..1feb370804b1 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -107,7 +107,8 @@ int sysctl_tcp_max_orphans __read_mostly = NR_FILE;
> >  #define FLAG_TS_PROGRESS       0x40000 /* Positive timestamp delta */
> >
> >  #define FLAG_ACKED             (FLAG_DATA_ACKED|FLAG_SYN_ACKED)
> > -#define FLAG_NOT_DUP           (FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED)
> > +#define FLAG_NOT_DUP           (FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED|\
> > +                                FLAG_SND_UNA_ADVANCED)
> >  #define FLAG_CA_ALERT          (FLAG_DATA_SACKED|FLAG_ECE|FLAG_DSACKING_ACK)
> >  #define FLAG_FORWARD_PROGRESS  (FLAG_ACKED|FLAG_DATA_SACKED)
> >
> > @@ -3959,10 +3960,10 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32
> >                 nwin <<= tp->rx_opt.snd_wscale;
> >
> >         if (tcp_may_update_window(tp, ack, ack_seq, nwin)) {
> > -               flag |= FLAG_WIN_UPDATE;
> >                 tcp_update_wl(tp, ack_seq);
> >
> >                 if (tp->snd_wnd != nwin) {
> > +                       flag |= FLAG_WIN_UPDATE;
> >                         tp->snd_wnd = nwin;
> >
> >                         /* Note, it is the only place, where
> > @@ -4354,7 +4355,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
> >                  */
> >                 tcp_update_wl(tp, ack_seq);
> >                 tcp_snd_una_update(tp, ack);
> > -               flag |= FLAG_WIN_UPDATE;
> >
> >                 NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPHPACKS);
> >         } else {
> > --
> > 2.47.3
>
> This patch passes all of our internal tests, as well as the new
> tools/testing/selftests/net/packetdrill/tcp_bidir_fast_retransmit_nosack.pkt
> test that Guillaume Nault posted in this thread.

I got a second frontier LLM to review this patch, and it assessed the
patch as correct.

During its review it also found a TLP-related packetdrill scenario
that shows buggy kernel TCP behavior with the current upstream
FLAG_WIN_UPDATE logic but is fixed by either this patch, or
Guillaume's original patch in this thread. I have attached the
packetdrill test.


neal

[-- Attachment #2: tcp_tlp_bidir_dupack_nodsack.pkt --]
[-- Type: application/octet-stream, Size: 2675 bytes --]

// SPDX-License-Identifier: GPL-2.0
//
// TLP with a SACK-capable peer that does not send DSACKs (e.g. some macOS
// stacks), on a bidirectional connection.
//
// The TLP retransmission turns out to be a duplicate. The peer answers it with
// a pure duplicate ACK at tlp_high_seq, but that dupack follows a data segment
// from the peer, so its SEG.SEQ is above SND.WL1.
//
// Before "tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.",
// tcp_ack_update_window() set FLAG_WIN_UPDATE on that dupack (SEG.SEQ >
// SND.WL1), so tcp_process_tlp_ack() did not treat it as a "pure dupack" and
// left the TLP episode open. The next ACK above tlp_high_seq was then taken as
// proof of a loss, and cwnd was reduced (TCPLossProbeRecovery) although
// nothing was lost.
//
// With the fix, the dupack has no FLAG_WIN_UPDATE (the window is unchanged),
// so it closes the TLP episode as "original and TLP probe arrived; no loss",
// and cwnd is not reduced.

`./defaults.sh`

    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

// SACK but no timestamps.
   +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>
  +.1 < . 1:1(0) ack 1 win 320
   +0 accept(3, ..., ...) = 4

// Send 2 MSS. TLP retransmits the last MSS after PTO = 2 * srtt = 200ms.
   +0 write(4, ..., 2000) = 2000
   +0 > P. 1:2001(2000) ack 1
  +.2 > P. 1001:2001(1000) ack 1

// The original flight arrived: the peer cumulatively ACKs it.
  +.1 < . 1:1(0) ack 2001 win 320

// The peer sends a request, which we ACK.
   +0 < P. 1:101(100) ack 2001 win 320
   +0~+.05 > . 2001:2001(0) ack 101

// The duplicate TLP retransmission reaches the peer, which sends a pure dupack
// (no DSACK). Its SEG.SEQ (101) is above SND.WL1 (1); the window is unchanged.
   +0 < . 101:101(0) ack 2001 win 320

// Send and get ACKed one more MSS, above tlp_high_seq.
   +0 write(4, ..., 1000) = 1000
   +0 > P. 2001:3001(1000) ack 101
  +.1 < . 101:101(0) ack 3001 win 320

// No loss happened, so there must be no TLP-based cwnd reduction: the flow is
// application-limited, so cwnd stays at the initial 10 and ssthresh is never
// set (a reduction would set ssthresh to 7 and cwnd to 7).
   +0 %{
assert tcpi_ca_state == TCP_CA_Open, tcpi_ca_state
assert tcpi_snd_cwnd == 10, tcpi_snd_cwnd
assert tcpi_snd_ssthresh > 1000000, tcpi_snd_ssthresh
}%
   +0 `nstat -az TcpExtTCPLossProbes TcpExtTCPLossProbeRecovery | tail -n +2 >&2;
       [ "$(nstat -az TcpExtTCPLossProbeRecovery | awk '/TcpExtTCPLossProbeRecovery/ {print $2}')" = 0 ]`

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

end of thread, other threads:[~2026-10-23 20:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-22  8:49 [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer Guillaume Nault
2026-10-22  9:46 ` Eric Dumazet
2026-10-23 11:53   ` Guillaume Nault
2026-10-23 16:17   ` Neal Cardwell
2026-10-23 16:10 ` Neal Cardwell
2026-10-23 17:04   ` Neal Cardwell
2026-10-23 20:31     ` Neal Cardwell

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