On 6/16/17, 5:07 AM, "Daniel Borkmann" <dan...@iogearbox.net> wrote:
On 06/15/2017 10:08 PM, Lawrence Brakmo wrote:
> Two new corresponding structs (one for the kernel one for the user/BPF
> program):
>
> /* kernel version */
> struct bpf_socket_ops_kern {
> struct sock *sk;
> __u32 is_req_sock:1;
> __u32 op;
> union {
> __u32 reply;
> __u32 replylong[4];
> };
> };
>
> /* user version */
> struct bpf_socket_ops {
> __u32 op;
> union {
> __u32 reply;
> __u32 replylong[4];
> };
> __u32 family;
> __u32 remote_ip4;
> __u32 local_ip4;
> __u32 remote_ip6[4];
> __u32 local_ip6[4];
> __u32 remote_port;
> __u32 local_port;
> };
Above and ...
struct bpf_sock {
__u32 bound_dev_if;
__u32 family;
__u32 type;
__u32 protocol;
};
... would result in two BPF sock user versions. It's okayish, but
given struct bpf_sock is quite generic, couldn't we merge the members
from struct bpf_socket_ops into struct bpf_sock instead?
Idea would be that sock_filter_is_valid_access() for cgroups would
then check off < 0 || off + size > offsetofend(struct bpf_sock, protocol)
to disallow new members, and your socket_ops_is_valid_access() could
allow and xlate the full range. The family member is already duplicate
and the others could then be accessed from these kind of BPF progs as
well, plus we have a single user representation similar as with __sk_buff
that multiple types will use.
I am concerned that it could make usage more confusing. One type of
sock program (cgroup) could only use a subset of the fields while the
other type (socket_ops) could use all (or a different subset). Then what
happens if there is a need to add a new field to cgroup type sock program?
In addition, in the near future I will have a patch to attach socket_ops
programs to cgroups.
I rather leave it as it is.
