On Tue, Jun 30, 2020 at 2:54 PM Eric Dumazet <[email protected]> wrote: > > On Tue, Jun 30, 2020 at 2:23 PM Eric Dumazet <[email protected]> wrote: > > > > On Tue, Jun 30, 2020 at 2:17 PM Mathieu Desnoyers > > <[email protected]> wrote: > > > > > > ----- On Jun 30, 2020, at 4:56 PM, Eric Dumazet [email protected] wrote: > > > > > > > On Tue, Jun 30, 2020 at 1:44 PM David Miller <[email protected]> > > > > wrote: > > > >> > > > >> From: Eric Dumazet <[email protected]> > > > >> Date: Tue, 30 Jun 2020 13:39:27 -0700 > > > >> > > > >> > The (C) & (B) case are certainly doable. > > > >> > > > > >> > A) case is more complex, I have no idea of breakages of various TCP > > > >> > stacks if a flow got SACK > > > >> > at some point (in 3WHS) but suddenly becomes Reno. > > > >> > > > >> I agree that C and B are the easiest to implement without having to > > > >> add complicated code to handle various negotiated TCP option > > > >> scenerios. > > > >> > > > >> It does seem to be that some entities do A, or did I misread your > > > >> behavioral analysis of various implementations Mathieu? > > > >> > > > >> Thanks. > > > > > > > > Yes, another question about Mathieu cases is do determine the behavior > > > > of all these stacks vs : > > > > SACK option > > > > TCP TS option. > > > > > > I will ask my customer's networking team to investigate these behaviors, > > > which will allow me to prepare a thorough reply to the questions raised > > > by Eric and David. I expect to have an answer within 2-3 weeks at most. > > > > > > Thank you! > > > > > > Great, I am working on adding back support for (B) & (C) by the end of > > this week. > > Note that the security issue (of sending uninit bytes to the wire) has > been independently fixed with [1] > > This means syzbot was able to have MD5+TS+SACK ~6 months ago. > > It seems we (linux) do not enable this combination for PASSIVE flows, > (according to tcp_synack_options()), > but for ACTIVE flows we do nothing special. > > So maybe code in tcp_synack_options() should be mirrored to > tcp_syn_options() for consistency. > (disabling TS if both MD5 and SACK are enabled)
Oh well, tcp_syn_options() is supposed to have the same logic. Maybe we have an issue with SYNCOOKIES (with MD5 + TS + SACK) Nice can of worms. > > [1] > > commit 9424e2e7ad93ffffa88f882c9bc5023570904b55 > Author: Eric Dumazet <[email protected]> > Date: Thu Dec 5 10:10:15 2019 -0800 > > tcp: md5: fix potential overestimation of TCP option space > > Back in 2008, Adam Langley fixed the corner case of packets for flows > having all of the following options : MD5 TS SACK > > Since MD5 needs 20 bytes, and TS needs 12 bytes, no sack block > can be cooked from the remaining 8 bytes. > > tcp_established_options() correctly sets opts->num_sack_blocks > to zero, but returns 36 instead of 32. > > This means TCP cooks packets with 4 extra bytes at the end > of options, containing unitialized bytes. > > Fixes: 33ad798c924b ("tcp: options clean up") > Signed-off-by: Eric Dumazet <[email protected]> > Reported-by: syzbot <[email protected]> > Acked-by: Neal Cardwell <[email protected]> > Acked-by: Soheil Hassas Yeganeh <[email protected]> > Signed-off-by: David S. Miller <[email protected]> > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index > be6d22b8190fa375074062032105879270af4be5..b184f03d743715ef4b2d166ceae651529be77953 > 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -755,8 +755,9 @@ static unsigned int tcp_established_options(struct > sock *sk, struct sk_buff *skb > min_t(unsigned int, eff_sacks, > (remaining - TCPOLEN_SACK_BASE_ALIGNED) / > TCPOLEN_SACK_PERBLOCK); > - size += TCPOLEN_SACK_BASE_ALIGNED + > - opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK; > + if (likely(opts->num_sack_blocks)) > + size += TCPOLEN_SACK_BASE_ALIGNED + > + opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK; > } > > return size;
