Re: [PATCH net] tcp: Fix dup-ack detection on non-SACK bidirectional data transfer.

[Guillaume Nault <gnault@redhat.com>]

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.