netdev - Re: [RFC PATCH net-next v2 01/15] bpf: BPF support for socket ops

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: <C2BE07F9-90D9-4985-9332-AB3D257C77CF@fb.com>
Date:   Mon, 19 Jun 2017 20:49:24 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        netdev <netdev@...r.kernel.org>
CC:     Kernel Team <Kernel-team@...com>, Blake Matheny <bmatheny@...com>,
        "Alexei Starovoitov" <ast@...com>,
        David Ahern <dsa@...ulusnetworks.com>
Subject: Re: [RFC PATCH net-next v2 01/15] bpf: BPF support for socket ops


On 6/19/17, 11:44 AM, "Daniel Borkmann" <daniel@...earbox.net> wrote:

    On 06/17/2017 01:41 AM, Lawrence Brakmo wrote:
    > On 6/16/17, 5:07 AM, "Daniel Borkmann" <daniel@...earbox.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.

As discussed in the other thread, I will keep the 2 structs
    
    >      > 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.

Done.
    
    >      > +	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.

I made lots of changes to how we set/attach the global_sock_ops program
affecting the files kernel/bpf/syscall.c, net/core/sock_bpfops.c and
samples/bpf/tcp_bpf.c. The patch set will be submitted later today.
    
    >      > +		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().
    [...]
    
Thanks again for the feedback.