netdev - Re: [PATCH net] bpf: always re-init the congestion control after switching to it

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: <CADVnQym8+Z_OXDk7a5tVH7a4HHkXLFQ8AnjJ0qxF6dmC8=tODA@mail.gmail.com>
Date:   Tue, 23 Jan 2018 14:39:21 -0500
From:   Neal Cardwell <ncardwell@...gle.com>
To:     Lawrence Brakmo <brakmo@...com>
Cc:     Yuchung Cheng <ycheng@...gle.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "soheil@...gle.com" <soheil@...gle.com>
Subject: Re: [PATCH net] bpf: always re-init the congestion control after
 switching to it

On Tue, Jan 23, 2018 at 2:20 PM, Lawrence Brakmo <brakmo@...com> wrote:
> On 1/23/18, 9:30 AM, "Yuchung Cheng" <ycheng@...gle.com> 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 <brakmo@...com> wrote:
> On 1/23/18, 9:30 AM, "Yuchung Cheng" <ycheng@...gle.com> 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?

neal