netdev - Re: [PATCH net-next] tcp: Fix CWV being too strict on thin streams

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

Date:	Tue, 22 Sep 2015 14:02:25 -0400
From:	Neal Cardwell <ncardwell@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Bendik Rønning Opstad <bro.devel@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	James Morris <jmorris@...ei.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
	Patrick McHardy <kaber@...sh.net>,
	Netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Andreas Petlund <apetlund@...ula.no>,
	Carsten Griwodz <griff@...ula.no>,
	Jonas Markussen <jonassm@....uio.no>,
	Kenneth Klette Jonassen <kennetkl@....uio.no>,
	Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH net-next] tcp: Fix CWV being too strict on thin streams

> I'll describe two example scenarios in detail. In both scenarios we are in
> congestion avoidance after experiencing loss. Nagle is disabled.

Thanks for the detailed follow-up! And thanks, Eric, for the packetdrill
script!

This looks like an issue of how to deal with the case when we run out of
packets to send and cwnd at exactly the same moment, and whether we consider
that case cwnd-limited.

Previously we did not consider ourselves cwnd-limited in that case, but I think
your "scenario 1", and Eric's packetdrill case, show convincingly that we ought
to consider ourselves cwnd-limited in that case.

I would suggest we try fixing this by making sure that in scenario 1, at the
moment when we fill the cwnd (packets_in_flight == cwnd == 2) we mark ourselves
as cwnd-limited.

More generally, my sense is that we should tweak the is_cwnd_limited code to
shift from saying "set is_cwnd_limited to true iff the cwnd is known to be
limiting transmits" to saying "set is_cwnd_limited to true iff the packets in
flight fill the cwnd".

This is partly justified by the kind of case presented here. Plus, in RFC 2861,
section 3.1, which this code is helping to implement, it says:

   After TCP sends a packet, it also checks to see if that packet filled
   the congestion window.  If so, the sender is network-limited, and
   sets the variable T_prev to the current TCP clock time,

So at the point in Scenario 1 when we send the second packet, and
  packets_in_flight == cwnd == 2
then IMHO we should set
  tp->is_cwnd_limited = true
and thus
  tp->snd_cwnd_stamp = tcp_time_stamp;

So maybe we want to slightly tweak the way tcp_write_xmit() and
tcp_tso_should_defer() treat this boundary condition? Gotta run to a
meeting....

neal

ps: If the tp->is_cwnd_limited and tcp_cwnd_validate() code were only used to
decide whether to increase cwnd, then I think your fix would probably be
sufficient. But it is also used for the code to implement RFC 2861 ("TCP
Congestion Window Validation"). So I don't think just changing
tp->is_cwnd_limited is sufficient.
--
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