Message-ID: <CAOftzPgMQ8_9g_hq7e7D3Zxod9e_jXsUUZwNz_8FziunxL2X2Q@mail.gmail.com>
Date:   Mon, 24 Sep 2018 11:04:02 -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:12, Daniel Borkmann <daniel@...earbox.net> wrote:
>
> Hi Joe,
>
> couple of comments inline:

Thanks for the review, I'll fix up those bits.

> 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
> > ...
> > +/* bpf_sk_lookup performs the core lookup for different types of sockets,
> > + * taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
> > + * Returns the socket as an 'unsigned long' to simplify the casting in the
> > + * callers to satisfy BPF_CALL declarations.
> > + */
> > +static unsigned long
> > +bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> > +           u8 proto, u64 netns_id, u64 flags)
> > +{
> > +     struct net *caller_net = dev_net(skb->dev);
>
> For sk_skb programs, are we *always* guaranteed to have a skb->dev assigned?
>
> This definitely holds true for tc programs, but afaik not for sk_skb ones where
> you enable the helpers below, so this would result in a NULL ptr dereference.

I'll update this to take the netns from the skb->sk in those hooks.

> > +BPF_CALL_2(bpf_sk_release, struct sock *, sk, u64, flags)
> > +{
> > +     if (!sock_flag(sk, SOCK_RCU_FREE))
> > +             sock_gen_put(sk);
> > +
> > +     if (unlikely(flags))
> > +             return -EINVAL;
>
> I guess it's probably okay to leave here, though I'm wondering whether it's
> worth to have a flags part in general in this helper. We need to release the
> reference in any case beforehands anyway as we otherwise leak it. Personally,
> I'd probably that arg here.

I agree, I can't think of a good reason to have optional behaviour on
socket release. I'll drop it.

> > +     return 0;
> > +}
> > +
> > +static const struct bpf_func_proto bpf_sk_release_proto = {
> > +     .func           = bpf_sk_release,
> > +     .gpl_only       = false,
> > +     .ret_type       = RET_INTEGER,
> > +     .arg1_type      = ARG_PTR_TO_SOCKET,
> > +     .arg2_type      = ARG_ANYTHING,
> > +};
> > +
> >  bool bpf_helper_changes_pkt_data(void *func)
> >  {
> >       if (func == bpf_skb_vlan_push ||
> > @@ -5018,6 +5155,12 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >       case BPF_FUNC_skb_ancestor_cgroup_id:
> >               return &bpf_skb_ancestor_cgroup_id_proto;
> >  #endif
> > +     case BPF_FUNC_sk_lookup_tcp:
> > +             return &bpf_sk_lookup_tcp_proto;
> > +     case BPF_FUNC_sk_lookup_udp:
> > +             return &bpf_sk_lookup_udp_proto;
> > +     case BPF_FUNC_sk_release:
> > +             return &bpf_sk_release_proto;
> >       default:
> >               return bpf_base_func_proto(func_id);
> >       }
> > @@ -5118,6 +5261,12 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >               return &bpf_sk_redirect_hash_proto;
> >       case BPF_FUNC_get_local_storage:
> >               return &bpf_get_local_storage_proto;
> > +     case BPF_FUNC_sk_lookup_tcp:
> > +             return &bpf_sk_lookup_tcp_proto;
> > +     case BPF_FUNC_sk_lookup_udp:
> > +             return &bpf_sk_lookup_udp_proto;
> > +     case BPF_FUNC_sk_release:
> > +             return &bpf_sk_release_proto;
>
> (See comment above.)

Powered by blists - more mailing lists