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: <967f7674-a0b2-8e25-8eea-d823d2c3353c@gmail.com>
Date:   Tue, 27 Nov 2018 13:54:31 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Neal Cardwell <ncardwell@...gle.com>,
        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 11/27/2018 01:19 PM, Neal Cardwell wrote:
> 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!
>

Oh right, I missed the else clause, I thought that tcp_add_reno_sack()
checking the num_dupack was enough.

Thanks.