On Mon, Jan 20, 2020 at 11:11 AM Matt Cover <werekraken@xxxxxxxxx> wrote:
>
> On Sat, Jan 18, 2020 at 8:05 PM John Fastabend <john.fastabend@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> >
> > 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 = ®s[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
