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]

Date:   Mon, 24 Sep 2018 21:18:37 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Joe Stringer <joe@...d.net.nz>
Cc:     ast@...nel.org, netdev <netdev@...r.kernel.org>,
        john fastabend <john.fastabend@...il.com>, tgraf@...g.ch,
        Martin KaFai Lau <kafai@...com>,
        Nitin Hande <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 08:05 PM, Joe Stringer wrote:
> On Mon, 24 Sep 2018 at 05:38, Daniel Borkmann <daniel@...earbox.net> 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 <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.
> 
> 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."