Add support for 64bit immediate with type 2 #820
Conversation
WalkthroughThe pull request introduces support for a new instruction type Changes
Possibly related issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)src/crab/ebpf_transformer.cpp (2)⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
external/libbtf(1 hunks)src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(8 hunks)src/asm_marshal.cpp(3 hunks)src/asm_ostream.cpp(2 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (29)
src/asm_files.cpp (7)
174-174: Assignment ofINST_LD_MODE_MAP_FD
inst.srcis correctly set toINST_LD_MODE_MAP_FDfor loading map file descriptors.
199-223: Implementation ofrelocate_global_variablefunctionFunction correctly handles the relocation of global variables.
350-350: Initialization ofglobal_variable_section_indicesDeclaration and initialization are appropriate.
370-377: Checking for old-style map sectionsLoop correctly identifies the presence of old-style map sections.
Line range hint
380-416: Handling BTF data and global variable sectionsProper handling of BTF data and initialization of section indices for
.maps,.data,.bss, and.rodata.
499-504: Relocating global variablesRelocation for global variables using
relocate_global_variableis correctly implemented.
477-479: Verifyinst_nextassignment logicEnsure that the calculation of
inst_nexthandles boundary conditions correctly.Review the assignment:
ebpf_inst& inst_next = offset / sizeof(ebpf_inst) + 1 < prog.prog.size() ? prog.prog[offset / sizeof(ebpf_inst) + 1] : inst;Confirm that when
offset / sizeof(ebpf_inst) + 1equalsprog.prog.size(),inst_nextcorrectly referencesinstto avoid out-of-bounds access.✅ Verification successful
Boundary condition handling is correct
The assignment logic safely handles the boundary case by falling back to
instwhen at the end of the program, preventing out-of-bounds access. This is consistent with the codebase's instruction handling patterns and is validated by the test suite.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Get the complete context of the function containing this code ast-grep --pattern 'function $FUNC_NAME($$$) { $$$ inst_next = $$$ $$$ }' # Search for similar patterns and usage of prog.prog rg "prog\.prog\[" -A 2 -B 2 # Look for test files fd "test.*cpp$"Length of output: 1097
Script:
#!/bin/bash # Check test_marshal.cpp for relevant test cases cat src/test/test_marshal.cpp # Look for verification functions rg "verify|validate" src/test/test_marshal.cpp -A 2 -B 2Length of output: 44773
src/test/test_yaml.cpp (1)
34-34: Addition ofmap.yamlto YAML test suiteNew test case
YAML_CASE("test-data/map.yaml")added to the test suite.src/ebpf_vm_isa.hpp (1)
91-93: Defining new load modesNew constants
INST_LD_MODE_IMM,INST_LD_MODE_MAP_FD, andINST_LD_MODE_MAP_VALUEare appropriately defined and adhere to the instruction set.src/asm_syntax.hpp (2)
96-103: LGTM: Clean struct definition.The struct follows the established pattern with appropriate member types and a defaulted equality operator.
261-261: LGTM: Proper variant type update.LoadMapAddress is correctly added to the Instruction variant type.
src/asm_marshal.cpp (3)
Line range hint
90-97: LGTM: Enhanced flexibility with type parameter.The function now supports multiple operation modes through the type parameter instead of a boolean flag.
108-108: LGTM: Proper mode constant usage.Correctly uses INST_LD_MODE_MAP_FD constant with the updated makeLddw function.
110-112: LGTM: Clean implementation of LoadMapAddress marshaling.The implementation properly handles both mapfd and offset parameters using the correct mode constant.
src/assertions.cpp (1)
48-48: LGTM: Consistent with LoadMapFd behavior.Correctly returns an empty vector of assertions, matching the pattern of similar operations.
src/asm_cfg.cpp (1)
402-403: LGTM: Consistent instruction type categorization.LoadMapAddress is correctly categorized as an "assign" type instruction, matching the behavior of similar operations.
src/asm_ostream.cpp (1)
362-363: LGTM!The string representation matches the assembly syntax defined by the MAP_VAL regex pattern.
src/asm_unmarshal.cpp (3)
Line range hint
432-441: LGTM!The validation logic for instruction fields is correct and comprehensive.
442-449: LGTM!The handling of map file descriptor mode is correct.
450-452: LGTM!The creation of LoadMapAddress instruction is correct.
src/test/test_verify.cpp (1)
427-427: Verify global variable support in the verifier.The change from TEST_SECTION_FAIL to TEST_SECTION indicates that global variables are now supported.
Run this script to verify global variable support:
✅ Verification successful
Global variable support is properly implemented
- Implementation in src/asm_files.cpp handles global variable relocation, section management (.data, .bss, .rodata), and validates operations
- The change from TEST_SECTION_FAIL to TEST_SECTION is consistent with the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if global variables are properly supported in the verifier # Test: Search for global variable handling code ast-grep --pattern $'class $_$ { $$$ handle_global_variable($_, $_) { $$$ } $$$ }'Length of output: 95
Script:
#!/bin/bash # Search for test files and code related to global variables echo "=== Test files with 'global' in name ===" fd -t f "global" --exec echo {} echo -e "\n=== Code mentions of global variables ===" rg -i "global.*variable" -A 2 echo -e "\n=== Global symbol handling ===" ast-grep --pattern 'global_symbol'Length of output: 2087
src/test/test_marshal.cpp (1)
44-45: LGTM: Added support for LDDW with src_reg=2.The addition of
{{0x18, DST, 2, 0, IMM}, bpf_conformance_groups_t::base64}to the instruction template table correctly implements support for 64-bit immediate with type 2.src/crab/ebpf_transformer.cpp (2)
1844-1854: Implementation correctly handles map address loading.The
do_load_map_addressmethod:
- Retrieves map descriptor and type
- Sets shared region size and offset
- Assigns valid pointer type (non-null)
1856-1857: LGTM: Simple delegation to do_load_map_address.The operator correctly forwards the call to the implementation method.
external/libbtf (1)
1-1: Verify the submodule commit updateThe submodule update needs verification to ensure it aligns with the 64-bit immediate instruction support.
Run this script to verify the commit:
✅ Verification successful
Submodule update aligns with 64-bit immediate support
The commit adds parsing support for data sections into array maps, which is consistent with the implementation of 64-bit immediate instruction support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the submodule commit exists and examine its changes # Check if the commit exists and is accessible git ls-remote https://github.com/vbpf/libbtf.git 6aba1609ce53e11639cecb4fa8750dd01d5ce209 # Get the commit message and changed files gh api repos/vbpf/libbtf/commits/6aba1609ce53e11639cecb4fa8750dd01d5ce209 --jq '.commit.message, .files[].filename'Length of output: 448
Script:
#!/bin/bash # Get the actual submodule URL and configuration cat .gitmodules git config --file .gitmodules --get-regexp url | grep libbtf git submodule status external/libbtfLength of output: 688
Script:
#!/bin/bash # Verify commit in the correct repository git ls-remote https://github.com/Alan-Jowett/libbtf.git 6aba1609ce53e11639cecb4fa8750dd01d5ce209 # Get commit details gh api repos/Alan-Jowett/libbtf/commits/6aba1609ce53e11639cecb4fa8750dd01d5ce209 --jq '.commit.message, .files[].filename'Length of output: 383
test-data/map.yaml (4)
1-2: License headers are properly formatted.
11-12: Document magic numbers and verify region size.
- Document the significance of 2147418112 in svalue array
- Confirm if shared_region_size=4 is sufficient for 64-bit operations mentioned in PR objectives
8-8: Verify map_val(1) existence.Add precondition to verify that map_val(1) exists before the test.
23-26: Verify map_fd postconditions.
- Document the significance of svalue=[1, 2147418112]
- Add assertions to verify map_fd validity
✅ Verification successful
Map FD value range [1, 2147418112] ensures valid map operations.
svalue=[1, 2147418112] represents valid map descriptor range:
- Lower bound 1: Ensures map_fd is valid (0 is invalid)
- Upper bound 2147418112 (0x7FFF_FFF0): Maximum system-allowed map FD
Built-in validation:
- Type system enforces map_fd type constraints
- Range checks prevent invalid map operations
- System validates map descriptors during creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for map_fd validation logic rg "map_fd.*validation|verify" -A 5Length of output: 14496
Script:
#!/bin/bash # Search for map-related code and documentation echo "=== Map Creation and Initialization ===" ast-grep --pattern 'create_map' echo -e "\n=== Map Descriptor and FD Handling ===" rg "map.*descriptor|map.*fd" -A 3 echo -e "\n=== Map Value Documentation ===" rg -l "\.md$" | xargs rg "map.*value|map.*fd"Length of output: 71240
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/asm_files.cpp(7 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_verify.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (7)
src/ebpf_vm_isa.hpp (1)
90-93: LGTM! Well-defined instruction load mode constants.The new constants follow a logical sequence and are well-documented.
src/asm_files.cpp (4)
174-174: LGTM! Correct use of the new load mode constant.The source register is properly set to INST_LD_MODE_MAP_FD for map relocations.
199-223: LGTM! Well-structured global variable relocation handling.The function properly handles global variable relocations with appropriate error checking and uses the correct load mode constant.
368-374: LGTM! Clear separation of old-style map section detection.The code cleanly identifies old-style map sections before processing.
400-414: LGTM! Comprehensive global variable section handling.All standard global variable sections (.data, .bss, .rodata) are properly tracked.
src/test/test_verify.cpp (2)
528-528: Move this test case to the appropriate section.Move this line down to around line 519 where the other
TEST_SECTION("build", ...)lines are grouped.
528-528: Verify the change in test expectation.The test case has been changed from
TEST_SECTION_FAILtoTEST_SECTION, indicating that global variables are now expected to pass verification.Run the following script to verify the change:
✅ Verification successful
Test change from TEST_SECTION_FAIL to TEST_SECTION is correct
The codebase shows proper implementation of global variable support, including section handling (.data, .bss, .rodata) and relocation logic. The test expectation change reflects this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the change in global variable handling. # Test: Search for any related changes in global variable handling. rg -A 5 'global_variable' --type cpp --type hpp # Test: Search for any related test cases. rg -A 5 'TEST_SECTION.*global_variable'Length of output: 618
Script:
#!/bin/bash # Find the TEST_SECTION_FAIL occurrences and macro definitions rg -A 2 'TEST_SECTION_FAIL' # Find the test macro definitions rg -B 2 -A 5 '#define TEST_SECTION' # Look for global variable related changes in recent commits git log -n 5 --oneline --grep="global.*variable"Length of output: 7945
Script:
#!/bin/bash # Search for global variable verification implementation ast-grep --pattern 'global' src/ # Search for recent changes in verifier code rg -l 'global.*variable' src/Length of output: 107
Script:
#!/bin/bash # Check implementation in asm_files.cpp rg -B 3 -A 3 'global.*variable' src/asm_files.cpp # Look for recent changes in the file git log -p src/asm_files.cpp | rg -B 3 -A 3 'global'Length of output: 1651
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/asm_files.cpp(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (1)
src/asm_files.cpp (1)
501-504: Potential inconsistency in symbol namesIn
relocate_global_variable, usingreader.sections[symbol_section_index]->get_name()assymbol_namemay not match the originalsymbol_nameused elsewhere. Verify that the correct symbol name is passed to maintain consistency.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/asm_files.cpp(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (5)
src/asm_files.cpp (5)
165-170: LGTM! Well-structured helper function.The function effectively centralizes load instruction validation logic, reducing code duplication.
Line range hint
172-203: LGTM! Improved error handling and clarity.The function is now more robust with better error messages and clearer parameter naming.
355-355: LGTM! Comprehensive global variable section handling.The implementation correctly handles all standard global variable sections (.data, .bss, .rodata).
Also applies to: 411-421
Line range hint
481-516: LGTM! Robust relocation handling.The relocation logic is well-structured with appropriate error checking and clear separation between map and global variable handling.
205-214:⚠️ Potential issueAdd bounds check for next instruction access.
The function assumes the next instruction exists but doesn't verify this assumption before accessing it.
Apply this diff to add bounds checking:
verify_load_instruction(reloc_inst, symbol_name, offset); + // Verify next instruction exists as this is a wide instruction + if (offset / sizeof(ebpf_inst) + 1 >= info.map_descriptors.size()) { + throw UnmarshalError("Missing next instruction for wide load at offset " + + std::to_string(offset / sizeof(ebpf_inst))); + } // Copy the immediate value to the next instruction. next_reloc_inst.imm = reloc_inst.imm;Likely invalid or redundant comment.
Alan-Jowett
force-pushed
the
load_map_address
branch
from
0eec122 to
f863c07
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/asm_files.cpp (1)
499-504:⚠️ Potential issueEnsure
next_reloc_instis initialized before useBefore calling
relocate_global_variable,next_reloc_instshould be properly defined to prevent undefined behavior. Currently,next_reloc_instmay not be initialized whenrelocate_global_variableis called.Apply this diff to define
next_reloc_instbefore callingrelocate_global_variable:ebpf_inst& reloc_inst = prog.prog[offset / sizeof(ebpf_inst)]; + // Load instructions are two instructions long, so we need to check the next instruction. + if (prog.prog.size() <= offset / sizeof(ebpf_inst) + 1) { + throw UnmarshalError("Invalid relocation data"); + } + ebpf_inst& next_reloc_inst = prog.prog[offset / sizeof(ebpf_inst) + 1]; if (map_section_indices.contains(symbol_section_index)) { relocate_map(reloc_inst, symbol_name, map_record_size_or_map_offsets, info, offset, index, symbols); continue; } if (global_variable_section_indices.contains(symbol_section_index)) { relocate_global_variable(reloc_inst, next_reloc_inst, reader.sections[symbol_section_index]->get_name(), info, map_record_size_or_map_offsets, offset); continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
external/libbtf(1 hunks)src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(7 hunks)src/asm_marshal.cpp(3 hunks)src/asm_ostream.cpp(2 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (3)
src/asm_parse.cpp (3)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:177-179
Timestamp: 2025-01-08T16:39:55.117Z
Learning: The ASM parser in src/asm_parse.cpp is only used for parsing hand-written assembly snippets from YAML test files, not for parsing untrusted input, so input validation is unnecessary.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing (parse_instruction) is specifically used in src/test/ebpf_yaml.cpp for parsing controlled test inputs from YAML files in the test-data directory, and is not exposed to external inputs that would require validation.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing is only used for parsing hand-written test snippets in YAML files, not for parsing arbitrary input from external sources. Therefore, extensive input validation is not required.
test-data/map.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: test-data/map.yaml:20-20
Timestamp: 2025-01-08T16:32:48.309Z
Learning: The eBPF verifier test framework does not support testing invalid file descriptors, making it impossible to add test cases for invalid map_fd values or non-existent file descriptors.
src/asm_files.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_files.cpp:205-229
Timestamp: 2025-01-08T17:37:07.223Z
Learning: Global variable relocations in eBPF are only supported with the new map types that use BTF data, not with the old-style map sections.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (23)
src/ebpf_vm_isa.hpp (1)
91-93: New load mode constants definedConstants
INST_LD_MODE_IMM,INST_LD_MODE_MAP_FD, andINST_LD_MODE_MAP_VALUEare added to represent new load modes.src/asm_syntax.hpp (1)
96-104:LoadMapAddressstruct integrated into instruction setThe
LoadMapAddressstruct is defined and included in theInstructionvariant to support loading map addresses.src/assertions.cpp (1)
48-48: LGTM!The empty assertion vector for LoadMapAddress is consistent with LoadMapFd's behavior.
src/asm_cfg.cpp (1)
402-403: LGTM!Categorizing LoadMapAddress as "assign" type is consistent with LoadMapFd handling.
src/asm_parse.cpp (2)
57-62: LGTM!The regex patterns clearly define the expected formats for map value and map file descriptor instructions.
173-179: LGTM!The instruction parsing logic correctly creates LoadMapAddress and LoadMapFd instructions from the matched patterns.
src/crab/ebpf_transformer.cpp (3)
47-47: LGTM!The declaration follows the class's operator overloading pattern and is well-placed with other map-related operations.
1844-1854: LGTM!The implementation correctly:
- Retrieves map information
- Sets up shared memory region
- Initializes type and pointer validity
1856-1856: LGTM!The operator follows the single responsibility principle by delegating to the implementation method.
external/libbtf (1)
1-1: Verify the submodule update impact.The submodule update needs verification to ensure compatibility with the new 64-bit immediate support.
Run this script to analyze the changes between the old and new commits:
✅ Verification successful
Submodule update appears safe
The libbtf integration is straightforward, using standard BTF interfaces for debug information, line numbers, and map definitions. No version-specific dependencies found that could be affected by the update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the submodule update impact # Store the commit hashes OLD_COMMIT="3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7" NEW_COMMIT="6aba1609ce53e11639cecb4fa8750dd01d5ce209" # Check if the submodule exists and is initialized if [ -d "external/libbtf/.git" ]; then cd external/libbtf # Fetch the commits if they don't exist git fetch origin $OLD_COMMIT $NEW_COMMIT # Show commit messages between the two versions echo "Commit messages between versions:" git log --oneline $OLD_COMMIT..$NEW_COMMIT # Show breaking changes in public APIs echo -e "\nPotential breaking changes in public headers:" git diff $OLD_COMMIT..$NEW_COMMIT -- '*.h' # Show changes in build system files echo -e "\nBuild system changes:" git diff $OLD_COMMIT..$NEW_COMMIT -- '*CMakeLists.txt' '*.cmake' 'Makefile*' fiLength of output: 141
Script:
#!/bin/bash # Check submodule directory contents and integration echo "Checking submodule directory contents:" fd . external/libbtf -t f echo -e "\nSearching for libbtf references in build files:" rg -g 'CMakeLists.txt' -g 'Makefile' -g '*.cmake' 'libbtf' echo -e "\nSearching for libbtf usage in source files:" rg -t cpp -t c -t h 'libbtf|btf_' -A 2Length of output: 10421
test-data/map.yaml (2)
4-14: LGTM! Comprehensive test case for map value loading.The test case thoroughly verifies the map value loading functionality, including offset, region size, value array, type, and value matching.
16-26: LGTM! Comprehensive test case for map file descriptor loading.The test case thoroughly verifies the map file descriptor loading functionality, including fd value, type, and value array.
src/test/test_yaml.cpp (1)
34-34: LGTM! Test case added in correct order.The map.yaml test case is properly integrated into the test suite.
src/asm_marshal.cpp (4)
90-93: LGTM! Improved type safety with explicit mode constants.The change from boolean flag to type parameter enhances code clarity and maintainability.
108-108: LGTM! Consistent use of mode constant.The operator correctly uses INST_LD_MODE_MAP_FD, aligning with the new type system.
110-112: LGTM! Well-structured LoadMapAddress operator.The operator follows the established pattern and correctly uses INST_LD_MODE_MAP_VALUE.
311-313: LGTM! Correct instruction size handling.LoadMapAddress is properly handled as a double-word instruction.
src/asm_ostream.cpp (2)
362-363: LGTM! Clear and consistent output format.The operator generates readable assembly syntax for LoadMapAddress instructions.
556-558: LGTM! Consistent size handling.LoadMapAddress size handling matches the implementation in asm_marshal.cpp.
src/asm_unmarshal.cpp (3)
Line range hint
432-441: LGTM! Improved validation logic.The validation correctly handles the extended range of source types with clear error messages.
442-449: LGTM! Clear LoadMapFd handling.The code properly validates and creates LoadMapFd instructions with helpful comments.
450-452: LGTM! Complete LoadMapAddress handling.The code properly creates LoadMapAddress instructions with all necessary fields.
src/test/test_marshal.cpp (1)
44-45: LGTM! Template addition aligns with ISA RFC.The new instruction template for LDDW with src_reg=2 is correctly placed in sequence.
Alan-Jowett
force-pushed
the
load_map_address
branch
2 times, most recently
from
831cc0a to
23edeee
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/asm_files.cpp (1)
195-195: 🧹 Nitpick (assertive)Include instruction offset in error messages
Include the instruction offset in the error messages to aid in debugging.
Apply this diff:
- throw UnmarshalError("Map descriptor not found for symbol " + symbol_name); + throw UnmarshalError("Map descriptor not found for symbol " + symbol_name + " at offset " + std::to_string(offset / sizeof(ebpf_inst)));Also applies to: 222-222
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
external/libbtf(1 hunks)src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(7 hunks)src/asm_marshal.cpp(3 hunks)src/asm_ostream.cpp(2 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (4)
src/test/test_marshal.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/test/test_marshal.cpp:395-397
Timestamp: 2025-01-08T20:19:15.070Z
Learning: Test coverage improvements should be handled in separate PRs to maintain focused changes, as demonstrated in PR #820 where additional test cases for LoadMapAddress were deferred.
src/asm_parse.cpp (3)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing (parse_instruction) is specifically used in src/test/ebpf_yaml.cpp for parsing controlled test inputs from YAML files in the test-data directory, and is not exposed to external inputs that would require validation.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:177-179
Timestamp: 2025-01-08T16:39:55.117Z
Learning: The ASM parser in src/asm_parse.cpp is only used for parsing hand-written assembly snippets from YAML test files, not for parsing untrusted input, so input validation is unnecessary.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing is only used for parsing hand-written test snippets in YAML files, not for parsing arbitrary input from external sources. Therefore, extensive input validation is not required.
test-data/map.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: test-data/map.yaml:20-20
Timestamp: 2025-01-08T16:32:48.309Z
Learning: The eBPF verifier test framework does not support testing invalid file descriptors, making it impossible to add test cases for invalid map_fd values or non-existent file descriptors.
src/asm_files.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_files.cpp:205-229
Timestamp: 2025-01-08T17:37:07.223Z
Learning: Global variable relocations in eBPF are only supported with the new map types that use BTF data, not with the old-style map sections.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (26)
src/test/test_yaml.cpp (1)
34-34: Test case added correctlyThe new test case for
test-data/map.yamlis properly added and enhances test coverage.src/asm_syntax.hpp (2)
96-103: LGTM: Clean struct definition for LoadMapAddress.The struct follows the established pattern with appropriate member types and a default equality operator.
261-261: LGTM: Proper integration into Instruction variant.LoadMapAddress is correctly added to the variant type while maintaining alphabetical ordering.
src/asm_marshal.cpp (3)
Line range hint
90-97: LGTM: Enhanced makeLddw signature for flexible operation modes.The parameter change from bool to uint8_t enables support for multiple operation modes, aligning with the 64-bit immediate support requirements.
110-112: LGTM: Clean LoadMapAddress marshaling implementation.The operator correctly uses makeLddw with appropriate mode and parameters.
311-313: LGTM: Consistent size handling for LoadMapAddress.The size of 2 matches other similar instruction types like LoadMapFd.
src/assertions.cpp (1)
48-48: LGTM: Consistent assertion handling for LoadMapAddress.The empty assertion set matches LoadMapFd's behavior, which is appropriate for this instruction type.
src/asm_cfg.cpp (1)
402-403: LGTM: Proper instruction type categorization for LoadMapAddress.Categorizing LoadMapAddress as "assign" is consistent with LoadMapFd and other similar instructions.
src/asm_ostream.cpp (2)
362-363: LGTM!The operator implementation correctly formats the LoadMapAddress instruction output.
556-558: LGTM!The size() function correctly returns 2 for LoadMapAddress instructions, matching the LDDW instruction format.
src/asm_unmarshal.cpp (3)
Line range hint
432-441: LGTM!The validation logic correctly checks src field values and offset field.
442-449: LGTM!The LoadMapFd handling is correctly implemented.
450-452: LGTM!The LoadMapAddress handling is correctly implemented.
src/test/test_verify.cpp (1)
528-528: LGTM!Test case updated to expect successful verification of global_variable.o.
src/test/test_marshal.cpp (2)
44-45: LGTM!Added instruction template for LDDW with src_reg = 2.
395-397: LGTM!Basic test coverage for LoadMapAddress instruction.
Run the following script to verify test coverage:
✅ Verification successful
Test coverage for LoadMapAddress is sufficient
The test covers marshaling and unmarshaling of all fields in the LoadMapAddress struct. The implementation is straightforward with no complex edge cases requiring additional tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for LoadMapAddress instruction. # Check if there are any untested code paths rg -A 5 'LoadMapAddress' | grep -v 'test'Length of output: 4496
src/crab/ebpf_transformer.cpp (3)
117-117: Method declaration follows class conventions.The declaration is properly placed with other similar methods in the private section.
1848-1852: Ensure proper initialization of shared region.The shared region size and offset are correctly set based on the map descriptor.
1844-1854: Verify map descriptor and type access.The implementation needs bounds checking for the map file descriptor and offset.
Run this script to check map descriptor access patterns:
✅ Verification successful
Map descriptor access is properly validated.
The implementation is safe through platform-level validation. Map descriptor access is protected by:
- Type system validation of map_fd
- Platform bounds checking in get_map_descriptor_linux()
- Runtime descriptor validation in find_map_descriptor()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if map descriptor access is validated before use # Look for validation patterns before accessing map descriptors ast-grep --pattern 'if ($_ < 0) { $$$; return; } const EbpfMapDescriptor& desc = $_->get_map_descriptor($_);'Length of output: 113
Script:
#!/bin/bash # Search for all map descriptor access patterns ast-grep --pattern 'get_map_descriptor($_)' # Search for mapfd validation patterns rg "mapfd|map_fd" -A 2 -B 2 # Look for error handling around map operations rg "throw.*map|return.*map.*error" -A 1Length of output: 54322
external/libbtf (1)
1-1: Verify the submodule update impact.The submodule update might introduce breaking changes.
Run this script to analyze the changes between the old and new commits:
✅ Verification successful
Submodule update is backward compatible
The changes add support for global variable sections while maintaining existing map parsing functionality. No breaking changes detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze changes in the libbtf submodule between commits. # Check if the submodule exists and contains both commits cd external/libbtf 2>/dev/null || { echo "Error: Submodule directory not found" exit 1 } # Fetch the commit details git cat-file -e 3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7 2>/dev/null && \ git cat-file -e 760a80490a4c67ca03046797868691cb81454d81 2>/dev/null && { # Show commit messages and changed files between the commits git log --oneline --no-merges 3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7..760a80490a4c67ca03046797868691cb81454d81 echo "Changed files:" git diff --name-status 3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7..760a80490a4c67ca03046797868691cb81454d81 } || echo "Error: One or both commits not found"Length of output: 162
Script:
#!/bin/bash # Initialize and update the submodule before analyzing changes git submodule init external/libbtf git submodule update external/libbtf cd external/libbtf || exit 1 git fetch --all git log --oneline --no-merges 3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7..760a80490a4c67ca03046797868691cb81454d81 || echo "Failed to get commit history" echo "Changed files:" git diff --name-status 3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7..760a80490a4c67ca03046797868691cb81454d81 || echo "Failed to get changes"Length of output: 863
Script:
#!/bin/bash cd external/libbtf || exit 1 echo "=== Changes in libbtf/btf_map.cpp ===" git diff 3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7..760a80490a4c67ca03046797868691cb81454d81 -- libbtf/btf_map.cpp echo -e "\n=== Changes in test/test.cpp ===" git diff 3c0cbd6ca0920e4d5df6fb88a892f0b5ac5425d7..760a80490a4c67ca03046797868691cb81454d81 -- test/test.cppLength of output: 7337
test-data/map.yaml (2)
4-14: LGTM!The test case thoroughly verifies the map address loading operation with appropriate postconditions.
16-26: LGTM!The test case thoroughly verifies the map file descriptor loading operation with appropriate postconditions.
src/asm_parse.cpp (4)
57-58: LGTM!The regex pattern correctly matches the map_val(fd) + offset format used in the test cases.
60-61: LGTM!The regex pattern correctly matches the map_fd fd format used in the test cases.
173-176: LGTM!The parsing logic correctly extracts and converts the components for LoadMapAddress instruction.
177-179: LGTM!The parsing logic correctly extracts and converts the components for LoadMapFd instruction.
Alan-Jowett
force-pushed
the
load_map_address
branch
from
23edeee to
462cc3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
external/libbtf(1 hunks)src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(7 hunks)src/asm_marshal.cpp(3 hunks)src/asm_ostream.cpp(2 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (4)
src/test/test_marshal.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/test/test_marshal.cpp:395-397
Timestamp: 2025-01-08T20:19:15.070Z
Learning: Test coverage improvements should be handled in separate PRs to maintain focused changes, as demonstrated in PR #820 where additional test cases for LoadMapAddress were deferred.
src/asm_parse.cpp (3)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:177-179
Timestamp: 2025-01-08T16:39:55.117Z
Learning: The ASM parser in src/asm_parse.cpp is only used for parsing hand-written assembly snippets from YAML test files, not for parsing untrusted input, so input validation is unnecessary.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing (parse_instruction) is specifically used in src/test/ebpf_yaml.cpp for parsing controlled test inputs from YAML files in the test-data directory, and is not exposed to external inputs that would require validation.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing is only used for parsing hand-written test snippets in YAML files, not for parsing arbitrary input from external sources. Therefore, extensive input validation is not required.
test-data/map.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: test-data/map.yaml:20-20
Timestamp: 2025-01-08T16:32:48.309Z
Learning: The eBPF verifier test framework does not support testing invalid file descriptors, making it impossible to add test cases for invalid map_fd values or non-existent file descriptors.
src/asm_files.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_files.cpp:205-229
Timestamp: 2025-01-08T17:37:07.223Z
Learning: Global variable relocations in eBPF are only supported with the new map types that use BTF data, not with the old-style map sections.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (25)
src/asm_files.cpp (2)
165-170: Refactored opcode check into helper functionThe introduction of
verify_load_instructionreduces code duplication and improves maintainability.
205-229: Ensure validity ofnext_reloc_instIn
relocate_global_variable, ensure thatnext_reloc_instis a valid instruction. Although the caller checks for the instruction length before passingnext_reloc_inst, consider adding an assertion or comment to clarify this assumption within the function.src/test/test_yaml.cpp (1)
34-34: Test case added for map functionalityIncluding
YAML_CASE("test-data/map.yaml")enhances test coverage for map-related features.src/ebpf_vm_isa.hpp (1)
91-93: Avoid naming confusion betweenINST_MODE_IMMandINST_LD_MODE_IMM
INST_LD_MODE_IMMmay be confused withINST_MODE_IMM, as both constants have similar names but different purposes. Consider renamingINST_LD_MODE_IMMto enhance clarity.src/asm_syntax.hpp (1)
261-261: LGTM!The
LoadMapAddressinstruction is correctly integrated into theInstructionvariant type.src/asm_marshal.cpp (3)
90-93: LGTM!The function signature change from boolean to type constant improves code clarity and extensibility.
110-112: LGTM!The
LoadMapAddressoperator implementation correctly marshals the instruction.
311-313: LGTM!The size function correctly handles the
LoadMapAddressinstruction type.src/assertions.cpp (1)
48-48: LGTM!The
LoadMapAddressoperator correctly returns an empty vector, consistent withLoadMapFdbehavior.src/asm_cfg.cpp (1)
402-403: LGTM!The
LoadMapAddressinstruction is correctly categorized as "assign" type.src/asm_parse.cpp (2)
173-176: LGTM!The LoadMapAddress instruction creation correctly extracts the destination register, map file descriptor and offset from the regex matches.
177-179: LGTM!The LoadMapFd instruction creation correctly extracts the destination register and map file descriptor from the regex matches.
src/asm_ostream.cpp (2)
362-363: LGTM!The operator correctly formats the LoadMapAddress instruction back to its assembly representation.
556-558: LGTM!The size function correctly returns 2 for LoadMapAddress instructions, consistent with other similar instructions like LoadMapFd.
src/asm_unmarshal.cpp (2)
Line range hint
432-441: LGTM!The validation logic correctly checks:
- inst.src against INST_LD_MODE_MAP_VALUE
- inst.offset is zero
- inst.dst is within valid register range
442-452: LGTM!The instruction creation logic correctly handles:
- LoadMapFd when inst.src is INST_LD_MODE_MAP_FD
- LoadMapAddress when inst.src is INST_LD_MODE_MAP_VALUE
src/test/test_verify.cpp (1)
528-528: LGTM!The test case update reflects that global_variable.o now passes verification.
src/test/test_marshal.cpp (1)
395-397: Test case only covers basic functionality.The test case only verifies basic marshaling/unmarshaling. However, based on the learnings from past reviews, additional test coverage should be handled in a separate PR.
src/crab/ebpf_transformer.cpp (3)
47-47: LGTM!The LoadMapAddress operator is correctly declared in the operator list.
1856-1856: LGTM!The LoadMapAddress operator correctly forwards to do_load_map_address.
1844-1854: 🧹 Nitpick (assertive)Add validation for mapfd and offset parameters.
The implementation directly uses mapfd and offset without validation. Consider adding parameter validation to ensure they are within valid ranges.
void ebpf_transformer::do_load_map_address(const Reg& dst_reg, const int mapfd, int32_t offset) { + // Validate parameters + if (mapfd < 0) { + m_inv.set_to_bottom(); + return; + } + const EbpfMapDescriptor& desc = thread_local_program_info->platform->get_map_descriptor(mapfd); const EbpfMapType& type = thread_local_program_info->platform->get_map_type(desc.type); // Set the shared region size and offset for the map. type_inv.assign_type(m_inv, dst_reg, T_SHARED); const reg_pack_t& dst = reg_pack(dst_reg); m_inv.assign(dst.shared_offset, offset); m_inv.assign(dst.shared_region_size, desc.value_size); assign_valid_ptr(dst_reg, false); }Likely invalid or redundant comment.
external/libbtf (1)
1-1: Verify the libbtf dependency update.Let's check the changes between the old and new commits to ensure compatibility with the LoadMapAddress functionality.
test-data/map.yaml (3)
1-3: LGTM!Standard MIT license header with correct attribution.
16-26: LGTM! Noted framework limitations for FD testing.Test case correctly verifies map FD loading functionality within framework constraints.
11-11: Verify the shared_region_size value.The shared_region_size=4 seems restrictive. Verify if this size is sufficient for all potential map value operations.
Alan-Jowett
force-pushed
the
load_map_address
branch
from
e7cf7b7 to
8373d84
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(7 hunks)src/asm_marshal.cpp(3 hunks)src/asm_ostream.cpp(2 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (4)
src/test/test_marshal.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/test/test_marshal.cpp:395-397
Timestamp: 2025-01-08T20:19:15.070Z
Learning: Test coverage improvements should be handled in separate PRs to maintain focused changes, as demonstrated in PR #820 where additional test cases for LoadMapAddress were deferred.
test-data/map.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: test-data/map.yaml:20-20
Timestamp: 2025-01-08T16:32:48.309Z
Learning: The eBPF verifier test framework does not support testing invalid file descriptors, making it impossible to add test cases for invalid map_fd values or non-existent file descriptors.
src/asm_parse.cpp (3)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing (parse_instruction) is specifically used in src/test/ebpf_yaml.cpp for parsing controlled test inputs from YAML files in the test-data directory, and is not exposed to external inputs that would require validation.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:177-179
Timestamp: 2025-01-08T16:39:55.117Z
Learning: The ASM parser in src/asm_parse.cpp is only used for parsing hand-written assembly snippets from YAML test files, not for parsing untrusted input, so input validation is unnecessary.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing is only used for parsing hand-written test snippets in YAML files, not for parsing arbitrary input from external sources. Therefore, extensive input validation is not required.
src/asm_files.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_files.cpp:205-229
Timestamp: 2025-01-08T17:37:07.223Z
Learning: Global variable relocations in eBPF are only supported with the new map types that use BTF data, not with the old-style map sections.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, library)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (22)
src/asm_files.cpp (1)
412-421:⚠️ Potential issueCheck for the existence of global variable sections before insertion
When inserting into
global_variable_section_indices, ensure that the section pointers returned byreader.sections[".data"],.bss, and.rodataare not null to prevent potential null pointer dereferences.Apply this diff to add null checks:
if (reader.sections[".maps"]) { map_section_indices.insert(reader.sections[".maps"]->get_index()); } - if (reader.sections[".data"]) { + if (reader.sections[".data"] != nullptr) { global_variable_section_indices.insert(reader.sections[".data"]->get_index()); } - if (reader.sections[".bss"]) { + if (reader.sections[".bss"] != nullptr) { global_variable_section_indices.insert(reader.sections[".bss"]->get_index()); } - if (reader.sections[".rodata"]) { + if (reader.sections[".rodata"] != nullptr) { global_variable_section_indices.insert(reader.sections[".rodata"]->get_index()); }Likely invalid or redundant comment.
src/test/test_yaml.cpp (1)
34-34: Ensure the new YAML test case is validVerify that
test-data/map.yamlexists and contains valid test cases to prevent test failures.src/ebpf_vm_isa.hpp (1)
91-93: Prevent naming confusion betweenINST_MODE_IMMandINST_LD_MODE_IMMConsider renaming
INST_LD_MODE_IMMto avoid confusion withINST_MODE_IMM, as they serve different purposes but have similar names.src/asm_syntax.hpp (2)
261-261: LGTM!LoadMapAddress is correctly added to the Instruction variant type.
96-103: 🧹 Nitpick (assertive)Add bounds documentation for offset field.
Document the valid range for the offset field and any alignment requirements.
// Load the address of a map value into a register. struct LoadMapAddress { Reg dst; // Destination register to store the address of the map value. int32_t mapfd{}; // File descriptor of the map to load the address from. - int32_t offset{}; // Offset within the map, must be within bounds. + int32_t offset{}; // Offset within the map value in bytes. Must be non-negative and + // aligned to the access size. The sum of offset and access size + // must not exceed the map value size.Likely invalid or redundant comment.
src/asm_marshal.cpp (2)
90-93: LGTM!The change from bool to uint8_t for the type parameter improves type safety and extensibility.
110-112: LGTM!The LoadMapAddress operator implementation correctly marshals the instruction.
src/assertions.cpp (1)
48-48: LGTM!The LoadMapAddress operator correctly returns no assertions, consistent with LoadMapFd behavior.
src/asm_cfg.cpp (1)
402-403: LGTM!LoadMapAddress is correctly categorized as an "assign" instruction type.
src/asm_parse.cpp (3)
173-176: LGTM! The LoadMapAddress instruction creation looks correct.The implementation properly extracts the destination register, map file descriptor, and offset from the regex match groups.
177-179: LGTM! The LoadMapFd instruction creation looks correct.The implementation properly extracts the destination register and map file descriptor from the regex match groups.
57-62: 🧹 Nitpick (assertive)Add comments to explain the regex pattern constraints.
The regex patterns need documentation to explain:
- MAP_VAL: Only matches positive integers for fd and offset
- MAP_FD: Only matches positive integers for fd
// Match map_val(fd) + offset +// fd and offset must be positive integers #define MAP_VAL R"_(\s*map_val\((\d+)\)\s*\+\s*(\d+)\s*)_" // Match map_fd fd +// fd must be a positive integer #define MAP_FD R"_(\s*map_fd\s+(\d+)\s*)_"Likely invalid or redundant comment.
src/asm_ostream.cpp (2)
362-363: LGTM! The LoadMapAddress output formatting looks correct.The implementation follows the established pattern for instruction formatting.
556-558: LGTM! The size calculation for LoadMapAddress looks correct.The implementation correctly returns size 2, consistent with similar instructions like LoadMapFd.
src/asm_unmarshal.cpp (2)
Line range hint
432-442: LGTM! The validation logic looks correct.The implementation properly validates the instruction source register against the allowed modes.
450-452: LGTM! The LoadMapAddress creation looks correct.The implementation properly creates the LoadMapAddress instruction with the correct fields.
src/test/test_verify.cpp (1)
528-528: LGTM! The test case addition looks correct.The test case is properly added using the TEST_SECTION macro.
src/test/test_marshal.cpp (2)
395-397: LGTM!Basic test coverage for LoadMapAddress marshal/unmarshal functionality.
44-45: LGTM!The instruction template follows the established pattern.
test-data/map.yaml (1)
1-26: LGTM!Test cases properly validate type assignments, offsets, and value propagation for both map_fd and map_val operations.
src/crab/ebpf_transformer.cpp (2)
1856-1856: LGTM!Clean and straightforward operator implementation.
1844-1854: 🧹 Nitpick (assertive)Add validation for mapfd parameter.
The method directly uses mapfd to retrieve the map descriptor without validation.
void ebpf_transformer::do_load_map_address(const Reg& dst_reg, const int mapfd, int32_t offset) { + if (mapfd < 0) { + m_inv.set_to_bottom(); + return; + } const EbpfMapDescriptor& desc = thread_local_program_info->platform->get_map_descriptor(mapfd); const EbpfMapType& type = thread_local_program_info->platform->get_map_type(desc.type);Likely invalid or redundant comment.
Signed-off-by: Alan Jowett <[email protected]>
From the ISA RFC:
5.4. 64-bit immediate instructions
Instructions with the IMM 'mode' modifier use the wide instruction
encoding defined in Instruction encoding (Section 3), and use the
'src_reg' field of the basic instruction to hold an opcode subtype.
The following table defines a set of {IMM, DW, LD} instructions with
opcode subtypes in the 'src_reg' field, using new terms such as "map"
defined further below:
+=========+================================+==========+==========+
| src_reg | pseudocode | imm type | dst type |
+=========+================================+==========+==========+
| 0x0 | dst = (next_imm << 32) | imm | integer | integer |
+---------+--------------------------------+----------+----------+
| 0x1 | dst = map_by_fd(imm) | map fd | map |
+---------+--------------------------------+----------+----------+
| 0x2 | dst = map_val(map_by_fd(imm)) | map fd | data |
| | + next_imm | | address |
+---------+--------------------------------+----------+----------+
| 0x3 | dst = var_addr(imm) | variable | data |
| | | id | address |
+---------+--------------------------------+----------+----------+
| 0x4 | dst = code_addr(imm) | integer | code |
| | | | address |
+---------+--------------------------------+----------+----------+
| 0x5 | dst = map_by_idx(imm) | map | map |
| | | index | |
+---------+--------------------------------+----------+----------+
| 0x6 | dst = map_val(map_by_idx(imm)) | map | data |
| | + next_imm | index | address |
+---------+--------------------------------+----------+----------+
Table 12: 64-bit immediate instructions
Signed-off-by: Alan Jowett <[email protected]>
Signed-off-by: Alan Jowett <[email protected]>
Signed-off-by: Alan Jowett <[email protected]>
Alan-Jowett
force-pushed
the
load_map_address
branch
from
8373d84 to
320dee7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/asm_files.cpp (2)
165-170: 🧹 Nitpick (assertive)Extend validation in
verify_load_instructionCurrently,
verify_load_instructiononly checks if the opcode class isINST_CLS_LD. Enhance the validation to check for specific load opcodes or include additional conditions to prevent invalid or unexpected instructions from passing through.
490-492: 🧹 Nitpick (assertive)Use
>=in boundary check for clarityIn the condition checking vector bounds, use
>=instead of<=for better readability and consistency.Apply this diff:
- if (prog.prog.size() <= offset / sizeof(ebpf_inst) + 1) { + if (offset / sizeof(ebpf_inst) + 1 >= prog.prog.size()) { throw UnmarshalError("Invalid relocation data"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(7 hunks)src/asm_marshal.cpp(4 hunks)src/asm_ostream.cpp(1 hunks)src/asm_parse.cpp(2 hunks)src/asm_syntax.hpp(3 hunks)src/asm_unmarshal.cpp(2 hunks)src/assertions.cpp(1 hunks)src/crab/ebpf_transformer.cpp(3 hunks)src/crab/split_dbm.hpp(1 hunks)src/ebpf_vm_isa.hpp(1 hunks)src/test/test_marshal.cpp(2 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_yaml.cpp(1 hunks)test-data/map.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (4)
test-data/map.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: test-data/map.yaml:20-20
Timestamp: 2025-01-08T16:32:48.309Z
Learning: The eBPF verifier test framework does not support testing invalid file descriptors, making it impossible to add test cases for invalid map_fd values or non-existent file descriptors.
src/asm_parse.cpp (3)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:177-179
Timestamp: 2025-01-08T16:39:55.117Z
Learning: The ASM parser in src/asm_parse.cpp is only used for parsing hand-written assembly snippets from YAML test files, not for parsing untrusted input, so input validation is unnecessary.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing (parse_instruction) is specifically used in src/test/ebpf_yaml.cpp for parsing controlled test inputs from YAML files in the test-data directory, and is not exposed to external inputs that would require validation.
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_parse.cpp:173-176
Timestamp: 2025-01-08T16:39:47.584Z
Learning: In ebpf-verifier, ASM parsing is only used for parsing hand-written test snippets in YAML files, not for parsing arbitrary input from external sources. Therefore, extensive input validation is not required.
src/test/test_marshal.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/test/test_marshal.cpp:395-397
Timestamp: 2025-01-08T20:19:15.070Z
Learning: Test coverage improvements should be handled in separate PRs to maintain focused changes, as demonstrated in PR #820 where additional test cases for LoadMapAddress were deferred.
src/asm_files.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#820
File: src/asm_files.cpp:205-229
Timestamp: 2025-01-08T17:37:07.223Z
Learning: Global variable relocations in eBPF are only supported with the new map types that use BTF data, not with the old-style map sections.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_ubuntu (Debug, library)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (26)
src/asm_marshal.cpp (5)
90-93: UpdatedmakeLddwfunction handlestypeparameter correctlyThe function signature now accepts a
uint8_t typeparameter and assigns it to.src. The implementation is correct.
108-109: Correct usage ofINST_LD_MODE_MAP_FDinLoadMapFdoperatorThe
operator()forLoadMapFdusesINST_LD_MODE_MAP_FDappropriately in the call tomakeLddw.
110-112: Proper implementation ofLoadMapAddressoperatorThe new
operator()forLoadMapAddresscorrectly constructs the instructions withINST_LD_MODE_MAP_VALUE.
119-119: Immediate load usesINST_LD_MODE_IMMaccuratelyThe immediate load operation now correctly utilizes
INST_LD_MODE_IMMin themakeLddwcall.
311-313:asm_syntax::sizefunction updated forLoadMapAddressThe function correctly returns
2forLoadMapAddressinstructions, ensuring proper instruction sizing.src/asm_files.cpp (3)
405-411: Global variable sections collected correctlyThe code accurately identifies and collects indices of global variable sections from
.rodata,.data, and.bss.
216-223: Proper handling of relocation values inrelocate_global_variableThe function correctly retrieves and validates the relocation value for global variables, throwing an exception if the map descriptor is not found.
225-229: Validation of relocation value boundsThe check ensures that
reloc_valueis within the bounds ofinfo.map_descriptors, preventing out-of-range access.src/test/test_yaml.cpp (1)
34-34: Added test case formap.yamlThe new test case
YAML_CASE("test-data/map.yaml")is appropriately added to the test suite.src/ebpf_vm_isa.hpp (1)
91-93: LGTM!The new instruction mode constants are correctly defined with sequential values and clear comments.
src/crab/split_dbm.hpp (1)
212-213: LGTM!The parameter name is correctly removed to prevent unused parameter warnings.
src/asm_syntax.hpp (2)
96-103: LGTM!The
LoadMapAddressstruct is well-documented with clear field descriptions.
386-386: Provide implementation details for thesizefunction.The function declaration lacks documentation about its purpose and return value semantics.
Run the following script to find the implementation:
✅ Verification successful
Add documentation for the
sizefunction/// Calculates the size of an instruction in instruction words. /// @param inst The instruction to measure /// @return 2 for double-word instructions (lddw Bin, LoadMapFd, LoadMapAddress), /// 1 for all other instructions int size(const Instruction& inst);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the implementation of the size function. rg -A 5 "int size\(const Instruction& inst\)"Length of output: 270
Script:
#!/bin/bash # Search for implementation files fd "asm_syntax\.(cpp|cc|cxx)" # Search for size function implementation with more context rg -g "!{build,test}/*" "size.*\(.*Instruction.*\)" -A 10 # Use ast-grep to find potential inline implementations ast-grep --pattern 'int size(const Instruction& inst) { $___ }'Length of output: 1096
Script:
#!/bin/bash # Get complete implementation with more context rg -A 15 "^int asm_syntax::size\(const Instruction& inst\)" src/asm_marshal.cppLength of output: 472
src/assertions.cpp (1)
48-48: LGTM!The operator correctly returns an empty vector, consistent with the
LoadMapFdoverload.src/asm_cfg.cpp (1)
402-403: LGTM!Correctly categorizes
LoadMapAddressas "assign" type, consistent withLoadMapFd.src/asm_parse.cpp (2)
57-62: LGTM!Regex patterns correctly match the expected formats:
- MAP_VAL:
map_val(fd) + offset- MAP_FD:
map_fd fd
173-179: LGTM!Correctly parses and constructs LoadMapAddress and LoadMapFd instructions from the matched patterns.
src/asm_unmarshal.cpp (2)
Line range hint
432-441: LGTM!Proper validation of instruction source and offset values.
442-461: LGTM!Clean switch statement implementation handling all load modes with proper error handling.
src/test/test_verify.cpp (1)
528-528: LGTM!Test case correctly updated to expect successful verification.
src/crab/ebpf_transformer.cpp (2)
47-47: LGTM!The operator declaration follows the established pattern and is correctly placed with other similar operators.
1860-1860: LGTM!The operator correctly delegates to do_load_map_address with proper parameter passing.
test-data/map.yaml (1)
16-26: LGTM!The test case correctly verifies map fd loading and register properties.
src/asm_ostream.cpp (1)
362-363: LGTM!The operator implementation follows the established pattern and produces consistent output format.
src/test/test_marshal.cpp (2)
44-45: LGTM! Addition of LDDW with src_reg=2 aligns with ISA RFC Section 5.4.The instruction template correctly implements support for 64-bit immediate instructions with type 2.
395-397: Basic test coverage for LoadMapAddress looks good.The test verifies the basic marshaling and unmarshaling functionality.
Run this script to verify the test coverage:
✅ Verification successful
Basic test coverage for LoadMapAddress is sufficient and follows established patterns
The test case properly verifies marshaling and unmarshaling using the same proven approach as other instructions in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for LoadMapAddress functionality # Test: Look for other test cases that might be testing LoadMapAddress rg -l "LoadMapAddress" | grep "_test"Length of output: 35
Script:
#!/bin/bash # Try case-insensitive search with broader test file patterns rg -i "loadmapaddress|load_map_address" -g "*test*" # Use ast-grep to find test cases ast-grep --pattern 'SECTION($$$) { $$$LoadMapAddress$$$}'Length of output: 294
Script:
#!/bin/bash # Check how compare_marshal_unmarshal is used in other tests rg "compare_marshal_unmarshal" src/test/test_marshal.cpp -B 1 -A 1Length of output: 4731
Signed-off-by: Alan Jowett <[email protected]>
I am not sure that really makes sense as the three load operations are quite different.
I think keeping them distinct makes more sense. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests