netdev - Re: [PATCH v2 bpf-next 10/11] bpf: Add BPF_SOCK_OPS_STATE

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: <20171226232357.52ayp76dfwzhg62c@ast-mbp>
Date:   Tue, 26 Dec 2017 15:23:59 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Lawrence Brakmo <brakmo@...com>
Cc:     netdev <netdev@...r.kernel.org>, Kernel Team <kernel-team@...com>,
        Blake Matheny <bmatheny@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 bpf-next 10/11] bpf: Add BPF_SOCK_OPS_STATE_CB

On Thu, Dec 21, 2017 at 05:21:00PM -0800, Lawrence Brakmo wrote:
> Adds support for calling sock_ops BPF program when there is a TCP state
> change. Two arguments are used; one for the old state and another for
> the new state.
> 
> New op: BPF_SOCK_OPS_STATE_CB.
> 
> Signed-off-by: Lawrence Brakmo <brakmo@...com>
> ---
>  include/uapi/linux/bpf.h | 4 ++++
>  include/uapi/linux/tcp.h | 1 +
>  net/ipv4/tcp.c           | 2 ++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 415b951..14040e0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1024,6 +1024,10 @@ enum {
>  					 * Arg1: sequence number of 1st byte
>  					 * Arg2: # segments
>  					 */
> +	BPF_SOCK_OPS_STATE_CB,		/* Called when TCP changes state.
> +					 * Arg1: old_state
> +					 * Arg2: new_state
> +					 */

exposing tcp state to tcp-bpf program means that internal enum
in include/net/tcp_states.h
enum {
        TCP_ESTABLISHED = 1,
        TCP_SYN_SENT,
        TCP_SYN_RECV,
        TCP_FIN_WAIT1,
        TCP_FIN_WAIT2,
...
becomes uapi.
Also since it's only exposed from tcp side, the same sk_state
field used by other protocols is not exposed and things like
DCCP_PASSIVE_CLOSEREQ stay kernel internal.
I think it's ok.
If we would need to add new tcp state we can always add
it to the end of the enum.
Also it's better to make this explicit and move this enum to
uapi/linux/tcp_states.h or ...

An alternative would be to have an array that does state
remapping just to be consumed by bpf program which is ugly
and slow.

Another alternative would be to add:
enum {
        BPF_TCP_ESTABLISHED = 1,
        BPF_TCP_SYN_SENT,
        BPF_TCP_SYN_RECV,
        BPF_TCP_FIN_WAIT1,
to uapi/linux/bpf.h
and have BUILD_BUG_ON(BPF_TCP_ESTABLISHED != TCP_ESTABLISHED);
and in case somebody touches that enum in the middle
the issue will be caught.

I'd like to hear Dave and Eric opinion here.