netdev - Re: [PATCH net] tcp: only postpone PROBE_RTT if RTT is < current min

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:   Mon, 16 Nov 2020 12:58:57 -0500
From:   Soheil Hassas Yeganeh <soheil@...gle.com>
To:     Ryan Sharpelletti <sharpelletti.kdev@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Ryan Sharpelletti <sharpelletti@...gle.com>,
        Neal Cardwell <ncardwell@...gle.com>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH net] tcp: only postpone PROBE_RTT if RTT is < current
 min_rtt estimate

LGTM. Thanks!

On Mon, Nov 16, 2020 at 12:44 PM Ryan Sharpelletti
<sharpelletti.kdev@...il.com> wrote:
>
> From: Ryan Sharpelletti <sharpelletti@...gle.com>
>
> During loss recovery, retransmitted packets are forced to use TCP
> timestamps to calculate the RTT samples, which have a millisecond
> granularity. BBR is designed using a microsecond granularity. As a
> result, multiple RTT samples could be truncated to the same RTT value
> during loss recovery. This is problematic, as BBR will not enter
> PROBE_RTT if the RTT sample is <= the current min_rtt sample, meaning
> that if there are persistent losses, PROBE_RTT will constantly be
> pushed off and potentially never re-entered. This patch makes sure
> that BBR enters PROBE_RTT by checking if RTT sample is < the current
> min_rtt sample, rather than <=.
>
> The Netflix transport/TCP team discovered this bug in the Linux TCP
> BBR code during lab tests.
>
> Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control")
> Signed-off-by: Ryan Sharpelletti <sharpelletti@...gle.com>
> Signed-off-by: Neal Cardwell <ncardwell@...gle.com>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@...gle.com>
> Signed-off-by: Yuchung Cheng <ycheng@...gle.com>
> ---
>  net/ipv4/tcp_bbr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
> index 6c4d79baff26..6ea3dc2e4219 100644
> --- a/net/ipv4/tcp_bbr.c
> +++ b/net/ipv4/tcp_bbr.c
> @@ -945,7 +945,7 @@ static void bbr_update_min_rtt(struct sock *sk, const struct rate_sample *rs)
>         filter_expired = after(tcp_jiffies32,
>                                bbr->min_rtt_stamp + bbr_min_rtt_win_sec * HZ);
>         if (rs->rtt_us >= 0 &&
> -           (rs->rtt_us <= bbr->min_rtt_us ||
> +           (rs->rtt_us < bbr->min_rtt_us ||
>              (filter_expired && !rs->is_ack_delayed))) {
>                 bbr->min_rtt_us = rs->rtt_us;
>                 bbr->min_rtt_stamp = tcp_jiffies32;
> --
> 2.29.2.299.gdc1121823c-goog
>