bpf: optmize builtin functions before we fallback to them by borkmann · Pull Request #11089 · cilium/cilium

borkmann · 2020-04-22T00:45:20Z

See commit msg.

joestringer · 2020-04-22T02:58:54Z

Huh, apparently one of the very few bpf/ unit tests triggered a failure on this:

test/bpf/unit-test
unit-test: ../../bpf/lib/ipv6_test.h:15: test_ipv6_addr_clear_suffix: Assertion `(__builtin_constant_p(v6.p1) ? ((__u32)( (((__u32)((__be32)(v6.p1)) & (__u32)0x000000ffUL) << 24) | (((__u32)((__be32)(v6.p1)) & (__u32)0x0000ff00UL) << 8) | (((__u32)((__be32)(v6.p1)) & (__u32)0x00ff0000UL) >> 8) | (((__u32)((__be32)(v6.p1)) & (__u32)0xff000000UL) >> 24))) : __builtin_bswap32(v6.p1)) == 0xffffffff' failed.

tklauser

Small nit: s/optmize/optimize/ in commit message and PR title.

aanm · 2020-04-23T10:07:38Z

this PR has been marked as a draft PR since it had a WIP label. Please click in "Ready for review" [below vvv ] once the PR is ready to be reviewed. CI will still run for draft PRs.

borkmann · 2020-04-24T00:04:36Z

test-me-please

borkmann · 2020-04-24T00:12:48Z

test-me-please

borkmann · 2020-04-24T00:20:39Z

test-me-please

coveralls · 2020-04-24T00:22:40Z

Coverage increased (+0.007%) to 44.768% when pulling fa17377 on pr/xdp-follow-ups-3 into 3ed89e4 on master.

borkmann · 2020-04-24T00:31:28Z

test-me-please

borkmann · 2020-04-24T00:38:04Z

test-me-please

pchaigno

This is great! One nit and a couple questions below.

Did you get a chance to check the complexity and program size impact? It looks like it could have a bigger impact than other mitigations we've discussed, and on all kernels 🎉

Convert all direct use of __builtin_mem{cpy,set}() over to the regular mem{cpy,set}() function as we're going to have a custom implementation of the latter two. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

While checking some of the xlated code from XDP side, I recently noticed __builtin_memcpy() patterns like: [...] 14: 55 04 ee 00 00 00 00 00 if r4 != 0 goto +238 <LBB0_2> 15: 71 32 2f 00 00 00 00 00 r2 = *(u8 *)(r3 + 47) 16: 67 02 00 00 08 00 00 00 r2 <<= 8 17: 71 31 2e 00 00 00 00 00 r1 = *(u8 *)(r3 + 46) 18: 4f 12 00 00 00 00 00 00 r2 |= r1 19: 7b 2a 78 ff 00 00 00 00 *(u64 *)(r10 - 136) = r2 20: 71 32 37 00 00 00 00 00 r2 = *(u8 *)(r3 + 55) 21: 67 02 00 00 08 00 00 00 r2 <<= 8 22: 71 31 36 00 00 00 00 00 r1 = *(u8 *)(r3 + 54) 23: 4f 12 00 00 00 00 00 00 r2 |= r1 24: 7b 2a b0 ff 00 00 00 00 *(u64 *)(r10 - 80) = r2 [...] This is bad since we end up doing byte-wise copy of data. LLVM is not aware of efficient unaligned access on x86-64 or arm64 etc so it cannot make any assumptions on the access. Implement optimized routines and make sure we don't blindly use pain __builtin_*() directly in the code. [...] 15: 79 31 38 00 00 00 00 00 r1 = *(u64 *)(r3 + 56) 16: 7b 1a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r1 17: 79 31 30 00 00 00 00 00 r1 = *(u64 *)(r3 + 48) 18: 7b 1a f0 ff 00 00 00 00 *(u64 *)(r10 - 16) = r1 19: 79 31 28 00 00 00 00 00 r1 = *(u64 *)(r3 + 40) 20: 7b 1a e8 ff 00 00 00 00 *(u64 *)(r10 - 24) = r1 21: 79 31 20 00 00 00 00 00 r1 = *(u64 *)(r3 + 32) 22: 7b 1a e0 ff 00 00 00 00 *(u64 *)(r10 - 32) = r1 23: 79 31 18 00 00 00 00 00 r1 = *(u64 *)(r3 + 24) 24: 7b 1a d8 ff 00 00 00 00 *(u64 *)(r10 - 40) = r1 [...] Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Place them into its own directory instead of directly into lib/ code. Adding more into lib/ would convolute it too much. Also add a Wno-builtin-declaration-mismatch for gcc. Tested via: # make unit-tests TESTPKGS= Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

borkmann · 2020-04-24T12:05:55Z

test-me-please

Add various new test cases for mem{cpy,set} adn run them as part of the bpf unit tests. I've added barrier_data() from [0] to prevent the compiler from any optimizations on the test data. Tested via: # make unit-tests TESTPKGS= [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7829fb09a2b4268b30dd9bc782fa5ebee278b137 Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

borkmann · 2020-04-24T12:49:56Z

test-me-please

borkmann · 2020-04-24T13:29:49Z

 98 9987M   98 9841M    0     0   104M      0  0:01:35  0:01:34  0:00:01  112M
 99 9987M   99 9953M    0     0   104M      0  0:01:35  0:01:35 --:--:--  112M
100 9987M  100 9987M    0     0   104M      0  0:01:35  0:01:35 --:--:--  112M
12:58:21  The machine index which stores all required information about
12:58:21  running Vagrant environments has become corrupt. This is usually
12:58:21  caused by external tampering of the Vagrant data folder.
12:58:21  
12:58:21  Vagrant cannot manage any Vagrant environments if the index is
12:58:21  corrupt. Please attempt to manually correct it. If you are unable
12:58:21  to manually correct it, then remove the data file at the path below.
12:58:21  This will leave all existing Vagrant environments "orphaned" and
12:58:21  they'll have to be destroyed manually.
12:58:21  
12:58:21  Path: /root/.vagrant.d/data/machine-index/index
12:58:21  box locked or unavailable, adding box from vagrant cloud
12:58:22  The machine index which stores all required information about
12:58:22  running Vagrant environments has become corrupt. This is usually
12:58:22  caused by external tampering of the Vagrant data folder.
12:58:22  
12:58:22  Vagrant cannot manage any Vagrant environments if the index is
12:58:22  corrupt. Please attempt to manually correct it. If you are unable
12:58:22  to manually correct it, then remove the data file at the path below.
12:58:22  This will leave all existing Vagrant environments "orphaned" and
12:58:22  they'll have to be destroyed manually.
12:58:22  
12:58:22  Path: /root/.vagrant.d/data/machine-index/index
Post stage

borkmann · 2020-04-24T13:33:09Z

test-with-kernel

qmonnet

Very nice set! Thanks

borkmann · 2020-04-24T15:50:56Z

restart-gke

borkmann · 2020-04-24T17:02:07Z

gke known hubble flake via #11141 (prior run before rebase was green on gke as well)

pchaigno · 2020-04-24T17:16:08Z

@cilium/ci is requested for review because of error in CODEOWNERS. #11125 should fix it.

borkmann added wip area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Apr 22, 2020

borkmann requested a review from a team April 22, 2020 00:45

pchaigno reviewed Apr 22, 2020

View reviewed changes

Comment thread bpf/include/bpf/builtins.h Outdated

borkmann force-pushed the pr/xdp-follow-ups-3 branch 6 times, most recently from 013032a to 16e925f Compare April 22, 2020 12:34

tklauser reviewed Apr 22, 2020

View reviewed changes

aanm marked this pull request as draft April 23, 2020 10:06

aanm removed the wip label Apr 23, 2020

borkmann force-pushed the pr/xdp-follow-ups-3 branch 2 times, most recently from f038a99 to 9a3a578 Compare April 24, 2020 00:02

borkmann marked this pull request as ready for review April 24, 2020 00:03

borkmann force-pushed the pr/xdp-follow-ups-3 branch from 9a3a578 to 5a1d08b Compare April 24, 2020 00:04

borkmann force-pushed the pr/xdp-follow-ups-3 branch from 5a1d08b to da30f2f Compare April 24, 2020 00:12

borkmann force-pushed the pr/xdp-follow-ups-3 branch from da30f2f to 83510c4 Compare April 24, 2020 00:20

borkmann force-pushed the pr/xdp-follow-ups-3 branch from 83510c4 to 1909dd1 Compare April 24, 2020 00:30

borkmann force-pushed the pr/xdp-follow-ups-3 branch from 1909dd1 to 8ffe63d Compare April 24, 2020 00:37

pchaigno requested changes Apr 24, 2020

View reviewed changes

Comment thread bpf/include/bpf/builtins.h Outdated

Comment thread bpf/include/bpf/builtins.h Outdated

Comment thread bpf/include/bpf/builtins.h

borkmann added 2 commits April 24, 2020 10:14

bpf: no more direct use of __builtin_mem{cpy,set} in bpf

b2a1804

Convert all direct use of __builtin_mem{cpy,set}() over to the regular mem{cpy,set}() function as we're going to have a custom implementation of the latter two. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

borkmann force-pushed the pr/xdp-follow-ups-3 branch from 8ffe63d to 742047a Compare April 24, 2020 08:24

borkmann requested review from a team as code owners April 24, 2020 12:05

borkmann force-pushed the pr/xdp-follow-ups-3 branch from fe9837d to fa17377 Compare April 24, 2020 12:49

borkmann added the pending-review label Apr 24, 2020

qmonnet approved these changes Apr 24, 2020

View reviewed changes

pchaigno approved these changes Apr 24, 2020

View reviewed changes

pchaigno merged commit 580f5b5 into master Apr 24, 2020

pchaigno deleted the pr/xdp-follow-ups-3 branch April 24, 2020 17:16

pchaigno mentioned this pull request Apr 25, 2020

test/bpf: Fix BPF unit tests #11158

Merged

qmonnet mentioned this pull request Apr 27, 2020

Reduce BPF program complexity for kernel 4.19 (ipv4+ipv6+debug=false+vxlan+kubeProxyReplacement=strict VS. fragment support) #11175

Closed

3 tasks

joestringer mentioned this pull request Apr 27, 2020

docs: updating contribution guide process #11174

Merged

pchaigno mentioned this pull request Dec 18, 2020

bpf: Fix misaligned access in bpf_xdp with prefilter and IPv6 #14447

Merged

Conversation

borkmann commented Apr 22, 2020

Uh oh!

joestringer commented Apr 22, 2020

Uh oh!

Uh oh!

tklauser left a comment

Choose a reason for hiding this comment

Uh oh!

aanm commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

coveralls commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

borkmann commented Apr 24, 2020

Uh oh!

pchaigno commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aanm commented Apr 23, 2020 •

edited

Loading

coveralls commented Apr 24, 2020 •

edited

Loading