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 <bra...@fb.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.
