Freeze .rodata before trying to load programs #1159
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
ti-mo
force-pushed
the
tb/make-rodata-const-again
branch
2 times, most recently
from
5f3a44d
to
792bcdb
Compare
|
Testing good on my side, thanks. |
lmb
reviewed
Oct 10, 2023
| // potentially pending creation. This is needed for frozen maps like .rodata | ||
| // that need to be finalized before invoking the verifier. | ||
| if !mapSpec.Type.canStoreMapOrProgram() { | ||
| if err := m.finalize(mapSpec); err != nil { |
There was a problem hiding this comment.
Can you remind me why we can't populate deferred maps at this point as well?
There was a problem hiding this comment.
Map fds need to be relocated into programs before a program can be loaded, but we can't populate a prog array without creating programs first. We've split up this process into multiple stages to keep things clear. Technically, we only need to defer populating prog arrays, but we can refactor this again later if new requirements pop up.
This test fails if .rodata is not frozen when freeze_rodata() is verified. Signed-off-by: Timo Beckers <timo@isovalent.com>
`cl.loadMap()` now finalizes maps that can be finalized right after creation, aka. 'scalar' maps without references to other bpf resources. Prog maps or outer maps are populated at a later stage. After doing a first pass over all requested Maps and Programs, a separate pass is run by `cl.populateMaps()` (renamed to `populateDeferredMaps()`) that only considers prog maps and outer maps, lazy-loads dependencies and fully resolves Spec.Contents, then finalizes the Map. This fixes .rodata being frozen after the verifier has run, which not only defeats the point of having constants, but also causes verifier errors if a runtime-provided constant is used as a return code or helper argument that must be proven to be within a certain range during verification. Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
force-pushed
the
tb/make-rodata-const-again
branch
from
792bcdb
to
ff77f82
Compare
lmb
approved these changes
Oct 10, 2023
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.
As reported in #1143 and #1156.
The change is fairly minimal, had to reindent most of
populateMaps().The gist of it is
cl.loadMap()now finalizes maps that can be finalized right after loading, aka. 'scalar' maps without references to other bpf resources. Prog maps or outer maps are skipped.After doing a first pass over all requested Maps and Programs, a separate pass is run by the renamed
populateDeferredMaps()that only considers prog maps and outer maps, lazy-loads dependencies and fully resolvesSpec.Contents, then finalizes the Map.