Re: [PATCH bpf-next] bpf: add bpf_ct_lookup_{tcp,udp}() helpers

On Sat, Jan 18, 2020 at 8:05 PM John Fastabend john.fastabend@gmail.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@stackpath.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.

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