Re: [PATCH net 5/5] tcp: send real dupACKs by locking advertized window for non-SACK flows

[Eric Dumazet <eric.dumazet@gmail.com>]

On Wed, 2018-03-07 at 22:09 +0200, Ilpo Järvinen wrote:
> Thanks for taking a look.
> 
> On Wed, 7 Mar 2018, Eric Dumazet wrote:
> > On Wed, 2018-03-07 at 14:59 +0200, Ilpo Järvinen wrote:
> > > Currently, the TCP code is overly eager to update window on
> > > every ACK. It makes some of the ACKs that the receiver should
> > > sent as dupACKs look like they update window update that are
> > > not considered real dupACKs by the non-SACK sender-side code.
> > > 
> > > Make sure that when an ofo segment is received, no change to
> > > window is applied if we are going to send a dupACK. It's ok
> > > to change the window for non-dupACKs (such as the first ACK
> > > after ofo arrivals start if that ACK was using delayed ACKs).
> > 
> > This looks dangerous to me.
> > 
> > We certainly want to lower the window at some point, or risk
> > expensive
> > pruning and/or drops.
> 
> I see you're conspiring for "treason" (if you recall those charmy
> times) ;-).
> 
> > It is not clear by reading your changelog/patch, but it looks like
> > some
> > malicious peers could hurt us.
> 
> The malicious peers can certainly send out-of-window data already at
> will 
> (with all of such packets being dropped regardless of that being 
> expensive or not) so I don't see there's a big change for malicious
> case 
> really. The window is only locked for what we've already promised for
> in 
> an earlier ACK! ...Previously, reneging that promise (advertising
> smaller 
> than the previous value) was called "treason" by us (unfortunately,
> the 
> message is no longer as charmy).
> 
> Even with this change, the window is free to change when the ack
> field is 
> updated which for legimate senders occurs typically once per RTT.
> 
> > By current standards, non TCP sack flows are not worth any
> > potential
> > issues.
> 
> Currently non-SACK senders cannot identify almost any duplicate ACKs 
> because the window keeps updating for almost all ACKs. As a result, 
> non-SACK senders end up doing loss recovery only with RTO. RTO
> recovery 
> without SACK is quite annoying because it potentially sends 
> large number of unnecessary rexmits.

I get that, but switching from "always" to "never" sounds dangerous.

May I suggest you refine your patch, to maybe allow win reductions, in
a logarithmic fashion maybe ?

This way, when you send 1000 DUPACK, only few of them would actually
have to lower the window, and 99% of them would be considered as DUPACK
by these prehistoric TCP stacks.