From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:34866 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751302AbeCOA3i (ORCPT ); Wed, 14 Mar 2018 20:29:38 -0400 Subject: Re: [PATCH RFC bpf-next 1/6] bpf: Hooks for sys_bind To: Daniel Borkmann , Alexei Starovoitov References: <20180314033934.3502167-1-ast@kernel.org> <20180314033934.3502167-2-ast@kernel.org> <043c6e66-aef1-51ad-177c-6e3925d4408a@iogearbox.net> <20180314181158.bsvgnffv3rurvdcx@ast-mbp> <54d95e56-02dc-b766-2221-9d5d6ce9985c@iogearbox.net> CC: Alexei Starovoitov , , , From: Alexei Starovoitov Message-ID: <136ee044-0cf7-a8d7-e6d2-3c4c362118e4@fb.com> Date: Wed, 14 Mar 2018 17:29:07 -0700 MIME-Version: 1.0 In-Reply-To: <54d95e56-02dc-b766-2221-9d5d6ce9985c@iogearbox.net> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: 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.