Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpf: clean-room implementation of parts of the code #2048

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@iaguis
Copy link
Contributor

commented Jun 18, 2019

Description

Kinvolk was contacted by @tgraf about suspected unattributed code which Kinvolk commited to the Felix repository.

This PR is a clean-room implementation for any similarities that we suspect @tgraf might be referring to.

Todos

  • Unit tests (full coverage)
  • Integration tests (delete as appropriate) In plan/Not needed/Done
  • Documentation
  • Backport
  • Release note

Release Note

None required

@iaguis iaguis requested a review from projectcalico/core-maintainers as a code owner Jun 18, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented Jun 18, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@tgraf

This comment has been minimized.

Copy link

commented Jun 18, 2019

@iaguis This is not a clean room design implementation. This PR is moving code around, un-inlining functions and converting switch conditionals to if conditionals. In fact, even after this PR, some code snippets still match code from the Cilium repository 100%. This PR does not change the fact that it's very clear where this code was derived from.

You are free to copy, reuse and modify the source code of the Cilium repository in any way you want, as long as you respect the open source license which includes, among other things, preserving the copyright and license as well as attributing the original authors. Please do so.

Regarding your statement that this is a suspicion. The following is a diff of relevant code sections between the original source [0] and the version committed to this repository:

[0] https://github.com/cilium/cilium/blob/master/bpf/bpf_xdp.c
[1] https://github.com/projectcalico/felix/blob/master/bpf/xdp/filter.c

I see no room for interpretation. Large sections are even entirely unmodified.

diff --git a/cilium.c b/cilium.c
index e4a1b90..2b41247 100644
--- a/cilium.c
+++ b/cilium.c
@@ -1,81 +1,73 @@
 static __always_inline void *xdp_data(const struct xdp_md *xdp)
 {
        return (void *)(unsigned long)xdp->data;
 }

 static __always_inline void *xdp_data_end(const struct xdp_md *xdp)
 {
        return (void *)(unsigned long)xdp->data_end;
 }

 static __always_inline bool xdp_no_room(const void *needed, const void *limit)
 {
-       return unlikely(needed > limit);
+       return needed > limit;
 }

 struct lpm_v4_key {
        struct bpf_lpm_trie_key lpm;
        __u8 addr[4];
 };

-struct lpm_v6_key {
-       struct bpf_lpm_trie_key lpm;
-       __u8 addr[16];
-};
-
 struct lpm_val {
-       /* Just dummy for now. */
-       __u8 flags;
+       __u32 ref_count;
 };

 [...]

 static __always_inline int check_v4(struct xdp_md *xdp)
 {
        void *data_end = xdp_data_end(xdp);
        void *data = xdp_data(xdp);
        struct iphdr *ipv4_hdr = data + sizeof(struct ethhdr);
-       struct lpm_v4_key pfx __maybe_unused;
+       struct lpm_v4_key pfx;
+       __u16 dest_port;

-       if (xdp_no_room(ipv4_hdr + 1, data_end))
+       if (xdp_no_room(ipv4_hdr + 1, data_end)) {
                return XDP_DROP;
+       }

-#ifdef CIDR4_FILTER
        __builtin_memcpy(pfx.lpm.data, &ipv4_hdr->saddr, sizeof(pfx.addr));
        pfx.lpm.prefixlen = 32;

-#ifdef CIDR4_LPM_PREFILTER
-       if (map_lookup_elem(&CIDR4_LMAP_NAME, &pfx))
+       if (map_lookup_elem(&calico_prefilter_v4, &pfx)) {

 [...]
 }

-static __always_inline int check_filters(struct xdp_md *xdp)
+static __always_inline int check_prefilter(struct xdp_md *xdp)
 {
        void *data_end = xdp_data_end(xdp);
        void *data = xdp_data(xdp);
        struct ethhdr *eth = data;
        __u16 proto;

-       if (xdp_no_room(eth + 1, data_end))
+       if (xdp_no_room(eth + 1, data_end)) {
                return XDP_DROP;
+       }

        proto = eth->h_proto;
-       if (proto == bpf_htons(ETH_P_IP))
+       if (proto == bpf_htons(ETH_P_IP)) {
                return check_v4(xdp);
-       else if (proto == bpf_htons(ETH_P_IPV6))
-               return check_v6(xdp);
-       else
-               /* Pass the rest to stack, we might later do more
-                * fine-grained filtering here.
-                */
+       } else {
+               /* other traffic can continue */
                return XDP_PASS;
+       }
 }

-__section("from-netdev")
-int xdp_start(struct xdp_md *xdp)
+__section("pre-filter")
+int xdp_enter(struct xdp_md *xdp)
 {
-       return check_filters(xdp);
+       return check_prefilter(xdp);
 }

-BPF_LICENSE("GPL");
+char ____license[] __section("license")  = "Apache-2.0";
@iaguis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Superseded by #2049

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.