netdev - Re: [PATCH v2 net-next 2/4] tcp: take care of compressed acks in tcp_add_reno

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: <CADVnQynMwYtvQcwVuYRJom1GVgBg-xLZNoHUC8oOxmd27=fQDA@mail.gmail.com>
Date:   Tue, 27 Nov 2018 16:19:48 -0500
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>, jean-louis@...ond.be,
        Yuchung Cheng <ycheng@...gle.com>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH v2 net-next 2/4] tcp: take care of compressed acks in tcp_add_reno_sack()

On Tue, Nov 27, 2018 at 10:57 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> Neal pointed out that non sack flows might suffer from ACK compression
> added in the following patch ("tcp: implement coalescing on backlog queue")
>
> Instead of tweaking tcp_add_backlog() we can take into
> account how many ACK were coalesced, this information
> will be available in skb_shinfo(skb)->gso_segs
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
...
> @@ -2679,8 +2683,8 @@ static void tcp_process_loss(struct sock *sk, int flag, bool is_dupack,
>                 /* A Reno DUPACK means new data in F-RTO step 2.b above are
>                  * delivered. Lower inflight to clock out (re)tranmissions.
>                  */
> -               if (after(tp->snd_nxt, tp->high_seq) && is_dupack)
> -                       tcp_add_reno_sack(sk);
> +               if (after(tp->snd_nxt, tp->high_seq))
> +                       tcp_add_reno_sack(sk, num_dupack);
>                 else if (flag & FLAG_SND_UNA_ADVANCED)
>                         tcp_reset_reno_sack(tp);
>         }

I think this probably should be checking num_dupack, something like:

+               if (after(tp->snd_nxt, tp->high_seq) && num_dupack)
+                       tcp_add_reno_sack(sk, num_dupack);

If we don't check num_dupack, that seems to mean that after FRTO sends
the two new data packets (making snd_nxt after high_seq), the patch
would have a particular non-SACK FRTO loss recovery always go into the
"if" branch where we tcp_add_reno_sack() function, and we would never
have a chance to get to the "else" branch where we check if
FLAG_SND_UNA_ADVANCED and zero out the reno SACKs.

Otherwise the patch looks great to me. Thanks for doing this!

neal