netdev - Re: tcp: multiple ssthresh reductions before all packets are retransmitted

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-prev] [day] [month] [year] [list]

Message-ID: <20140616184823.GA20932@unicorn.suse.cz>
Date:	Mon, 16 Jun 2014 20:48:23 +0200
From:	Michal Kubecek <mkubecek@...e.cz>
To:	Yuchung Cheng <ycheng@...gle.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: tcp: multiple ssthresh reductions before all packets are
 retransmitted

On Mon, Jun 16, 2014 at 10:02:08AM -0700, Yuchung Cheng wrote:
> On Mon, Jun 16, 2014 at 8:35 AM, Michal Kubecek <mkubecek@...e.cz> wrote:
> >
> > RFC 5681 says that ssthresh reduction in response to RTO should be done
> > only once and should not be repeated until all packets from the first
> > loss are retransmitted. RFC 6582 (as well as its predecessor RFC 3782)
> > is even more specific and says that when loss is detected, one should
> > mark current SND.NXT and ssthresh shouldn't be reduced again due to
> > a loss until SND.UNA reaches this remembered value.
> >
> > Linux implementation does exactly that but in TCP_CA_Loss state,
> > tcp_enter_loss() also takes icsk_retransmits into account:
> >
> >         /* Reduce ssthresh if it has not yet been made inside this window. */
> >         if (icsk->icsk_ca_state <= TCP_CA_Disorder ||
> >             !after(tp->high_seq, tp->snd_una) ||
> >             (icsk->icsk_ca_state == TCP_CA_Loss && !icsk->icsk_retransmits)) {
> >                 new_recovery = true;
> >                 tp->prior_ssthresh = tcp_current_ssthresh(sk);
> >                 tp->snd_ssthresh = icsk->icsk_ca_ops->ssthresh(sk);
> >                 tcp_ca_event(sk, CA_EVENT_LOSS);
> >         }
> >
> > This seems correct as icsk_retransmits is supposed to mean "we still
> > have packets to retransmit". But icsk_retransmits is reset in
> > tcp_process_loss() if one of two conditions is satisfied:
> 
> icsk_retransmits indicates the number of recurring timeouts (of the
> same sequence). so it is reset when the recovery is done or SND_UNA is
> advanced. the variable name however is confusing.

In that case, I suppose the problem would be this part

    (icsk->icsk_ca_state == TCP_CA_Loss && !icsk->icsk_retransmits)

of the condition above (in tcp_enter_loss()). As that would mean we
would allow further ssthresh reduction as soon as SND.UNA moves forward
(not when it reaches high_seq).

> does your suggested change fix the problem you are observing?

It does, with it ssthresh isn't lowered again until all lost packets are
retransmitted (at which time, cwnd is already reasonably high). But
I wasn't sure it wouldn't break something else.

                                                          Michal Kubecek

--
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