netdev - Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in BPF

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: <e808c3a0-dfe5-855c-6d92-da89d9db2adb@iogearbox.net>
Date:   Mon, 24 Sep 2018 14:38:27 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Joe Stringer <joe@...d.net.nz>, ast@...nel.org
Cc:     netdev@...r.kernel.org, john.fastabend@...il.com, tgraf@...g.ch,
        kafai@...com, nitin.hande@...il.com, mauricio.vasquez@...ito.it
Subject: Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in
 BPF

On 09/24/2018 02:12 PM, Daniel Borkmann wrote:
> Hi Joe,
> 
> couple of comments inline:
> 
> On 09/21/2018 07:10 PM, Joe Stringer wrote:
>> This patch adds new BPF helper functions, bpf_sk_lookup_tcp() and
>> bpf_sk_lookup_udp() which allows BPF programs to find out if there is a
>> socket listening on this host, and returns a socket pointer which the
>> BPF program can then access to determine, for instance, whether to
>> forward or drop traffic. bpf_sk_lookup_xxx() may take a reference on the
>> socket, so when a BPF program makes use of this function, it must
>> subsequently pass the returned pointer into the newly added sk_release()
>> to return the reference.
>>
>> By way of example, the following pseudocode would filter inbound
>> connections at XDP if there is no corresponding service listening for
>> the traffic:
>>
>>   struct bpf_sock_tuple tuple;
>>   struct bpf_sock_ops *sk;
>>
>>   populate_tuple(ctx, &tuple); // Extract the 5tuple from the packet
>>   sk = bpf_sk_lookup_tcp(ctx, &tuple, sizeof tuple, netns, 0);
>>   if (!sk) {
>>     // Couldn't find a socket listening for this traffic. Drop.
>>     return TC_ACT_SHOT;
>>   }
>>   bpf_sk_release(sk, 0);
>>   return TC_ACT_OK;
>>
>> Signed-off-by: Joe Stringer <joe@...d.net.nz>
>>
>> ---
>>
>> v2: Rework 'struct bpf_sock_tuple' to allow passing a packet pointer
>>     Limit netns_id field to 32 bits
>>     Fix compile error with CONFIG_IPV6 enabled
>>     Allow direct packet access from helper
>> ---
>>  include/uapi/linux/bpf.h                  |  57 ++++++++-
>>  kernel/bpf/verifier.c                     |   8 +-
>>  net/core/filter.c                         | 149 ++++++++++++++++++++++
>>  tools/include/uapi/linux/bpf.h            |  57 ++++++++-
>>  tools/testing/selftests/bpf/bpf_helpers.h |  12 ++
>>  5 files changed, 280 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index aa5ccd2385ed..620adbb09a94 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -2143,6 +2143,41 @@ union bpf_attr {
>>   *		request in the skb.
>>   *	Return
>>   *		0 on success, or a negative error in case of failure.
>> + *
>> + * struct bpf_sock_ops *bpf_sk_lookup_tcp(ctx, tuple, tuple_size, netns, flags)
> 
> Nit: could you add proper signature like in other cases that are documented?
> 
>> + * 	Decription
>> + * 		Look for TCP socket matching 'tuple'. The return value must
>> + * 		be checked, and if non-NULL, released via bpf_sk_release().
>> + * 		@ctx: pointer to ctx
>> + * 		@tuple: pointer to struct bpf_sock_tuple
>> + * 		@tuple_size: size of the tuple
>> + * 		@netns: network namespace id
>> + * 		@flags: flags value
> 
> Should probably say in all three cases that it's unused right now and reserved
> for future.

I think the two lookup helpers here also need to have a better documentation in
terms of semantics wrt netns, so it definitely needs to be documented when it's
derived from the skb's dev and when netns is used.

>> + * 	Return
>> + * 		pointer to socket ops on success, or
>> + * 		NULL in case of failure
>> + *
>> + * struct bpf_sock_ops *bpf_sk_lookup_udp(ctx, tuple, tuple_size, netns, flags)
>> + * 	Decription
>> + * 		Look for UDP socket matching 'tuple'. The return value must
>> + * 		be checked, and if non-NULL, released via bpf_sk_release().
>> + * 		@ctx: pointer to ctx
>> + * 		@tuple: pointer to struct bpf_sock_tuple
>> + * 		@tuple_size: size of the tuple
>> + * 		@netns: network namespace id
>> + * 		@flags: flags value
>> + * 	Return
>> + * 		pointer to socket ops on success, or
>> + * 		NULL in case of failure
>> + *
>> + *  int bpf_sk_release(sock, flags)
>> + * 	Description
>> + * 		Release the reference held by 'sock'.
>> + * 		@sock: Pointer reference to release. Must be found via
>> + * 		       bpf_sk_lookup_xxx().
>> + * 		@flags: flags value
>> + * 	Return
>> + * 		0 on success, or a negative error in case of failure.
>>   */