Support bpf2bpf calls from ELF files #693
Conversation
Signed-off-by: Dave Thaler <[email protected]>
WalkthroughThe changes involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant MainProgram
participant ELFProcessor
participant FunctionRelocationQueue
MainProgram->>ELFProcessor: Read ELF file
ELFProcessor->>FunctionRelocationQueue: Queue relocation requests
FunctionRelocationQueue->>ELFProcessor: Process queued relocations
ELFProcessor->>MainProgram: Return processed ELF
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)
🔇 Additional comments (8)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/asm_files.cpp (7 hunks)
- src/test/test_verify.cpp (2 hunks)
🔇 Additional comments (7)
src/test/test_verify.cpp (4)
79-82: LGTM: New macro for testing programs expected to fail verification.The
TEST_PROGRAM_REJECTmacro is well-defined and consistent with the existing testing framework. It correctly sets up a test case for programs that should fail verification, which enhances the test coverage.
505-505: LGTM: New test case for subprogram expected to fail verification.This test case correctly uses the new
TEST_PROGRAM_REJECTmacro to test the "plus1" subprogram, which is expected to fail verification. This adds valuable coverage to the test suite.
506-506: LGTM: New test case for subprogram expected to pass verification.This test case appropriately uses the
TEST_PROGRAMmacro to test the "func" program, which is expected to pass verification. The comment clarifies that this subprogram can be called from the main program, explaining its expected behavior.
79-82: Summary: Enhancements to eBPF program verification testing.The changes in this file improve the test coverage for eBPF program verification:
- A new macro
TEST_PROGRAM_REJECThas been added to facilitate testing of programs expected to fail verification.- Two new test cases have been added:
- One using
TEST_PROGRAM_REJECTto test a subprogram expected to fail verification.- Another using
TEST_PROGRAMto test a subprogram expected to pass verification.These additions enhance the robustness of the test suite by explicitly covering both positive and negative test scenarios for bpf2bpf calls across separate program objects.
Also applies to: 505-506
src/asm_files.cpp (3)
Line range hint
63-76: Consistent Type Usage for Index ParameterThe function
get_symbol_name_and_section_indexnow usesELFIO::Elf_Xwordfor theindexparameter instead ofELFIO::Elf_Word. This change allows the function to handle larger symbol tables, which is consistent with the ELF64 specification and ensures correctness when processing files with a large number of symbols.
Line range hint
77-90: Update Index Parameter Type to ELFIO::Elf_XwordThe
get_valuefunction'sindexparameter has been updated fromELFIO::Elf_WordtoELFIO::Elf_Xword. This adjustment aligns with the changes made inget_symbol_name_and_section_indexand ensures that the function can handle symbol tables with a larger number of entries, enhancing compatibility with ELF64 files.
411-421: Verify Correctness ofprog_indexAssignmentIn the creation of
function_relocation,prog_indexis set usingres.size()beforeprogis pushed intores:function_relocation fr{.prog_index = res.size(), /* ... */ };Since
progis added toresafter this point, there is a possibility thatprog_indexmight not correctly reference the current program.Please confirm that
res.size()accurately represents the index of the currentprog. If not, consider adjusting the code to addprogtoresbefore this assignment or modifyprog_indexaccordingly.
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/asm_files.cpp (7 hunks)
🔇 Additional comments (3)
src/asm_files.cpp (3)
202-207: Declaration offunction_relocationstruct is appropriateThe new
struct function_relocationneatly encapsulates the data required for queuing function relocations, enhancing code clarity and maintainability.
443-450: Logic for appending subprograms is correctly implementedThe
append_subprogramsfunction appropriately processes function relocations and appends subprograms to ensure that all calls are resolved within the same program for verification.
Line range hint
63-77: Verify usage of updated function parameter typesThe functions
get_symbol_name_and_section_indexandget_valuenow have theindexparameter type changed fromELFIO::Elf_WordtoELFIO::Elf_Xword. Ensure that all calls to these functions throughout the codebase are updated to useELFIO::Elf_Xwordto prevent potential type mismatches or truncation.Run the following script to find and review all usages:
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/asm_files.cpp (7 hunks)
🔇 Additional comments (5)
src/asm_files.cpp (5)
209-229:append_subprogramFunction ImplementationThe
append_subprogramfunction correctly locates and appends the subprogram instructions to the main program. The checks for zero-size subprograms and proper error handling are appropriate.
231-259: Efficient Management of Function Relocations inappend_subprogramsThe
append_subprogramsfunction efficiently handles function relocations and ensures that each subprogram is appended only once. The use ofsubprogram_offsetsto track appended subprograms improves performance by avoiding duplicate processing.
421-428: Confirm Correctness of Function Relocation HandlingThe code queues up function relocations for local call instructions. Ensure that the
relocation_entry_indexcorresponds correctly to the target function, and that the function name resolution is accurate.
445-445: Avoid Storing References in ContainersThe
segment_to_programmap stores references toraw_programobjects:std::map<std::string, raw_program&> segment_to_program;Storing references in standard containers can lead to dangling references if the referenced objects are moved or if the container is copied. Consider storing pointers or indices instead to ensure the references remain valid.
379-384:⚠️ Potential issueRemove Unused Variable
subprogramsThe variable
subprogramsis declared andprogram_nameis inserted into it, but it is not used elsewhere in the code. This unused variable can be safely removed to clean up the code.Apply this diff to remove the unused variable:
- std::set<string> subprograms{}; - subprograms.insert(program_name);Likely invalid or redundant comment.
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/asm_files.cpp (7 hunks)
🧰 Additional context used
📓 Learnings (1)
src/asm_files.cpp (1)
Learnt from: dthaler PR: vbpf/ebpf-verifier#693 File: src/asm_files.cpp:63-63 Timestamp: 2024-09-27T03:48:52.280Z Learning: When working with the ELFIO library, `ELFIO::Elf_Half` is the correct type for return values of 16-bit fields provided by the library and should not be changed to `ELFIO::Elf_Xword`.
🔇 Additional comments (3)
src/asm_files.cpp (3)
202-209: Well-documentedfunction_relocationstructureThe added documentation above the
function_relocationstruct definition enhances code readability and maintainability.
63-63: Consistency in ELFIO Types for Symbol IndicesThe functions
get_symbol_name_and_section_indexandget_valuenow useELFIO::Elf_Xwordfor theindexparameter, supporting larger symbol tables. This change improves compatibility with ELF files containing more symbols.Also applies to: 77-77
253-258: Check for Potential Integer Overflow in Offset CalculationThe code correctly checks for overflow when calculating
offset_diff:int64_t offset_diff = (int64_t)(target_offset - reloc.source_offset - 1); if (offset_diff < INT32_MIN || offset_diff > INT32_MAX) { throw std::runtime_error("Offset difference out of int32_t range for instruction at source offset " + std::to_string(reloc.source_offset)); } prog.prog[reloc.source_offset].imm = (int32_t)offset_diff;This ensures that the cast to
int32_tis safe and prevents potential errors.
Signed-off-by: Dave Thaler <[email protected]>
PR #608 added support for bpf2bpf calls when instructions were contiguous, including YAML tests for it.
However PR #638 added support for multiple programs per section, and in that support, instructions for different programs (or subprograms) are not contiguous but are in separate program objects.
This PR adds support for bpf2bpf calls across program objects, including ELF tests for it (using an exising ebpf-sample sample). In the existing bpf2bpf.o sample, "func" calls "prog1" in a valid way. "prog1" is not verifiable on its own since its prototype doesn't match the program type, and a negative test is added for this case.
(The type change to the get_value() argument is to resolve a compiler warning.)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation