netdev - Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives

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]

Message-ID: <CADVnQy=3s4AAX-SyHaRURPgA=mN=m4DBP=Jz3HGt5pBPbiRaaA@mail.gmail.com>
Date:   Mon, 2 Jul 2018 10:57:03 -0400
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Lawrence Brakmo <brakmo@...com>
Cc:     Netdev <netdev@...r.kernel.org>, Kernel Team <Kernel-team@...com>,
        bmatheny@...com, ast@...com, Yuchung Cheng <ycheng@...gle.com>,
        Steve Ibanez <sibanez@...nford.edu>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives

On Sat, Jun 30, 2018 at 9:47 PM Lawrence Brakmo <brakmo@...com> wrote:
> I see two issues, one is that entering quickack mode as you
> mentioned does not insure that it will still be on when the CWR
> arrives. The second issue is that the problem occurs right after the
> receiver sends a small reply which results in entering pingpong mode
> right before the sender starts the new request by sending just one
> packet (i.e. forces delayed ack).
>
> I compiled and tested your patch. Both 99 and 99.9 percentile
> latencies are around 40ms. Looking at the packet traces shows that
> some CWR marked packets are not being ack immediately (delayed by
> 40ms).

Thanks, Larry! So your tests provide nice, specific evidence that it
is good to force an immediate ACK when a receiver receives a packet
with CWR marked. Given that, I am wondering what the simplest way is
to achieve that goal.

What if, rather than plumbing a new specific signal into
__tcp_ack_snd_check(), we use the existing general quick-ack
mechanism, where various parts of the TCP stack (like
__tcp_ecn_check_ce())  are already using the quick-ack mechanism to
"remotely" signal to __tcp_ack_snd_check() that they want an immediate
ACK.

For example, would it work to do something like:

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c53ae5fc834a5..8168d1938b376 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -262,6 +262,12 @@ static void __tcp_ecn_check_ce(struct sock *sk,
const struct sk_buff *skb)
 {
        struct tcp_sock *tp = tcp_sk(sk);

+       /* If the sender is telling us it has entered CWR, then its cwnd may be
+        * very low (even just 1 packet), so we should ACK immediately.
+        */
+       if (tcp_hdr(skb)->cwr)
+               tcp_enter_quickack_mode(sk, 2);
+
        switch (TCP_SKB_CB(skb)->ip_dsfield & INET_ECN_MASK) {
        case INET_ECN_NOT_ECT:
                /* Insure that GCN will not continue to mark packets. */

And then since that broadens the mission of this function beyond
checking just the ECT/CE bits, I supposed we could rename the
__tcp_ecn_check_ce() and tcp_ecn_check_ce() functions to
__tcp_ecn_check() and tcp_ecn_check(), or something like that.

Would that work for this particular issue?

neal