Add support for 64bit immediate with type 2 (load map value address). #119
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new instruction type Changes
Sequence DiagramsequenceDiagram
participant Parser as Instruction Parser
participant Unmarshaller as Unmarshaller
participant Transformer as eBPF Transformer
participant Marshaller as Marshaller
Parser->>Unmarshaller: Parse instruction
Unmarshaller-->>Parser: Create LoadMapAddress instruction
Parser->>Transformer: Transform instruction
Transformer-->>Parser: Process map address loading
Parser->>Marshaller: Marshal instruction
Marshaller-->>Parser: Prepare instruction for execution
Poem
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/asm_files.cpp (1)
401-402: Remove unused variable 'map_descriptors_offsets'The variable
map_descriptors_offsetsdeclared at line 401 is unused, leading to a compiler warning and potential confusion. Consider removing it to clean up the code.Apply this diff to remove the unused variable:
- auto& map_descriptors_offsets = std::get<1>(map_record_size_or_map_offsets);🧰 Tools
🪛 GitHub Actions: CPP Code Coverage
[warning] 401-401: Unused variable 'map_descriptors_offsets'
src/asm_marshal.cpp (1)
311-313: Synchronizesizefunction with marshaling logicSince the marshaling logic for
LoadMapAddressshould produce two instructions, thesizefunction correctly returns 2. Ensure that this size corresponds to the implemented marshaling logic.After implementing the marshaling logic, confirm that the
sizefunction accurately reflects the number of instructions generated forLoadMapAddress.src/asm_unmarshal.cpp (1)
Line range hint
432-452: LGTM! Consider adding a comment for the magic number.The implementation correctly handles the
LoadMapAddressinstruction type, maintaining consistency with existing error handling and validation.Consider adding a comment explaining the magic number 2, similar to the comment for
src == 1:if (inst.src == 2) { + // magic number, meaning we're loading a map value address + // (for details, look for BPF_PSEUDO_MAP_VALUE in the kernel) return LoadMapAddress{.dst = Reg{inst.dst}, .mapfd = inst.imm, .offset = next_imm}; }src/crab/ebpf_transformer.cpp (1)
1844-1854: Add input validation and documentation.While the implementation is functionally correct, consider these improvements:
- Add input validation for the
offsetparameter to ensure it's within bounds of the map's value size.- Add documentation explaining the method's purpose and behavior.
- Add error handling for map descriptor lookup failures.
Here's a suggested implementation with the improvements:
+/** + * Loads a map's address with the specified offset into a register. + * + * @param dst_reg The destination register + * @param mapfd The map file descriptor + * @param offset The offset within the map's value region + * @throws std::out_of_range if the map descriptor lookup fails + */ void ebpf_transformer::do_load_map_address(const Reg& dst_reg, const int mapfd, int32_t offset) { 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); + // Validate offset is within bounds + if (offset < 0 || offset >= desc.value_size) { + throw std::out_of_range("Map address offset out of bounds"); + } + // 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); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
external/libbtf(1 hunks)src/asm_cfg.cpp(1 hunks)src/asm_files.cpp(7 hunks)src/asm_marshal.cpp(2 hunks)src/asm_ostream.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)
✅ Files skipped from review due to trivial changes (1)
- external/libbtf
🧰 Additional context used
🪛 GitHub Actions: CPP Code Coverage
src/asm_files.cpp
[warning] 287-287: Comparison of integer expressions of different signedness: 'int' and 'std::map<int, btf_line_info_t>::size_type'
[warning] 401-401: Unused variable 'map_descriptors_offsets'
🔇 Additional comments (9)
src/asm_files.cpp (2)
Line range hint
287-287: Address signedness comparison warningA compiler warning indicates a comparison between an
intand asize_ttype at line 287. This can lead to unexpected behavior due to signedness mismatch. Ensure that both operands are of the same type by casting or changing variable types as necessary.Since the specific code is not shown, please review line 287 to correct the signed/unsigned comparison. If
some_int_variableis of typeint, consider casting it tosize_tor changing its type.
499-504:⚠️ Potential issuePotential out-of-bounds access in
inst_nextIn the
relocate_global_variablecall,inst_nextmay reference an instruction beyond the end of the program if not properly checked. Ensure thatinst_nextis within the bounds ofprog.progto prevent undefined behavior.Please verify that
inst_nextis valid before accessing it. You can modify the code to include a boundary check:ebpf_inst& inst = prog.prog[offset / sizeof(ebpf_inst)]; -ebpf_inst& inst_next = offset / sizeof(ebpf_inst) + 1 < prog.prog.size() - ? prog.prog[offset / sizeof(ebpf_inst) + 1] - : inst; +size_t inst_index = offset / sizeof(ebpf_inst); +ebpf_inst& inst_next = inst_index + 1 < prog.prog.size() + ? prog.prog[inst_index + 1] + : inst;src/asm_syntax.hpp (1)
96-104: 🛠️ Refactor suggestionEnsure consistent handling of
LoadMapAddressin visitorsThe new
LoadMapAddressinstruction has been added to theInstructionvariant. Please verify that all visitor patterns and handlers throughout the codebase are updated to accommodate this new instruction to prevent unhandled cases.Ensure that any code relying on pattern matching over
InstructionhandlesLoadMapAddressappropriately. This includes updating visitors, switch statements, and other pattern matches.src/assertions.cpp (1)
48-48: LGTM!The implementation is consistent with other map-related operations in the class, and correctly returns an empty vector of assertions for the
LoadMapAddressinstruction.src/asm_cfg.cpp (1)
402-403: LGTM!The classification of
LoadMapAddressas an "assign" instruction type is consistent with similar map-related operations likeLoadMapFd.src/asm_ostream.cpp (2)
362-363: LGTM!The formatting of
LoadMapAddressinstructions is clear and consistent with the codebase's style.
556-558: LGTM!The size of 2 for
LoadMapAddressinstructions is consistent with similar instructions likeLoadMapFd.src/crab/ebpf_transformer.cpp (2)
47-47: LGTM! Method declaration follows class conventions.The declaration of
operator()(const LoadMapAddress&)is well-placed among other instruction handlers and follows the established pattern of the class.
1856-1857: LGTM! Simple delegation to helper method.The operator implementation correctly delegates to the helper method, maintaining a clean separation of concerns.
Alan-Jowett
changed the title
Alan-Jowett
force-pushed
the
load_map_address
branch
4 times, most recently
from
f863c07 to
831cc0a
Compare
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]>
Alan-Jowett
force-pushed
the
load_map_address
branch
from
831cc0a to
23edeee
Compare
Add support to the verifier to handle 64bit immediate loads with source == 2, which is defined as:
Generated PR description
This pull request introduces several significant changes to the codebase, focusing on the addition of support for a new
LoadMapAddressinstruction, enhancements to ELF file handling, and various updates to the instruction set and related components. Below is a summary of the most important changes:Support for
LoadMapAddressInstruction:LoadMapAddressstruct to the instruction set (src/asm_syntax.hpp). [1] [2]instypefunction to handle theLoadMapAddressinstruction (src/asm_cfg.cpp).LoadMapAddress(src/asm_marshal.cpp,src/asm_unmarshal.cpp). [1] [2] [3]CommandPrinterVisitorto supportLoadMapAddress(src/asm_ostream.cpp). [1] [2]ELF File Handling Enhancements:
.data,.bss, and.rodatasections (src/asm_files.cpp). [1] [2] [3] [4] [5] [6]Transformer and Assertion Updates:
ebpf_transformerclass to handleLoadMapAddressinstructions (src/crab/ebpf_transformer.cpp). [1] [2] [3]LoadMapAddressin theAssertExtractorclass (src/assertions.cpp).Miscellaneous Changes:
external/libbtfto a new commit (external/libbtf).<assert.h>header insrc/asm_files.cppfor assertions (src/asm_files.cpp).Summary by CodeRabbit
New Features
Bug Fixes
Refactor