Re: [PATCH -next v2 1/2] syncookies: remove ecn_ok validation when decoding option timestamp

[Florian Westphal <fw@strlen.de>]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-10-31 at 13:13 +0100, Florian Westphal wrote:
> > Won't work anymore when tcp_ecn=0 and RTAX_FEATURES route attribute did
> > allow ecn.
> > 
> > Just turn on ecn if the client ts says so.
> > 
> > This means that while syn cookies are in use clients can turn on ecn
> > even if it is off on the server.
> > 
> > However, there seems to be no harm in permitting this.
> > 
> > Alternatively one can extend the test to also perform route lookup and
> > check RTAX_FEATURES, but it simply doesn't appear to be worth the effort.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  Changes since v1:
> >   - reword commit message
> 
> Sorry.
> 
> Google chose to disable ecn on their GFE, so we set sysctl_tcp_ecn to 0
> 
> If I understand your patch, if a synflood is going on, some innocent
> connections could get ECN enabled, while we do not want this to ever
> happen. ECN really hurts our customers, this is a known fact.
>
> You cannot change this like that, it would force us (and maybe others)
> to either revert this patch, or add a knob.

Mot needed, if you think its wrong to remove the check, I will respin
with a proper validation.

> If sysctl_tcp_ecn = 0, there is no way a connection should have ECN
> enabled. This was documented years ago.

It would only get enabled if the echoed timestamp (ie the timestamp we
sent in the synack) indicates that ecn was enabled, i.e. the client or
a middlebox would have to munge/modify it to set the 'ecn on' bit in the
timestamp.

If that is too fragile in your opinion I will respin the patch to include
the additional validation via dst.  We already need to fetch the dst
object anyway to fetch certain route attributes not in the timestamp or
cookie, so its only a matter of reorganizing code first to avoid two lookups.

Let me know what you prefer.

Thanks,
Florian