On 09/01/2017 01:30 PM, William Tu wrote:
This patch adds two BPF conntrack helper functions, bpf_ct_lookup()
and bpf_ct_commit(), to enable the possibility of BPF stateful firewall.
There are two ways to implement BPF conntrack. One way is to not
rely on helpers but implement the conntrack state table using BPF
maps. So conntrack is basically another BPF program extracting
the tuples and lookup/update its map. Currenly Cillium project has
implemented this way.
Instaed, this patch is proposed to reuse the the existing netfilter
conntrack. By creating two helpers, bpf_skb_ct_lookup() and
bpf_skb_ct_commit(), a BPF program simply lookup the conntrack state,
based on the conntrack zone, or commit the new connection into the
netfilter conntrack table, for later lookup hit. In the case where
a fragmented packet is received, the helper handles it by submitting
packets to ip_defrag() and return TC_ACT_STOLEN if being queued.
An simple example is to allow PING packet to your host, only when
your host initiates the ping. The first ping from your host to
outside will be tracked into conntrack table as new, and the echo
reply coming into your host will make it established. So the
subsequent ping will pass the firwall. (see tcbpf3_kern.c and
test_conntrack_bpf.h)
Now the helper is attached to TC, and requires skb to interact with
netfilter conntrack, so XDP program does not apply here.
In the future, I'd like to test/implement ip defragmentation, alg,
and nat. But I'd like to have some early feedbacks. Thanks!
Signed-off-by: William Tu <[email protected]>
Cc: Joe Stringer <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
[...]
Just curious, were you able to do some performance testing with
the helper?
[...]
FN(unspec), \
@@ -638,6 +650,8 @@ union bpf_attr {
FN(redirect_map), \
FN(sk_redirect_map), \
FN(sock_map_update), \
+ FN(skb_ct_lookup), \
+ FN(skb_ct_commit), \
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
@@ -737,6 +751,23 @@ struct bpf_tunnel_key {
__u32 tunnel_label;
};
+#define BPF_CT_LABELS_LEN 16
+#define BPF_F_CT_DEFRAG (1ULL << 0)
+
+struct bpf_conntrack_info {
+ __u32 ct_state;
+ __u16 family;
+ __u16 zone_id;
Have you considered also supporting zone directions, so you can have
overlapping tuples? Presumably if you have some other interaction
with CT where they are used and you do the lookup here with dir as
NF_CT_DEFAULT_ZONE_DIR, then CT won't find them if I remember correctly.
+ /* for conntrack mark */
+ __u32 mark_value;
+ __u32 mark_mask;
+
+ /* for conntrack label */
+ __u8 ct_label_value[16];
+ __u8 ct_label_mask[16];
+};
+
/* Generic BPF return codes which all BPF program types may support.
* The values are binary compatible with their TC_ACT_* counter-part to
* provide backwards compatibility with existing SCHED_CLS and SCHED_ACT
diff --git a/net/core/filter.c b/net/core/filter.c
index c6a37fe0285b..906518043f19 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -56,6 +56,11 @@
#include <net/busy_poll.h>
#include <net/tcp.h>
#include <linux/bpf_trace.h>
+#include <net/netfilter/nf_conntrack_core.h>
+#include <net/netfilter/nf_conntrack.h>
+#include <net/netfilter/nf_conntrack_helper.h>
+#include <net/netfilter/nf_conntrack_labels.h>
+#include <net/netfilter/nf_conntrack_zones.h>
/**
* sk_filter_trim_cap - run a packet through a socket filter
@@ -2674,6 +2679,221 @@ static unsigned long bpf_skb_copy(void *dst_buff, const
void *skb,
return 0;
}
+/* Lookup the conntrack with bpf_conntrack_info.
+ * If found, receive the conntrack state, mark, and label
+ */
+BPF_CALL_3(bpf_skb_ct_lookup, struct sk_buff *, skb,
+ struct bpf_conntrack_info *, info, u64, flags)
+{
+ struct net *net = dev_net(skb->dev);
+ enum ip_conntrack_info ctinfo;
+ struct nf_conntrack_zone zone;
+ struct nf_conn_labels *cl;
+ struct nf_conn *ct, *tmpl;
+ int err, nh_ofs;
+ struct iphdr *iph;
Please err out on invalid flags, otherwise new flags cannot
be added anymore since they would break existing programs.
+ /* Conntrack expects L3 packet */
+ nh_ofs = skb_network_offset(skb);
+ skb_pull_rcsum(skb, nh_ofs);
+ iph = ip_hdr(skb);
What happens if you call this function with an IPv6 or some
other non-IPv4 pkt and go below into IPv4 defrag for example?
We would also need to handle this for IPv6 in general, and
bail out if skb->protocol is neither IPv4 nor IPv6.
+ if (ip_is_fragment(iph) && (flags & BPF_F_CT_DEFRAG)) {
+ /* XXX: not test yet */
+ enum ip_defrag_users user =
+ IP_DEFRAG_CONNTRACK_IN + info->zone_id;
+
+ memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
The cb[] is already used in this layer by tc and BPF, you would
somewhat need to save / restore the content of the cb[] before
returning back to BPF program, or making sure the skb never goes
back to either of them in first place, but see below.
+ err = ip_defrag(net, skb, user);
+ if (err == -EINPROGRESS)
+ return TC_ACT_STOLEN;
This doesn't work like that, I'm afraid. You'll get a use
after free here. Reason is that skb can be freed in ip_defrag()
and there is zero guarantee or control that the BPF program
won't touch the skb anymore. E.g. the program could just ignore
the return code and continuing accessing the already freed skb.
See what we did with bpf_redirect() helper, the actual redirection
happens outside of BPF program in skb_do_redirect(). As preemption
is still disabled at that time, it's valid to do so, and when a
prog ignores bpf_redirect()'s return code, then it doesn't affect
system's stability at all. So this would need to be some kind of
new tc action to make a similar split.
You also have a second use after free above in the sense that when
you call into ip_frag_queue() from a helper, then you need to
invalidate packet pointers for BPF. Everything that can call into
pskb_expand_head() has to be added to bpf_helper_changes_pkt_data()
and needs to add a bpf_compute_data_end(skb) after we reallocated
skb head, as otherwise your underlying buffer skb->data points to
is still the one already passed back to the allocator.
+ }
+
+ /* TODO: conntrack expectation */
+
+ nf_ct_zone_init(&zone, info->zone_id,
+ NF_CT_DEFAULT_ZONE_DIR, 0);
+ tmpl = nf_ct_tmpl_alloc(net, &zone, GFP_KERNEL);
As mentioned by Alexei, we're under RCU critical section, so not
allowed to sleep, saw the same below in bpf_skb_ct_commit(), so
needs to be atomic indeed.
+ if (!tmpl) {
+ pr_err("Failed to allocate conntrack template\n");
+ goto error;
+ }
+ nf_conntrack_get(&tmpl->ct_general);
+ nf_ct_set(skb, tmpl, IP_CT_NEW);
+
+ do {
+ err = nf_conntrack_in(net, info->family,
+ NF_INET_PRE_ROUTING, skb);
+ } while (err != NF_ACCEPT);
Why looping here, is that for when you fail to allocate CT entry
due to mem shortage? Shouldn't we rather just bail out?
+ ct = nf_ct_get(skb, &ctinfo);
+ if (!ct) {
+ pr_err("nf_ct_get failed\n");
+ goto error;
+ }
+
+ info->ct_state = (u8)ctinfo;
+ info->mark_value = ct->mark;
+
+ cl = nf_ct_labels_find(ct);
+ if (cl) {
+ size_t len = sizeof(cl->bits);
+
+ if (len > BPF_CT_LABELS_LEN)
+ len = BPF_CT_LABELS_LEN;
+ else if (len < BPF_CT_LABELS_LEN)
+ memset(info->ct_label_value, 0, BPF_CT_LABELS_LEN);
+ memcpy(info->ct_label_value, cl->bits, len);
+ } else {
+ memset(info->ct_label_value, 0, BPF_CT_LABELS_LEN);
+ }
+
+error:
+ skb_push_rcsum(skb, nh_ofs);
+ return TC_ACT_OK;
+}
+
+static const struct bpf_func_proto bpf_skb_ct_lookup_proto = {
+ .func = bpf_skb_ct_lookup,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_ANYTHING,
+};
+
+static int bpf_skb_ct_set_mark(struct sk_buff *skb, u32 ct_mark, u32 mask)
+{
+#if IS_ENABLED(CONFIG_NF_CONNTRACK_MARK)
+ enum ip_conntrack_info ctinfo;
+ struct nf_conn *ct;
+ u32 new_mark;
+
+ ct = nf_ct_get(skb, &ctinfo);
+ if (!ct)
+ return 0;
+
+ new_mark = ct_mark | (ct->mark & ~(mask));
+ if (ct->mark != new_mark)
+ ct->mark = new_mark;
+ return 0;
+#else
+ return -ENOTSUP;
+#endif
+}
+
+static int bpf_skb_ct_set_labels(struct sk_buff *skb,
+ const u8 *labels, const u8 *mask)
+{
+#if IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS)
+ enum ip_conntrack_info ctinfo;
+ struct nf_conn_labels *cl;
+ struct nf_conn *ct;
+ int err;
+
+ ct = nf_ct_get(skb, &ctinfo);
+ if (!ct)
+ return 0;
+
+ cl = nf_ct_labels_find(ct);
+ if (!cl) {
+ nf_ct_labels_ext_add(ct);
Does this need a nf_connlabels_get() somewhere?
+ cl = nf_ct_labels_find(ct);
+ if (!cl)
+ return -ENOSPC;
+ }
+
+ err = nf_connlabels_replace(ct, (u32 *)labels, (u32 *)mask,
+ BPF_CT_LABELS_LEN / sizeof(u32));
+ if (err)
+ return err;
+ return 0;
+#else
+ return -ENOTSUP;
+#endif
+}
+
+static bool labels_nonzero(const u8 *labels)
+{
+ int i;
+
+ for (i = 0; i < BPF_CT_LABELS_LEN; i++)
+ if (labels[i])
+ return true;
+ return false;
+}
+
+/* Take the skb and do nf_conntrack_in first,
+ * set mark and labels, and confirm
+ */
+BPF_CALL_3(bpf_skb_ct_commit, struct sk_buff *, skb,
+ struct bpf_conntrack_info *, info, u64, flags)
+{
+ struct net *net = dev_net(skb->dev);
+ enum ip_conntrack_info ctinfo;
+ struct nf_conntrack_zone zone;
+ struct nf_conn *ct, *tmpl;
+ int err, nh_ofs;
(err on invalid flags)
+ /* Conntrack expects L3 packet */
+ nh_ofs = skb_network_offset(skb);
+ skb_pull_rcsum(skb, nh_ofs);
Doesn't above skb_pull_rcsum() assume that you have a guaranteed
linear area that you work on here? I don't think we have it as a
guarantee. (Same also in bpf_skb_ct_lookup().)
+ nf_ct_zone_init(&zone, info->zone_id,
+ NF_CT_DEFAULT_ZONE_DIR, 0);
+ tmpl = nf_ct_tmpl_alloc(net, &zone, GFP_KERNEL);
(GFP_KERNEL)
+ if (!tmpl) {
+ pr_err("Failed to allocate conntrack template\n");
+ goto error;
+ }
+
+ nf_conntrack_get(&tmpl->ct_general);
+ nf_ct_set(skb, tmpl, IP_CT_NEW);
+
+ do {
+ err = nf_conntrack_in(net, info->family,
+ NF_INET_PRE_ROUTING, skb);
+ } while (err != NF_ACCEPT);
+
+ ct = nf_ct_get(skb, &ctinfo);
+ if (!ct) {
+ pr_err("nf_ct_get failed\n");
+ goto error;
_______________________________________________
iovisor-dev mailing list
[email protected]
https://lists.iovisor.org/mailman/listinfo/iovisor-dev
