netdev - Re: [PATCH] RFC2988bis + taking RTT sample from 3WHS for the passive open side

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 8 Jun 2011 10:04:30 -0700
From:	Jerry Chu <hkchu@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	Hagen Paul Pfeifer <hagen@...u.net>, tsunanet@...il.com,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] RFC2988bis + taking RTT sample from 3WHS for the passive
 open side

+netdev (add missing cc)

On Wed, Jun 8, 2011 at 9:30 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
>
> Le mardi 07 juin 2011 à 22:00 -0700, H.K. Jerry Chu a écrit :
> > From: Jerry Chu <hkchu@...gle.com>
> >
> > This patch lowers the default initRTO from 3secs to 1sec per
> > RFC2988bis. It falls back to 3secs if the SYN or SYN-ACK packet
> > has been retransmitted, AND the TCP timestamp option is not on.
> >
> > It also adds support to take RTT sample during 3WHS on the passive
> > open side, just like its active open counterpart, and uses it, if
> > valid, to seed the initRTO for the data transmission phase.
> >
> > The patch also resets ssthresh to its initial default at the
> > beginning of the data transmission phase, and reduces cwnd to 1 if
> > there has been MORE THAN ONE retransmission during 3WHS per RFC5681.
> >
> > Signed-off-by: H.K. Jerry Chu <hkchu@...gle.com>
> > ---
>
> ...
>
> > @@ -1854,7 +1859,7 @@ static int tcp_v4_init_sock(struct sock *sk)
> >        * algorithms that we must have the following bandaid to talk
> >        * efficiently to them.  -DaveM
> >        */
> > -     tp->snd_cwnd = 2;
> > +     tp->snd_cwnd = TCP_INIT_CWND;
>
> Hmm, are you sure this belongs to this patch ?
>
> >
> >       /* See draft-stevens-tcpca-spec-01 for discussion of the
> >        * initialization of these values.
> > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> > index 80b1f80..d2fe4e0 100644
> > --- a/net/ipv4/tcp_minisocks.c
> > +++ b/net/ipv4/tcp_minisocks.c
> > @@ -486,7 +486,7 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
> >                * algorithms that we must have the following bandaid to talk
> >                * efficiently to them.  -DaveM
> >                */
> > -             newtp->snd_cwnd = 2;
> > +             newtp->snd_cwnd = TCP_INIT_CWND;
>
> same here ?

It's part of the cwnd initialization stuff, which I included some fix
per RFC5681 (in
tcp_init_metrics()). The field is currently not being used anyway
until data-in-SYN is
supported. I fixed it here just because TCP_INIT_CWND is the right value.

>
> >               newtp->snd_cwnd_cnt = 0;
> >               newtp->bytes_acked = 0;
> >
> > @@ -720,6 +720,10 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
> >               NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPDEFERACCEPTDROP);
> >               return NULL;
> >       }
> > +     if (tmp_opt.saw_tstamp && tmp_opt.rcv_tsecr)
> > +             tcp_rsk(req)->snt_synack = tmp_opt.rcv_tsecr;
> > +     else if (req->retrans) /* don't take RTT sample if retrans && ~TS */
> > +             tcp_rsk(req)->snt_synack = 0;
>
> Could you clarify why this is done in this order ?

What else is more appropriate? It needs to decide if we've got a valid
RTT sample.
If TS is on and rcv_tsec != 0 then we've got a valid RTT sample. Otherwise it
does not take sample if there has been retrans per Karn's algorithm.

Thanks,

Jerry

>
> Thanks !
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html