Message-ID: <CAGyo_hpcO-f9uxQFDfKZNz=1t6Yux+LzxN1qLHKf6PXMAtWQ-w@mail.gmail.com>
Date:   Mon, 20 Jan 2020 13:10:51 -0700
From:   Matt Cover <werekraken@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Shuah Khan <shuah@...nel.org>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Quentin Monnet <quentin.monnet@...ronome.com>,
        Matthew Cover <matthew.cover@...ckpath.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Andrey Ignatov <rdna@...com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Jiong Wang <jiong.wang@...ronome.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: add bpf_ct_lookup_{tcp,udp}() helpers

On Mon, Jan 20, 2020 at 11:11 AM Matt Cover <werekraken@...il.com> wrote:
>
> On Sat, Jan 18, 2020 at 8:05 PM John Fastabend <john.fastabend@...il.com> wrote:
> >
> > Matthew Cover wrote:
> > > Allow looking up an nf_conn. This allows eBPF programs to leverage
> > > nf_conntrack state for similar purposes to socket state use cases,
> > > as provided by the socket lookup helpers. This is particularly
> > > useful when nf_conntrack state is locally available, but socket
> > > state is not.
> > >
> > > Signed-off-by: Matthew Cover <matthew.cover@...ckpath.com>
> > > ---
> >
> > Couple coding comments below. Also looks like a couple build errors
> > so fix those up. I'm still thinking over this though.
>
> Thank you for taking the time to look this over. I will be looking
> into the build issues.

Looks like I missed static inline on a couple functions when
nf_conntrack isn't builtin. I'll include the fix in v2.

