From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH net] bpf: always re-init the congestion control after switching to it Date: Tue, 23 Jan 2018 11:50:04 -0800 Message-ID: <1516737004.3715.8.camel@gmail.com> References: <20180123173009.205669-1-ycheng@google.com> <30304D2B-B533-423A-B78A-19A1A6949194@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: Yuchung Cheng , "davem@davemloft.net" , "netdev@vger.kernel.org" , "edumazet@google.com" , "soheil@google.com" To: Neal Cardwell , Lawrence Brakmo Return-path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:41579 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751964AbeAWTuG (ORCPT ); Tue, 23 Jan 2018 14:50:06 -0500 Received: by mail-pf0-f195.google.com with SMTP id c6so1141462pfi.8 for ; Tue, 23 Jan 2018 11:50:06 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, 2018-01-23 at 14:39 -0500, Neal Cardwell wrote: > On Tue, Jan 23, 2018 at 2:20 PM, Lawrence Brakmo wrote: > > On 1/23/18, 9:30 AM, "Yuchung Cheng" wrote: > > > > The original patch that changes TCP's congestion control via eBPF only > > re-initializes the new congestion control, if the BPF op is set to an > > (invalid) value beyond BPF_SOCK_OPS_NEEDS_ECN. Consequently TCP will > > > > What do you mean by “(invalid) value”? > > > > run the new congestion control from random states. > > > > This has always been possible with setsockopt, no? > > > > This patch fixes > > the issue by always re-init the congestion control like other means > > such as setsockopt and sysctl changes. > > > > The current code re-inits the congestion control when calling > > tcp_set_congestion_control except when it is called early on (i.e. op <= > > BPF_SOCK_OPS_NEEDS_ECN). In that case there is no need to re-initialize > > since it will be initialized later by TCP when the connection is established. > > > > Otherwise, if we always call tcp_reinit_congestion_control we would call > > tcp_cleanup_congestion_control when the congestion control has not been > > initialized yet. > > On Tue, Jan 23, 2018 at 2:20 PM, Lawrence Brakmo wrote: > > On 1/23/18, 9:30 AM, "Yuchung Cheng" wrote: > > > > The original patch that changes TCP's congestion control via eBPF only > > re-initializes the new congestion control, if the BPF op is set to an > > (invalid) value beyond BPF_SOCK_OPS_NEEDS_ECN. Consequently TCP will > > > > What do you mean by “(invalid) value”? > > > > run the new congestion control from random states. > > > > This has always been possible with setsockopt, no? > > > > This patch fixes > > the issue by always re-init the congestion control like other means > > such as setsockopt and sysctl changes. > > > > The current code re-inits the congestion control when calling > > tcp_set_congestion_control except when it is called early on (i.e. op <= > > BPF_SOCK_OPS_NEEDS_ECN). In that case there is no need to re-initialize > > since it will be initialized later by TCP when the connection is established. > > > > Otherwise, if we always call tcp_reinit_congestion_control we would call > > tcp_cleanup_congestion_control when the congestion control has not been > > initialized yet. > > Interesting. So I wonder if the symptoms we were seeing were due to > kernels that did not yet have this fix: > > 27204aaa9dc6 ("tcp: uniform the set up of sockets after successful > connection): > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=27204aaa9dc67b833b77179fdac556288ec3a4bf > > Before that fix, there could be TFO passive connections that at SYN time called: > tcp_init_congestion_control(child); > and then: > tcp_call_bpf(child, BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB); > > So that if the CC was switched in the > BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB handler then there would be no > init for the new module? Note that bpf_sock->op can be written by a malicious BPF filter. So, a malicious filter can switch from Cubic to BBR without re-init, and bad things can happen. I do not believe we should trust BPF here.