From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCHv2 bpf-next 07/11] bpf: Add helper to retrieve socket in BPF Date: Mon, 24 Sep 2018 14:38:27 +0200 Message-ID: References: <20180921171043.20823-1-joe@wand.net.nz> <20180921171043.20823-8-joe@wand.net.nz> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, john.fastabend@gmail.com, tgraf@suug.ch, kafai@fb.com, nitin.hande@gmail.com, mauricio.vasquez@polito.it To: Joe Stringer , ast@kernel.org Return-path: Received: from www62.your-server.de ([213.133.104.62]:57631 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729720AbeIXSka (ORCPT ); Mon, 24 Sep 2018 14:40:30 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> >> --- >> >> 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. >> */