>
> >
> > Also I prefer the tests in their own patch. So make it a two patch
> > series.
>
> Sounds good. I will submit as a series for v2.
>
> >
> > fwiw I think we could build a native xdp lib for connection tracking
> > but maybe there are reasons to pull in core conn tracking. Seems like
> > a separate discussion.
>
> Native xdp connection tracking would be cool as well. Cilium seems to
> have ebpf conntrack; perhaps it can provide some useful insights into
> that effort.
>
> Even with native xdp connection tracking available, I see value in
> these helpers, particularly when core conntrack is already in use.
>
> >
> > > + * struct bpf_nf_conn *bpf_ct_lookup_udp(void *ctx, struct bpf_nf_conntrack_tuple *tuple, u32 tuple_size, u64 netns, u64 flags)
> > > + *   Description
> > > + *           Look for UDP nf_conntrack entry matching *tuple*, optionally in
> > > + *           a child network namespace *netns*. The return value must be
> > > + *           checked, and if non-**NULL**, released via
> > > + *           **bpf_ct_release**\ ().
> > > + *
> > > + *           The *ctx* should point to the context of the program, such as
> > > + *           the skb or xdp_md (depending on the hook in use). This is used
> > > + *           to determine the base network namespace for the lookup.
> > > + *
> > > + *           *tuple_size* must be one of:
> > > + *
> > > + *           **sizeof**\ (*tuple*\ **->ipv4**)
> > > + *                   Look for an IPv4 nf_conn.
> > > + *           **sizeof**\ (*tuple*\ **->ipv6**)
> > > + *                   Look for an IPv6 nf_conn.
> > > + *
> > > + *           If the *netns* is a negative signed 32-bit integer, then the
> > > + *           nf_conn lookup table in the netns associated with the *ctx* will
> > > + *           will be used. For the TC hooks, this is the netns of the device
> > > + *           in the skb. For XDP hooks, this is the netns of the device in
> > > + *           the xdp_md. If *netns* is any other signed 32-bit value greater
> > > + *           than or equal to zero then it specifies the ID of the netns
> > > + *           relative to the netns associated with the *ctx*. *netns* values
> > > + *           beyond the range of 32-bit integers are reserved for future
> > > + *           use.
> >
> > I find the usage of netns a bit awkward. Its being passed as a u64 and
> > then used as a signed int with the pivot depending on negative?
> >
> > How about pivot on a flag instead of the signed bit of netns here.
>
> The interface (and much of the code) is a clone of the
> bpf_sk_lookup_xxx helper functions. I figured having it match would
> both make it familiar and give this patch a better chance of being
> applied.
>
> I'd prefer not to diverge from bpf_sk_lookup_xxx helpers here. That
> is my only objection to what you propose.
>
> >
> > > + *
> > > + *           All values for *flags* are reserved for future usage, and must
> > > + *           be left at zero.
> > > + *
> > > + *           This helper is available only if the kernel was compiled with
> > > + *           **CONFIG_NF_CONNTRACK=y** configuration option.
> >
> > I suspect this should be,
> >
> > "This helper will return NULL if the kernel was compiled with ..."
> >
>
> Good idea. I'll work this into v2 for additional clarity.
>
> > Same comment for the earlier _tcp helper.
> >
> > > + *   Return
> > > + *           Pointer to **struct bpf_nf_conn**, or **NULL** in case of
> > > + *           failure.
> > > + *
> > > + * int bpf_ct_release(struct bpf_nf_conn *ct)
> > > + *   Description
> > > + *           Release the reference held by *ct*. *ct* must be a
> > > + *           non-**NULL** pointer that was returned from
> > > + *           **bpf_ct_lookup_xxx**\ ().
> > > + *   Return
> > > + *           0 on success, or a negative error in case of failure.
> > >   */
> > >  #define __BPF_FUNC_MAPPER(FN)                \
> > >       FN(unspec),                     \
> >
> > [...]
> >
> > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > >   * function eBPF program intends to call
> > > @@ -3278,6 +3363,30 @@ struct bpf_sock_tuple {
> > >       };
> > >  };
> > >
> > > +struct bpf_nf_conn {
> > > +     __u32 cpu;
> > > +     __u32 mark;
> > > +     __u32 status;
> > > +     __u32 timeout;
> > > +};
> > > +
> > > +struct bpf_nf_conntrack_tuple {
> > > +     union {
> > > +             struct {
> > > +                     __be32 saddr;
> > > +                     __be32 daddr;
> > > +                     __be16 sport;
> > > +                     __be16 dport;
> > > +             } ipv4;
> > > +             struct {
> > > +                     __be32 saddr[4];
> > > +                     __be32 daddr[4];
> > > +                     __be16 sport;
> > > +                     __be16 dport;
> > > +             } ipv6;
> > > +     };
> > > +};
> > > +
> >
> > [...]
> >
> > > +static int check_nf_ct_access(struct bpf_verifier_env *env, int insn_idx,
> > > +                          u32 regno, int off, int size,
> > > +                          enum bpf_access_type t)
> > > +{
> > > +     struct bpf_reg_state *regs = cur_regs(env);
> > > +     struct bpf_reg_state *reg = &regs[regno];
> > > +     struct bpf_insn_access_aux info = {};
> > > +     bool valid;
> > > +
> > > +     switch (reg->type) {
> > > +     case PTR_TO_NF_CONN:
> > > +             valid = bpf_nf_conn_is_valid_access(off, size, t, &info);
> > > +             break;
> > > +     default:
> > > +             valid = false;
> > > +     }
> > > +
> > > +     if (valid) {
> > > +             env->insn_aux_data[insn_idx].ctx_field_size =
> > > +                     info.ctx_field_size;
> > > +             return 0;
> > > +     }
> > > +
> > > +     verbose(env, "R%d invalid %s access off=%d size=%d\n",
> > > +             regno, reg_type_str[reg->type], off, size);
> > > +
> > > +     return -EACCES;
> >
> > nit, but this construction feels odd to me. How about,
> >
> >  if (reg->type != PTR_TO_NF_CONN) {
> >         verbose(...)
> >         return -EACCES;
> >  }
> >
> >  env-> ...
> >  return 0;
> >
> > The switch sort of implies you have some ideas on future types? What would
> > those be?
>
> Sure, I can reduce this down if desired. I was viewing it more as
> following the pattern seen in other check access functions.
>
> I do plan to introduce a "tcp_nf_conn" as another series, akin to
> "tcp_sock". When that happens this construct may make more sense.
>
> e.g.
>        case offsetof(struct bpf_tcp_nf_conn, state):
> ...
>                *insn++ = BPF_LDX_MEM(BPF_B, si->dst_reg, si->src_reg,
>                                offsetof(struct nf_conn, proto) +
>                                offsetof(union nf_conntrack_proto, tcp) +
>                                offsetof(struct ip_ct_tcp, state));
>
> >
> > > +}
> > > +
> > >  static int check_sock_access(struct bpf_verifier_env *env, int insn_idx,
> > >                            u32 regno, int off, int size,
> > >                            enum bpf_access_type t)
> > > @@ -2511,6 +2556,13 @@ static bool is_ctx_reg(struct bpf_verifier_env *env, int regno)
> > >       return reg->type == PTR_TO_CTX;
> > >  }
> >
> > [...]
> >
> >
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 17de674..39ba965 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -74,6 +74,12 @@
> >
> > [...]
> >
> > > +static struct nf_conn *
> > > +__bpf_ct_lookup(struct sk_buff *skb, struct bpf_nf_conntrack_tuple *tuple, u32 len,
> > > +             struct net *caller_net, u8 proto, u64 netns_id, u64 flags)
> >
> > Why not just make netns an int instead of pulling a unsigned from the helper and
> > then converting it into an int?
>
> These three are mostly a question of if we want to diverge from
> __bpf_sk_lookup. If we choose to do so, then do we want to update
> __bpf_sk_lookup to match? I think there is benefit to having the
> uapi exposed interfaces match.
>
> >
> > > +{
> > > +     struct nf_conn *ct = NULL;
> > > +     u8 family = AF_UNSPEC;
> > > +     struct net *net;
> > > +
> > > +     if (len == sizeof(tuple->ipv4))
> > > +             family = AF_INET;
> > > +     else if (len == sizeof(tuple->ipv6))
> > > +             family = AF_INET6;
> > > +     else
> > > +             goto out;
> > > +
> > > +     if (unlikely(family == AF_UNSPEC || flags ||
> > > +                  !((s32)netns_id < 0 || netns_id <= S32_MAX)))
> >                                             ^^^^^^^^^^^^^^^^^^^^
> > If you pass an int here and use flags to set the type I think you avoid this
> > check.
>
> See previous.
>
> >
> > > +             goto out;
> > > +
> > > +     if ((s32)netns_id < 0) {
> >
> > I don't like this casting here again fallout from u64->int conversion.
>
> See previous.
>
> >
> > > +             net = caller_net;
> > > +             ct = ct_lookup(net, tuple, family, proto);
> > > +     } else {
> > > +             net = get_net_ns_by_id(caller_net, netns_id);
> > > +             if (unlikely(!net))
> > > +                     goto out;
> > > +             ct = ct_lookup(net, tuple, family, proto);
> > > +             put_net(net);
> > > +     }
> > > +
> > > +out:
> > > +     return ct;
> > > +}
> > > +
> >
> > [...]
> >
> > Thanks!
> > John

Powered by blists - more mailing lists