ip checksum issue ?

[sbernard31]

Hi,
I'm pretty much a novice in XDP and low level network protocol but reading rfc791 I understand that checksum should be calculate using the whole ip header.

E.g. :

 Checksum

    The internet header checksum is recomputed if the internet header is
    changed.  For example, a reduction of the time to live, additions or
    changes to internet options, or due to fragmentation.  This checksum
    at the internet level is intended to protect the internet header
    fields from transmission errors.

So would it be more correct to calculate checksum using iph->ihl instead of sizeof(struct iphdr) in balancer_helpers.h ?

(bonus question why using >> 1 instead of / 2 ? 2nd solution seems semantically more appropriate, is there any reason or this is just a programming preference ?)

[tehnerd]

1. unroll for this loop is being done in compile time. iph->ihl is a runtime var. so unroll wont be possible. for balancer code it doesn't matter that we use struct iphdr, as we dont support anything w/ ip options set (so iph->ihl for us is always equal to 5 aka sizeof(struct iphdr))

2. preference. butall of this computed in compile time. so there wont be any perf difference of / vs >>.

[sbernard31]

Thx @tehnerd for the quick response.

About unroll, As I understand, the idea is to replace at compile time a loop by a "static" sequence of instructions. I would have said that was because dynamic loop was not accepted by the bpf verifier, but I write an XDP program with a loop like this :

iph->check = 0;
u32 csum = 0;
__u16 * next_iph_u16 = (__u16 *)iph;
for (int i = 0; i < iph->ihl / 2; i++) {
    csum += *next_iph_u16++;
}
iph->check = ~((csum & 0xffff) + (csum >> 16));

And it seems that works.
So is this about optimization ? or did I missed something ?

[tehnerd]

Have you tried to load it into kernel? Program compiled != it is going to work in kernel. Bpf’s verifier will refuse it when you will try to do so

[sbernard31]

I make it works. I mean I tested it and see modified packet with wireshark.

But If I well understand you, you don't expect this code works ?

[tehnerd]

Could you share the whole code of your program?

[tehnerd]

Yes I don’t expect your code to work. Unless somewhere above you have line like “iph->ihl = 5” and compiler doing unroll for you under the hood

[sbernard31]

Here is my code not intended to be shared I just played with XDP to understand it ...

[tehnerd]

ok. so your code w/ BPF(blahblahb, debug=2):

//  this is equal to line 78
80: (15) if r1 == 0x100007f goto pc+72
 R0=inv2 R1=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6=ctx(id=0,off=0,imm=0) R7=pkt(id=0,off=0,r=34,imm=0) R8=pkt(id=0,off=14,r=34,imm=0) R10=fp0
81: (b7) r1 = 0
// this is memset  from 81
82: (73) *(u8 *)(r7 +5) = r1
83: (73) *(u8 *)(r7 +4) = r1
84: (73) *(u8 *)(r7 +3) = r1
85: (73) *(u8 *)(r7 +2) = r1
86: (73) *(u8 *)(r7 +1) = r1
87: (73) *(u8 *)(r7 +0) = r1
88: (b7) r2 = 16777343
// this is 82 
89: (63) *(u32 *)(r7 +30) = r2
// this is 87
90: (6b) *(u16 *)(r7 +24) = r1
// this is 89
91: (71) r2 = *(u8 *)(r7 +14)
// r3 now equal to the start of the packet. first byte is version + ihl
92: (bf) r3 = r2
// this is testing that ihl at least more than 1
93: (57) r3 &= 14
94: (15) if r3 == 0x0 goto pc+4
 R0=inv2 R1=inv0 R2=inv(id=0,umax_value=255,var_off=(0x0; 0xff)) R3=inv(id=0,umax_value=14,var_off=(0x0; 0xe)) R6=ctx(id=0,off=0,imm=0) R7=pkt(id=0,off=0,r=34,imm=0) R8=pkt(id=0,off=14,r=34,imm=0) R10=fp0
95: (69) r1 = *(u16 *)(r8 +0)
// r2 point to first byte of the packet. two lines bellow is ihl/2
96: (77) r2 >>= 1
97: (57) r2 &= 7
// this says "if ihl /2 is more that 1 (so original ihl is more than 2) -> exit
98: (25) if r2 > 0x1 goto pc+55
 R0=inv2 R1=inv(id=0,umax_value=65535,var_off=(0x0; 0xffff)) R2=inv(id=0,umax_value=1,var_off=(0x0; 0x1)) R3=inv(id=0,umax_value=14,var_off=(0x0; 0xe)) R6=ctx(id=0,off=0,imm=0) R7=pkt(id=0,off=0,r=34,imm=0) R8=pkt(id=0,off=14,r=34,imm=0) R10=fp0
99: (bf) r2 = r1
100: (77) r2 >>= 16
101: (0f) r2 += r1
// here aends first pass of the loop and we exit form it. line 102 here is equal to line 94 in your code
102: (a7) r2 ^= -1
// line below is equal to line 94 in your code. 
103: (6b) *(u16 *)(r7 +24) = r2

