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 11:05:33 -0700
From:   Joe Stringer <joe@...d.net.nz>
To:     daniel@...earbox.net
Cc:     Joe Stringer <joe@...d.net.nz>, 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 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.
> >> + *  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.
> >>   */