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 21:18:37 +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: ast@kernel.org, netdev , john fastabend , tgraf@suug.ch, Martin KaFai Lau , Nitin Hande , mauricio.vasquez@polito.it To: Joe Stringer Return-path: Received: from www62.your-server.de ([213.133.104.62]:60922 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730681AbeIYBW0 (ORCPT ); Mon, 24 Sep 2018 21:22:26 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 09/24/2018 08:05 PM, Joe Stringer wrote: > On Mon, 24 Sep 2018 at 05:38, Daniel Borkmann wrote: >> 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. > > Good point, maybe some text like this: > > If 'netns' is 0, then the netns associated with the ctx will be used. > For the tc hooks, this is the netns of the device in the skb. For > socket hooks, this is the netns of the socket. If netns is nonzero, > then this is the netns id relative to the netns associated with the > ctx. Looks good, I'd probably add to the first one "[...] then the socket lookup table in the netns associated with the ctx will be used."