On 06/17/2017 01:41 AM, Lawrence Brakmo wrote:
On 6/16/17, 5:07 AM, "Daniel Borkmann" <dan...@iogearbox.net> wrote:
[...]
I see. You are saying have one struct in common but still keep the two
PROG_TYPES? That makes sense. Do we really need two different
is_valid_access functions? Both types should be able to see all
the fields (otherwise adding new fields becomes messy).
Would probably leave the two is_valid_access() separate initially, and
once people ask for it we could potentially open this up to some of
the other fields that are available at that time.
> Currently there are two types of ops. The first type expects the BPF
> program to return a value which is then used by the caller (or a
> negative value to indicate the operation is not supported). The second
> type expects state changes to be done by the BPF program, for example
> through a setsockopt BPF helper function, and they ignore the return
> value.
[...]
> +/* Call BPF_SOCKET_OPS program that returns an int. If the return value
> + * is < 0, then the BPF op failed (for example if the loaded BPF
> + * program does not support the chosen operation or there is no BPF
> + * program loaded).
> + */
> +#ifdef CONFIG_BPF
> +static inline int tcp_call_bpf(struct sock *sk, bool is_req_sock, int
op)
> +{
> + struct bpf_socket_ops_kern socket_ops;
> +
> + memset(&socket_ops, 0, sizeof(socket_ops));
> + socket_ops.sk = sk;
> + socket_ops.is_req_sock = is_req_sock ? 1 : 0;
Is is_req_sock actually used here in this patch (apart from setting it)?
Not seeing that BPF prog will access it, if it also shouldn't access it,
then bool type would be better.
The only reason I used a bit was in case I wanted to add more fields later on.
Does it make sense or should I just use bool?
Didn't know that, but I think starting out with bool seems a bit
cleaner, if needed we could later still switch to bitfield.
> + socket_ops.op = op;
> +
> + return bpf_socket_ops_call(&socket_ops);
> +}
[...]
> +/* Global BPF program for sockets */
> +static struct bpf_prog *bpf_socket_ops_prog;
> +static DEFINE_RWLOCK(bpf_socket_ops_lock);
> +
> +int bpf_socket_ops_set_prog(int fd)
> +{
> + int err = 0;
> +
> + write_lock(&bpf_socket_ops_lock);
> + if (bpf_socket_ops_prog) {
> + bpf_prog_put(bpf_socket_ops_prog);
> + bpf_socket_ops_prog = NULL;
> + }
> +
> + /* fd of zero is used as a signal to remove the current
> + * bpf_socket_ops_prog.
> + */
> + if (fd == 0) {
Can we make the fd related semantics similar to dev_change_xdp_fd()?
Do you mean remove program is fd < 0 instead of == 0?
Yes, that and also the ordering of dropping the ref of the existing
bpf_socket_ops_prog program with setting the new one, so you can
convert bpf_socket_ops_prog to RCU more easily.
> + write_unlock(&bpf_socket_ops_lock);
> + return 1;
> + }
> +
> + bpf_socket_ops_prog = bpf_prog_get_type(fd,
BPF_PROG_TYPE_SOCKET_OPS);
> + if (IS_ERR(bpf_socket_ops_prog)) {
> + bpf_prog_put(bpf_socket_ops_prog);
This will crash the kernel, passing err value to bpf_prog_put().
[...]
