small probability outer ip header checksum error

[duanxiongchun]

we test katran in our environment。and find some outer ip header checksum error about （1/100000）rate 。
I check the code in katran/lib/bpf/pckt_encap.h。
in some Small probability 。(csum & 0xffff) + (csum >> 16) may be overflow in 16bit。 it‘s may cause the problem I had mention

after fix code under groud。 this problem disappear。

- iph->check = ~((csum & 0xffff) + (csum >> 16));
+ csum = (csum & 0xffff) + (csum >> 16);
+ csum += (csum >>16);
+ iph->check = ~(csum);

thanks。

[tehnerd]

Thanks. Will look and fix

[tehnerd]

something like this should fix this (we have same issue with icmp generation routines):

diff --git a/katran/lib/bpf/balancer_helpers.h b/katran/lib/bpf/balancer_helpers.h
index 47a5b93..7321d26 100644
--- a/katran/lib/bpf/balancer_helpers.h
+++ b/katran/lib/bpf/balancer_helpers.h
@@ -49,6 +49,16 @@ static inline void ipv4_csum(void *data_start, int data_size,  __u64 *csum) {
   *csum = csum_fold_helper(*csum);
 }

+__attribute__((__always_inline__))
+static inline void ipv4_csum_inline(void *iph, __u64 *csum) {
+  __u16 *next_iph_u16 = (__u16 *)iph;
+  #pragma clang loop unroll(full)
+  for (int i = 0; i < sizeof(struct iphdr) >> 1; i++) {
+     *csum += *next_iph_u16++;
+  }
+  *csum = csum_fold_helper(*csum);
+}
+
 __attribute__((__always_inline__))
 static inline void ipv4_l4_csum(void *data_start, int data_size,
                                 __u64 *csum, struct iphdr *iph) {
diff --git a/katran/lib/bpf/handle_icmp.h b/katran/lib/bpf/handle_icmp.h
index 4e63bc7..02f5661 100644
--- a/katran/lib/bpf/handle_icmp.h
+++ b/katran/lib/bpf/handle_icmp.h
@@ -60,9 +60,7 @@ static inline int send_icmp_reply(void *data, void *data_end) {
   struct icmphdr *icmp_hdr;
   __u32 tmp_addr = 0;
   __u64 off = 0;
-  __u32 csum = 0;
-  __u32 csum1 = 0;
-  __u16 *next_iph_u16;
+  __u64 csum = 0;
   if ((data + sizeof(struct eth_hdr)
       + sizeof(struct iphdr) + sizeof(struct icmphdr)) > data_end) {
     return XDP_DROP;
@@ -81,12 +79,8 @@ static inline int send_icmp_reply(void *data, void *data_end) {
   iph->daddr = iph->saddr;
   iph->saddr = tmp_addr;
   iph->check = 0;
-  next_iph_u16 = (__u16 *)iph;
-  #pragma clang loop unroll(full)
-  for (int i = 0; i < sizeof(struct iphdr) >> 1; i++) {
-     csum += *next_iph_u16++;
-  }
-  iph->check = ~((csum & 0xffff) + (csum >> 16));
+  ipv4_csum_inline(iph, &csum);
+  iph->check = csum;
   return swap_mac_and_send(data, data_end);
 }

diff --git a/katran/lib/bpf/pckt_encap.h b/katran/lib/bpf/pckt_encap.h
index 7d3ed3d..e8d5e09 100644
--- a/katran/lib/bpf/pckt_encap.h
+++ b/katran/lib/bpf/pckt_encap.h
@@ -29,6 +29,7 @@

 #include "balancer_consts.h"
 #include "balancer_structs.h"
+#include "balancer_helpers.h"
 #include "bpf_helpers.h"
 #include "pckt_parsing.h"

@@ -97,8 +98,7 @@ static inline bool encap_v4(struct xdp_md *xdp, struct ctl_value *cval,
   __u32 ip_suffix = htons(pckt->flow.port16[0]);
   ip_suffix <<= 16;
   ip_suffix ^= pckt->flow.src;
-  __u16 *next_iph_u16;
-  __u32 csum = 0;
+  __u64 csum = 0;
   // ipip encap
   if (bpf_xdp_adjust_head(xdp, 0 - (int)sizeof(struct iphdr))) {
     return false;
@@ -129,13 +129,8 @@ static inline bool encap_v4(struct xdp_md *xdp, struct ctl_value *cval,

   iph->saddr = ((0xFFFF0000 & ip_suffix) | IPIP_V4_PREFIX);
   iph->ttl = DEFAULT_TTL;
-
-  next_iph_u16 = (__u16 *)iph;
-  #pragma clang loop unroll(full)
-  for (int i = 0; i < sizeof(struct iphdr) >> 1; i++) {
-     csum += *next_iph_u16++;
-  }
-  iph->check = ~((csum & 0xffff) + (csum >> 16));
+  ipv4_csum_inline(iph, &csum);
+  iph->check = csum;
   return true;
 }

i will do more tests tomorrow and the will create a proper diff with a fix. thank you

[duanxiongchun]

using 64bit csum， it look like no change to overflow 。 great！

[tehnerd]

@duanxiongchun could you please share your setup (src addr + port + what real address you are using) so i can reproduce this error internally. just to make sure that our fix is, well, fixing it.

[tehnerd]

nvm. found few examples

[tehnerd]

should be fixed with latest commit. thanks for reporting

[duanxiongchun]

write a test to request 1000000 time, you will find one in most probability