// bellow is memcpy etc
104: (71) r1 = *(u8 *)(r7 +9)
105: (67) r1 <<= 8
106: (71) r2 = *(u8 *)(r7 +8)
107: (4f) r1 |= r2
108: (71) r2 = *(u8 *)(r7 +11)
109: (67) r2 <<= 8
110: (71) r3 = *(u8 *)(r7 +10)
111: (4f) r2 |= r3
112: (67) r2 <<= 16
113: (4f) r2 |= r1
114: (71) r3 = *(u8 *)(r7 +1)
115: (67) r3 <<= 8
116: (71) r1 = *(u8 *)(r7 +0)
117: (4f) r3 |= r1
118: (71) r1 = *(u8 *)(r7 +3)
119: (67) r1 <<= 8
120: (71) r4 = *(u8 *)(r7 +2)
121: (4f) r1 |= r4
122: (63) *(u32 *)(r10 -40) = r2
123: (67) r1 <<= 16
124: (4f) r1 |= r3
125: (71) r2 = *(u8 *)(r7 +5)
126: (67) r2 <<= 8
127: (71) r3 = *(u8 *)(r7 +4)
128: (4f) r2 |= r3
129: (71) r3 = *(u8 *)(r7 +7)
130: (67) r3 <<= 8
131: (71) r4 = *(u8 *)(r7 +6)
132: (4f) r3 |= r4
133: (67) r3 <<= 16
134: (4f) r3 |= r2
135: (67) r3 <<= 32
136: (4f) r3 |= r1

as you can see from comments above from bpf's assembler pov loop is executed at most once. in comparison if we would do the way we do in katran's code (by uncommenting 91 and commenting out line 90)

82: (73) *(u8 *)(r7 +5) = r1
83: (73) *(u8 *)(r7 +4) = r1
84: (73) *(u8 *)(r7 +3) = r1
85: (73) *(u8 *)(r7 +2) = r1
86: (73) *(u8 *)(r7 +1) = r1
87: (73) *(u8 *)(r7 +0) = r1
88: (b7) r1 = 16777343
89: (63) *(u32 *)(r7 +30) = r1
90: (69) r1 = *(u16 *)(r7 +14)
91: (69) r2 = *(u16 *)(r7 +16)
92: (0f) r2 += r1
93: (69) r1 = *(u16 *)(r7 +18)
94: (0f) r2 += r1
95: (69) r1 = *(u16 *)(r7 +20)
96: (0f) r2 += r1
97: (69) r1 = *(u16 *)(r7 +22)
98: (0f) r2 += r1
99: (69) r1 = *(u16 *)(r7 +26)
100: (0f) r2 += r1
101: (69) r1 = *(u16 *)(r7 +28)
102: (0f) r2 += r1
103: (07) r2 += 383
104: (bf) r1 = r2
105: (77) r1 >>= 16
106: (0f) r1 += r2
107: (a7) r1 ^= -1
108: (6b) *(u16 *)(r7 +24) = r1

you can see full unroll and everything is workign as expected.
if you would run tcpdump w/ -vvv flags from your version you will see that csum is not correct.
so yeah. your code doesnt work as you expect it to work

[sbernard31]

Thx a lot about the detailed explanation !

Playing with wireshark a bit, I validate that your way calculates valid checksum (and of course mine not)

But I was a bit confused about why "my code" is translated in this byte code... I mean : why it is unroll like there is at most one iteration in the loop?

I finally get that IHL is the size in 32 bit words, so the code I wanted to write :

iph->check = 0;
u32 csum = 0;
__u16 * next_iph_u16 = (__u16 *)iph;
for (int i = 0; i < iph->ihl *  2; i++) {
    csum += *next_iph_u16++;
}
iph->check = ~((csum & 0xffff) + (csum >> 16));

Fixing that, we get a more expected result as the byte code is now rejected by Bpf's verifier.

bpf: Failed to load program: Invalid argument
back-edge from insn 149 to 142

So a question remains, how could we calculate a checksum taking into account "variable size" IP options ? I mean, one of the XDP use case is about modifying packet and so calculate checksum should be a pretty standard use case ? Or maybe IP options is almost never used ?

(I totally aware that now my questions do not concern Katran directly, so do not hesitate to tell me if I abuse of your time or knowledge.)

[tehnerd]

"So a question remains, how could we calculate a checksum taking into account "variable size" IP options ?" by using bpf_csum_diff (see https://github.com/facebookincubator/katran/blob/master/katran/lib/bpf/balancer_helpers.h#L47 as example). but you need a kernel w/ support for this helper (it is not in 18.04 ubuntu yet).

"Or maybe IP options is almost never used ?" in real world on host-to-host packets they don't (there is some application for em for protocols, which are run between routers; but this is out of scope of this discussion)

[sbernard31]

Ok, I get it. Thx again for your time :)