netdev - Re: [PATCH bpf-next v5 05/11] bpf: Adds field bpf_sock_ops_flags to tcp

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DCE23041-18AF-402A-B075-B6A641AC108A@fb.com>
Date:   Wed, 10 Jan 2018 00:31:38 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        netdev <netdev@...r.kernel.org>
CC:     Kernel Team <Kernel-team@...com>, Blake Matheny <bmatheny@...com>,
        "Alexei Starovoitov" <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Neal Cardwell" <ncardwell@...gle.com>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH bpf-next v5 05/11] bpf: Adds field bpf_sock_ops_flags to
 tcp_sock

On 1/9/18, 3:31 PM, "Eric Dumazet" <eric.dumazet@...il.com> wrote:

    On Tue, 2018-01-09 at 13:06 -0800, Lawrence Brakmo wrote:
    > Adds field bpf_sock_ops_flags to tcp_sock and bpf_sock_ops. Its primary
    > use is to determine if there should be calls to sock_ops bpf program at
    > various points in the TCP code. The field is initialized to zero,
    > disabling the calls. A sock_ops BPF program can set, per connection and
    > as necessary, when the connection is established.
    > 
    > It also adds support for reading and writting the field within a
    > sock_ops BPF program.
    > 
    > Examples of where to call the bpf program:
    > 
    > 1) When RTO fires
    > 2) When a packet is retransmitted
    > 3) When the connection terminates
    > 4) When a packet is sent
    > 5) When a packet is received
    > 
    > Signed-off-by: Lawrence Brakmo <brakmo@...com>
    > ---
    >  include/linux/tcp.h      | 8 ++++++++
    >  include/uapi/linux/bpf.h | 1 +
    >  net/core/filter.c        | 7 +++++++
    >  3 files changed, 16 insertions(+)
    > 
    > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
    > index 4f93f095..62f4388 100644
    > --- a/include/linux/tcp.h
    > +++ b/include/linux/tcp.h
    > @@ -373,6 +373,14 @@ struct tcp_sock {
    >  	 */
    >  	struct request_sock *fastopen_rsk;
    >  	u32	*saved_syn;
    > +
    > +/* Sock_ops bpf program related variables */
    > +#ifdef CONFIG_BPF
    > +	u32     bpf_sock_ops_flags;     /* values defined in uapi/linux/tcp.h */
    > +#define BPF_SOCK_OPS_TEST_FLAG(TP, ARG) (TP->bpf_sock_ops_flags & ARG)
    > +#else
    > +#define BPF_SOCK_OPS_TEST_FLAG(TP, ARG) 0
    > +#endif
    >  };
    > 
    
    It looks like we add yet another TCP socket field for some feature that
     is only used by you or FB :/
    
I am certainly hoping it will be used by others as well, but it is not possible
until it is available (many people expressed interest at Netdev 2.2). 

    At least please try to fill a hole (on 64bit kernels), instead of
    adding one more.

Makes sense.
    
    Also, should not we reject attempts to use bits that are not yet
    supported by the kernel ?
    
    If a BPF filter expects to be called for every retransmit, but kernel
    is too old, this wont work.
    
Thanks, good point. I will add a helper function that returns which
callbacks are supported by the current kernel so the bpf program can
take appropriate action.

Eric, thank you for the feedback.