Custom labels 2 #4
Merged
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
This is necessary in some cases when we need the verifier to _not_ be able to predict the bounds of a variable, which can cause an explosion in the amount of state it needs to consider and prevent verification in under 1M instructions. See the comment for fuller details. This also requires setting the Clang target to `bpf` and passing the arm64/x86 defines manually (rather than relying on `-target` to set them), because otherwise the inline eBPF assembly doesn't work.
This commit is dead code until the user-mode code adds to cause it to be invoked. It expects to be passed a `GoCustomLabelsOffset` object in the `go_procs` map, which controls how it finds the custom labels map for a given PID.
If the `CollectCustomLabels` config flag is turned on, this parses Go binaries to determine their version number, then uses a hardcoded map of offsets by version to populate the `go_procs` map and instruct the unwinder to collect custom labels. In the future, we can automate updating this map, and also fall back to finding the labels via disassembly when a given version doesn't have them available. Those features are not yet present in this commit, however.
|
Which kernels does this verify on, and how many instructions is it? |
|
It's still failing on 4.19 and 5.4 which I am investigating now. |
|
This now verifies on all tested kernels except 4.19.x. Total instructions: 28892 |
|
So the issue is that 4.19 has a 4k instruction count limit right? I think it's fine to ignore 4.19. |
gnurizen
reviewed
Aug 7, 2024
| // We need to write this in assembly because the verifier doesn't understand | ||
| // that val_len has already been bounds-checked above, apparently | ||
| // because clang has spilled it to the stack rather than | ||
| // keeping it in a register. |
There was a problem hiding this comment.
Curious if you tried putting the "register" keyword on val_len?
There was a problem hiding this comment.
I didn't try, but I looked into it and apparently modern compilers simply ignore the register keyword: https://stackoverflow.com/questions/10675072/is-the-register-keyword-still-used
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.
Attempt 2 at custom labels PR.
This is broken up into individual PRs which should hopefully be independently digestible