| Message ID | 20210811024056.235161-1-ncardwell@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6de035fec045f8ae5ee5f3a02373a18b939e91fb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tcp_bbr: fix u32 wrap bug in round logic if bbr_init() called after 2B packets | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 4 blamed authors not CCed: soheil@google.com vanj@google.com edumazet@google.com nanditad@google.com; 7 maintainers not CCed: vanj@google.com dsahern@kernel.org soheil@google.com edumazet@google.com kuba@kernel.org nanditad@google.com yoshfuji@linux-ipv6.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 8/11/21 4:40 AM, Neal Cardwell wrote: > Currently if BBR congestion control is initialized after more than 2B > packets have been delivered, depending on the phase of the > tp->delivered counter the tracking of BBR round trips can get stuck. > > The bug arises because if tp->delivered is between 2^31 and 2^32 at > the time the BBR congestion control module is initialized, then the > initialization of bbr->next_rtt_delivered to 0 will cause the logic to > believe that the end of the round trip is still billions of packets in > the future. More specifically, the following check will fail > repeatedly: > > !before(rs->prior_delivered, bbr->next_rtt_delivered) > > and thus the connection will take up to 2B packets delivered before > that check will pass and the connection will set: > > bbr->round_start = 1; > > This could cause many mechanisms in BBR to fail to trigger, for > example bbr_check_full_bw_reached() would likely never exit STARTUP. > > This bug is 5 years old and has not been observed, and as a practical > matter this would likely rarely trigger, since it would require > transferring at least 2B packets, or likely more than 3 terabytes of > data, before switching congestion control algorithms to BBR. > > This patch is a stable candidate for kernels as far back as v4.9, > when tcp_bbr.c was added. > > Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control") > Signed-off-by: Neal Cardwell <ncardwell@google.com> > Reviewed-by: Yuchung Cheng <ycheng@google.com> > Reviewed-by: Kevin Yang <yyd@google.com> Nice catch :( Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Tue, 10 Aug 2021 22:40:56 -0400 you wrote: > Currently if BBR congestion control is initialized after more than 2B > packets have been delivered, depending on the phase of the > tp->delivered counter the tracking of BBR round trips can get stuck. > > The bug arises because if tp->delivered is between 2^31 and 2^32 at > the time the BBR congestion control module is initialized, then the > initialization of bbr->next_rtt_delivered to 0 will cause the logic to > believe that the end of the round trip is still billions of packets in > the future. More specifically, the following check will fail > repeatedly: > > [...] Here is the summary with links: - [net] tcp_bbr: fix u32 wrap bug in round logic if bbr_init() called after 2B packets https://git.kernel.org/netdev/net/c/6de035fec045 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c index 6ea3dc2e4219..6274462b86b4 100644 --- a/net/ipv4/tcp_bbr.c +++ b/net/ipv4/tcp_bbr.c @@ -1041,7 +1041,7 @@ static void bbr_init(struct sock *sk) bbr->prior_cwnd = 0; tp->snd_ssthresh = TCP_INFINITE_SSTHRESH; bbr->rtt_cnt = 0; - bbr->next_rtt_delivered = 0; + bbr->next_rtt_delivered = tp->delivered; bbr->prev_ca_state = TCP_CA_Open; bbr->packet_conservation = 0;
Currently if BBR congestion control is initialized after more than 2B packets have been delivered, depending on the phase of the tp->delivered counter the tracking of BBR round trips can get stuck. The bug arises because if tp->delivered is between 2^31 and 2^32 at the time the BBR congestion control module is initialized, then the initialization of bbr->next_rtt_delivered to 0 will cause the logic to believe that the end of the round trip is still billions of packets in the future. More specifically, the following check will fail repeatedly: !before(rs->prior_delivered, bbr->next_rtt_delivered) and thus the connection will take up to 2B packets delivered before that check will pass and the connection will set: bbr->round_start = 1; This could cause many mechanisms in BBR to fail to trigger, for example bbr_check_full_bw_reached() would likely never exit STARTUP. This bug is 5 years old and has not been observed, and as a practical matter this would likely rarely trigger, since it would require transferring at least 2B packets, or likely more than 3 terabytes of data, before switching congestion control algorithms to BBR. This patch is a stable candidate for kernels as far back as v4.9, when tcp_bbr.c was added. Fixes: 0f8782ea1497 ("tcp_bbr: add BBR congestion control") Signed-off-by: Neal Cardwell <ncardwell@google.com> Reviewed-by: Yuchung Cheng <ycheng@google.com> Reviewed-by: Kevin Yang <yyd@google.com> --- net/ipv4/tcp_bbr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)