Fix wrong csum for non-first ipv4 fragments

[liuyuan10]

Before this commit, natting are blindly changing l4 ports to non-first ipv4
fragments, causing wrong csum.

Also fixes the same issue during the rev natting for dsr.

Signed-off-by: Yuan Liu liuyuan@google.com

Fix natting of non-first ipv4 fragments.

[joestringer]

Thanks for the contribution!

Upon first review, I was confused why you would avoid NATing for second and subsequent fragments since the L3 addresses still need to be translated. However upon closer read, this PR is avoiding port translation (and corresponding checksum calculation for L4) on second/subsequent fragments. Please update the commit message and PR description to reflect this, it will help avoid confusion in future.

Do you have a reproducer to regression-test this issue? It seems like something we would want tests in the codebase for.

Misc additional cosmetic feedback below.

[liuyuan10]

Thanks for the contribution!

Upon first review, I was confused why you would avoid NATing for second and subsequent fragments since the L3 addresses still need to be translated. However upon closer read, this PR is avoiding port translation (and corresponding checksum calculation for L4) on second/subsequent fragments. Please update the commit message and PR description to reflect this, it will help avoid confusion in future.

Done

Do you have a reproducer to regression-test this issue? It seems like something we would want tests in the codebase for.

I was doing manual testing with a cluster with a udp_echo application. I don't know about regression-tests. do you have more info?

[joestringer]

I was doing manual testing with a cluster with a udp_echo application. I don't know about regression-tests. do you have more info?

@liuyuan10 We have fragmentation tests under test/k8sT/Services.go, so I was wondering if we could extend them to include a test that would validate the fix (and prevent us merging code that breaks it in future). The end-to-end testing framework we use is described in more detail here: https://docs.cilium.io/en/v1.8/contributing/testing/e2e/ .

[joestringer]

test-me-please

[liuyuan10]

I was doing manual testing with a cluster with a udp_echo application. I don't know about regression-tests. do you have more info?

@liuyuan10 We have fragmentation tests under test/k8sT/Services.go, so I was wondering if we could extend them to include a test that would validate the fix (and prevent us merging code that breaks it in future). The end-to-end testing framework we use is described in more detail here: https://docs.cilium.io/en/v1.8/contributing/testing/e2e/ .

I see. let me look into that. can we merge this PR first?

[joestringer]

let me look into that. can we merge this PR first?

Yep, once it passes the full CI.

Feel free to ignore the Cilium-Ginkgo-GKE job failure for now, there is an infrastructure issue. When we get an update on that being fixed, we can re-trigger that job.

[brb]

Great find! I'm wondering why it was not caught by the testIPv4FragmentSupport in test/k8sT/Services.go? /cc @qmonnet

[pchaigno]

The 4.9 build is failing because of a new complexity issue:

2020-10-09T18:52:24.130199902Z level=error msg="Command execution failed" cmd="[tc filter replace dev lxc_health egress prio 1 handle 1 bpf da obj 3350_next/bpf_lxc.o sec to-container]" error="exit status 1" subsys=datapath-loader
2020-10-09T18:52:24.130222938Z level=warning subsys=datapath-loader
2020-10-09T18:52:24.130225739Z level=warning msg="Prog section 'to-container' rejected: Argument list too long (7)!" subsys=datapath-loader
2020-10-09T18:52:24.130274563Z level=warning msg=" - Type:         3" subsys=datapath-loader
2020-10-09T18:52:24.130280055Z level=warning msg=" - Attach Type:  0" subsys=datapath-loader
2020-10-09T18:52:24.130282364Z level=warning msg=" - Instructions: 1716 (0 over limit)" subsys=datapath-loader
2020-10-09T18:52:24.130284500Z level=warning msg=" - License:      GPL" subsys=datapath-loader
2020-10-09T18:52:24.130286629Z level=warning subsys=datapath-loader
2020-10-09T18:52:24.130288654Z level=warning msg="Verifier analysis:" subsys=datapath-loader
2020-10-09T18:52:24.130290702Z level=warning subsys=datapath-loader
2020-10-09T18:52:24.130292748Z level=warning msg="Skipped 4880636 bytes, use 'verb' option for the full verbose log." subsys=datapath-loader
2020-10-09T18:52:24.130294873Z level=warning msg="[...]" subsys=datapath-loader
2020-10-09T18:52:24.130296926Z level=warning msg="6b) *(u16 *)(r10 -52) = r3" subsys=datapath-loader
2020-10-09T18:52:24.130299379Z level=warning msg="751: (67) r3 <<= 32" subsys=datapath-loader
[...]
2020-10-09T18:52:24.130522901Z level=warning msg="1681: (b7) r1 = 256" subsys=datapath-loader
2020-10-09T18:52:24.130525017Z level=warning msg="1682: (7b) *(u64 *)(r10 -120) = r1" subsys=datapath-loader
2020-10-09T18:52:24.130527142Z level=warning msg="BPF program is too large. Proccessed 98305 insn" subsys=datapath-loader

[qmonnet]

Great find! I'm wondering why it was not caught by the testIPv4FragmentSupport in test/k8sT/Services.go? /cc @qmonnet

In the tests, we check that fragments are effectively processed and sent to the backend by looking for updates on the counters in the CT map:

cmdOut := "cilium bpf ct list global | awk ..."

We don't check for valid checksums on the receiving end at the moment.

[qmonnet]

Not sure the revalidate_data() are necessary, but the rest of the code looks good, thank you!

[pchaigno]

Let's see if the complexity is still a concern first:
retest-4.9
Looks fixed 🎉

[pchaigno]

test-me-please

[qmonnet]

Thanks for the changes!