Fix 5.10+ complexity issue with kubeProxyReplacement=disabled
#16084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pchaigno
added
kind/bug
maintainer-s-little-helper
bot
assigned jrfastab, michi-covalent and aditighag and unassigned michi-covalent
michi-covalent
approved these changes
May 11, 2021
borkmann
reviewed
May 12, 2021
borkmann
reviewed
May 12, 2021
mattr=+alu32, supported since LLVM 7.0 and implied by mcpu=v3, enables
the use of 32-bit registers in BPF bytecode. Enabling this compiler
option can however result in loading issues as illustrated below.
12: (61) r1 = *(u32 *)(r0 +80) // ctx->data_end
13: (61) r6 = *(u32 *)(r0 +76) // ctx->data
14: (bc) w7 = w6 // <- verifier looses track of inferred pkt type here.
[...]
38: (71) r1 = *(u8 *)(r7 +20)
R7 invalid mem access 'inv'
These errors typically happen because the data and data_end pointers are
actually 32-bit registers. Depending on how these pointers are used,
LLVM sometimes makes use of that assumption (e.g., 32-bit assignment on
instruction 14 above). The verifier is however not able to follow and
reject such programs.
We can usually work around those by ensuring these pointers are only
used via 64-bit types. This commit implements this wherever needed to
pass the verifier.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
Set mcpu=v3 in the compiler on kernels 5.10+ to use all available eBPF instructions and 32-bit registers. This change fixes the complexity issue we're hitting on v5.10+ when socket-level load balancing is disabled (via enable-host-services=false or kube-proxy-replacement=disabled). Using the third eBPF instruction set doesn't reduce complexity for all BPF programs but it leads to more standard numbers, with less variations in complexities. A big part of this improvement is due to the implicit use of mattr=+alu32 to enable 32-bit eBPF registers. In addition to the end-to-end test on bpf-next, this change was tested on kernels 5.10 and 5.11 with the existing verifier-test.sh, compiling the datapath with both KERNEL=netnext and KERNEL=419. Signed-off-by: Paul Chaignon <paul@cilium.io>
On master and with kernels 5.10+, we have a complexity issue when ENABLE_HOST_SERVICES_FULL is undefined (i.e., socket-level load balancing is disabled and additional code compiled in bpf_lxc as a replacement). Our verifier test included a workaround for that issue, by always defining ENABLE_HOST_SERVICES_FULL on newer kernels. This commit removes that workaround since the previous commit fixed the complexity issue. Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno
force-pushed
the
fix-5.10-complexity-issue
branch
from
5884637
to
552ec87
Compare
|
test-me-please |
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.10
in 1.10.0-rc2
maintainer-s-little-helper
bot
moved this from Backport pending to v1.10
to Needs backport from master
in 1.10.0-rc2
maintainer-s-little-helper
bot
moved this from Backport pending to v1.10
to Needs backport from master
in 1.10.0-rc2
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.10
in 1.10.0-rc2
maintainer-s-little-helper
bot
moved this from Backport pending to v1.10
to Backport done to v1.10
in 1.10.0-rc2
@brb It's on my radar to backport to both v1.9 and v1.8. I want to finish testing with the latest BPF security fixes first as those will be backported to all LTS kernels. Looks good so far, but I didn't run the full e2e tests yet. |
|
Marking for backport to v1.9. |
Is this not going to be backported to 1.8 after all? |
Probably not. It's a fair bit of effort to backport to stable branches because several other PRs need to be backported as well. v1.8 is also not supported since the release of v1.11. |
pchaigno
added
the
kind/complexity-issue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request fixes #14726, our complexity issue with
kubeProxyReplacement=disabledwhen running Linux 5.10+. The fix is less general than originally hoped and only applies to 5.10+; other complexity issues are therefore not fixed by this change. With a bit more work, we may able to extend it to 5.1+ in the future.See commits for details. As a summary:
mattr=+alu32.mcpu=v3(impliesmattr=+alu32) on kernels 5.10+.In addition to the end-to-end tests, these changes were also tested on kernels 5.10 and 5.11 with datapath configurations
KERNEL=419andKERNEL=netnext.