On Fri, 2018-01-19 at 19:52 -0800, Alexei Starovoitov wrote: > On Fri, Jan 19, 2018 at 05:45:42PM -0800, Lawrence Brakmo wrote: > > Adds field bpf_sock_ops_cb_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 it, 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. Reading is done by accessing the field directly. > > However, writing is done through the helper function > > bpf_sock_ops_cb_flags_set, in order to return an error if a BPF program > > is trying to set a callback that is not supported in the current kernel > > (i.e. running an older kernel). The helper function returns 0 if it was > > able to set all of the bits set in the argument, a positive number > > containing the bits that could not be set, or -EINVAL if the socket is > > not a full TCP socket. > > ... > > +/* Sock_ops bpf program related variables */ > > +#ifdef CONFIG_BPF > > + u8 bpf_sock_ops_cb_flags; /* Control calling BPF programs > > + * values defined in uapi/linux/tcp.h > > I guess we can extend u8 into u16 or more if necessary in the future. > > > + * int bpf_sock_ops_cb_flags_set(bpf_sock_ops, flags) > > + * Set callback flags for sock_ops > > + * @bpf_sock_ops: pointer to bpf_sock_ops_kern struct > > + * @flags: flags value > > + * Return: 0 for no error > > + * -EINVAL if there is no full tcp socket > > + * bits in flags that are not supported by current kernel > > ... > > +BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock, > > + int, argval) > > +{ > > + struct sock *sk = bpf_sock->sk; > > + int val = argval & BPF_SOCK_OPS_ALL_CB_FLAGS; > > + > > + if (!sk_fullsock(sk)) > > + return -EINVAL; > > + > > +#ifdef CONFIG_INET > > + if (val) > > + tcp_sk(sk)->bpf_sock_ops_cb_flags = val; > > + > > + return argval & (~BPF_SOCK_OPS_ALL_CB_FLAGS); > > interesting idea! took me some time to realize the potential > of such semantics, but now I like it a lot. > It blends 'set good flag' with 'which flags are supported' logic > into single helper. Nice. > Thanks for adding a test for both ways. > Acked-by: Alexei Starovoitov <a...@kernel.org> > > Eric, does this approach address your concerns?
Yes, this seems fine, thanks.