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

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.

Also I prefer the tests in their own patch. So make it a two patch series.

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.

• • 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.

• • 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 ..."

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?

+}

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?

+{

• 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.

• goto out;
  

• if ((s32)netns_id < 0) {

I don't like this casting here again fallout from u64->int conversion.

• 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