On 3/14/18 4:27 PM, Daniel Borkmann wrote:
On 03/14/2018 07:11 PM, Alexei Starovoitov wrote:
On Wed, Mar 14, 2018 at 03:37:01PM +0100, Daniel Borkmann wrote:
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -133,6 +133,8 @@ enum bpf_prog_type {
BPF_PROG_TYPE_SOCK_OPS,
BPF_PROG_TYPE_SK_SKB,
BPF_PROG_TYPE_CGROUP_DEVICE,
+ BPF_PROG_TYPE_CGROUP_INET4_BIND,
+ BPF_PROG_TYPE_CGROUP_INET6_BIND,
Could those all be merged into BPF_PROG_TYPE_SOCK_OPS? I'm slowly getting
confused by the many sock_*/sk_* prog types we have. The attach hook could
still be something like BPF_CGROUP_BIND/BPF_CGROUP_CONNECT. Potentially
storing some prog-type specific void *private_data in prog's aux during
verification could be a way (similarly as you mention) which can later be
retrieved at attach time to reject with -ENOTSUPP or such.
that's exacly what I mentioned in the cover letter,
but we need to extend attach cmd with verifier-like log_buf+log_size.
since simple enotsupp will be impossible to debug.
Hmm, adding verifier-like log_buf + log_size feels a bit of a kludge just
for this case, but it's the usual problem where getting a std error code
is like looking into a crystal ball to figure where it's coming from. I'd see
only couple of other alternatives: distinct error code like ENAVAIL for such
mismatch, or telling the verifier upfront where this is going to be attached
to - same as we do with the dev for offloading or as you did with splitting
the prog types or some sort of notion of sub-prog types; or having a query
API that returns possible/compatible attach types for this loaded prog via
e.g. bpf_prog_get_info_by_fd() that loader can precheck or check when error
occurs. All nothing really nice, though.
query after loading isn't great, since possible attach combinations
will be too high for human to understand,
but I like "passing attach_type into prog_load" idea.
That should work and it fits existing prog_ifindex too.
So we'll add '__u32 attach_type' to prog_load cmd.
elf loader would still need to parse section name to
figure out prog type and attach type.
Something like:
SEC("sock_addr/bind_v4") my_prog(struct bpf_sock_addr *ctx)
SEC("sock_addr/connect_v6") my_prog(struct bpf_sock_addr *ctx)
We still need new prog type for bind_v4/bind_v6/connect_v4/connect_v6
hooks with distinct 'struct bpf_sock_addr' context,
since the prog is accessing both sockaddr and sock.
Adding user_ip4, user_ip6 fields to 'struct bpf_sock_ops'
is doable, but it would be too confusing to users, so imo that's
not a good option.
For post-bind hook we probably can reuse 'struct bpf_sock_ops'
and BPF_PROG_TYPE_SOCK_OPS, since there only sock is the context.
Making verifier-like log_buf + log_size generic meaning not just for the case
of BPF_PROG_ATTACH specifically might be yet another option, meaning you'd
have a new BPF_GET_ERROR command to pick up the log for the last failed bpf(2)
cmd, but either that might require adding a BPF related pointer to task struct
for this or any other future BPF feature (maybe not really an option); or to
have some sort of bpf cmd to config and obtain an fd for error queue/log once,
where this can then be retrieved from and used for all cmds generically.
I don't think we want to hold on to error logs in the kernel,
since user may not query it right away or ever.
verifier log is freed right after prog_load cmd is done.
Seems like it would potentially be on top of that, plus having an option to
attach from within the app instead of orchestrator.
right. let's worry about it as potential next step.
