Date:   Thu, 15 Mar 2018 00:27:45 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net,
        netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH RFC bpf-next 1/6] bpf: Hooks for sys_bind

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.

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.

[...]
>>> +struct bpf_sock_addr {
>>> +	__u32 user_family;	/* Allows 4-byte read, but no write. */
>>> +	__u32 user_ip4;		/* Allows 1,2,4-byte read and 4-byte write.
>>> +				 * Stored in network byte order.
>>> +				 */
>>> +	__u32 user_ip6[4];	/* Allows 1,2,4-byte read an 4-byte write.
>>> +				 * Stored in network byte order.
>>> +				 */
>>> +	__u32 user_port;	/* Allows 4-byte read and write.
>>> +				 * Stored in network byte order
>>> +				 */
>>> +	__u32 family;		/* Allows 4-byte read, but no write */
>>> +	__u32 type;		/* Allows 4-byte read, but no write */
>>> +	__u32 protocol;		/* Allows 4-byte read, but no write */
>>
>> I recall bind to prefix came up from time to time in BPF context in the sense
>> to let the app itself be more flexible to bind to BPF prog. Do you see also app
>> to be able to add a BPF prog into the array itself?
> 
> I'm not following. In this case the container management framework
> will attach bpf progs to cgroups and apps inside the cgroups will
> get their bind/connects overwritten when necessary.

Was mostly just thinking whether it could also cover the use case that was
brought up from time to time e.g.:

  https://www.mail-archive.com/netdev@vger.kernel.org/msg100914.html

Seems like it would potentially be on top of that, plus having an option to
attach from within the app instead of orchestrator.

Powered by blists - more mailing lists