Fixes for multiple bpf2bpf stack levels #801
Conversation
WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)src/asm_files.cpp (3)🔇 Additional comments (6)src/asm_files.cpp (6)
The function appropriately searches for a subprogram by name and section, returning a pointer to the subprogram if found. Line range hint The logic for appending subprograms is correctly implemented, ensuring that subprograms are resolved only once and handling recursion properly.
Assigning
The logic to propagate line information when only the first instruction per source line has BTF data is valid. Copying the previous line info when the current instruction lacks it ensures that all instructions have associated line information.
The code correctly appends subprograms to the calling programs, ensuring the programs are fully verifiable and instruction offsets are properly adjusted.
The reverse iteration safely filters programs based on the desired section. The check 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
📒 Files selected for processing (4)
ebpf-samples(1 hunks)src/asm_files.cpp(4 hunks)src/spec_type_descriptors.hpp(1 hunks)src/test/test_verify.cpp(1 hunks)
🔇 Additional comments (4)
ebpf-samples (1)
1-1: Verify the submodule commit and its contents
✅ Verification successful
Submodule update contains required test changes for BTF loader
The commit d8ae598 updates bpf2bpf.c to test multiple stack levels by:
- Adding a new
add2function that creates additional stack frames - Modifying the test to call through multiple stack levels
- Changing the expected result to account for multiple function calls
This change aligns with the PR objective of fixing BTF loader code issues with multiple programs per section, as it provides a test case with deeper stack manipulation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the submodule commit exists and contains required test changes
# Expected results:
# 1. Commit should exist and be accessible
# 2. Changes should include test updates for BTF loader fixes
# Check if commit exists
git ls-remote https://github.com/vbpf/ebpf-samples.git d8ae598573421b336260502459b6e9e164e75eb3
# Look for BTF-related test changes in commit
gh api repos/vbpf/ebpf-samples/commits/d8ae598573421b336260502459b6e9e164e75eb3 | jq -r '.commit.message'
# Search for BTF-related files in the commit
gh api repos/vbpf/ebpf-samples/commits/d8ae598573421b336260502459b6e9e164e75eb3 | jq -r '.files[].filename' | grep -i "btf"
Length of output: 433
Script:
#!/bin/bash
# Let's check the actual files changed in the commit
gh api repos/vbpf/ebpf-samples/commits/d8ae598573421b336260502459b6e9e164e75eb3 | jq -r '.files[].filename'
# Get the patch to see the actual changes
gh api repos/vbpf/ebpf-samples/commits/d8ae598573421b336260502459b6e9e164e75eb3 | jq -r '.files[].patch'
Length of output: 1155
src/spec_type_descriptors.hpp (1)
73-73: Verify thread-safety with resolved_subprograms flag.
The presence of thread_local_program_info suggests concurrent usage. Need to verify thread-safety of subprogram resolution.
src/test/test_verify.cpp (2)
513-513: LGTM!
The test case for "func" remains unchanged as expected.
511-512: Test cases updated to handle multiple programs per section.
The changes reflect the fix for issue #792, where the test now correctly expects two programs in the .text section instead of one.
Run the following script to verify the test changes align with the actual program changes:
Make read_elf() with a desired_section work correctly Fixes vbpf#792 Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
dthaler
force-pushed
the
bpf2bpf-multistack
branch
from
4ddc74a to
903421e
Compare
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 (2)
src/asm_files.cpp(4 hunks)src/spec_type_descriptors.hpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/asm_files.cpp (4)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#801
File: src/asm_files.cpp:483-483
Timestamp: 2024-11-23T17:05:19.380Z
Learning: In `src/asm_files.cpp`, when `program.info.line_info` is an `std::map`, the existing logic in the loop copying line info over `program.prog.size()` is correct and does not require additional resizing or checks.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#801
File: src/asm_files.cpp:483-483
Timestamp: 2024-11-23T17:02:37.673Z
Learning: In `src/asm_files.cpp`, the `program.info.line_info` is an `std::map`, which does not have a `resize()` method.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#801
File: src/asm_files.cpp:258-260
Timestamp: 2024-11-23T17:03:10.573Z
Learning: `prog.info.line_info` is an `std::map`, so when merging line information entries, use the `insert` method to add entries from `subprogram.info.line_info`.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#801
File: src/asm_files.cpp:208-213
Timestamp: 2024-11-23T16:59:47.517Z
Learning: In the `ebpf-verifier` codebase, the number of programs is generally very small, so optimizing the `find_subprogram` function with a map is unnecessary.
🔇 Additional comments (1)
src/spec_type_descriptors.hpp (1)
73-76: Verify the usage of resolved_subprograms flag.
The flag addition is correct and properly initialized. Let's verify its usage across the codebase.
✅ Verification successful
The resolved_subprograms flag is correctly used to prevent duplicate subprogram resolution
The flag is checked and set in src/asm_files.cpp before appending subprograms:
- Guards against duplicate resolution with an early return if already resolved
- Sets the flag to true before proceeding with subprogram resolution
- Used in conjunction with
append_subprogramsfunction calls for proper control flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of resolved_subprograms flag to ensure it's properly checked and set.
# Find all references to resolved_subprograms
rg -A 5 'resolved_subprograms'
# Find potential subprogram resolution logic
ast-grep --pattern 'append_subprograms($$$)'
Length of output: 1283
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (2)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#801
File: src/asm_files.cpp:483-483
Timestamp: 2024-11-23T17:05:19.380Z
Learning: In `src/asm_files.cpp`, when `program.info.line_info` is an `std::map`, the existing logic in the loop copying line info over `program.prog.size()` is correct and does not require additional resizing or checks.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#801
File: src/asm_files.cpp:483-483
Timestamp: 2024-11-23T17:02:37.673Z
Learning: In `src/asm_files.cpp`, the `program.info.line_info` is an `std::map`, which does not have a `resize()` method.
🔇 Additional comments (3)
src/asm_files.cpp (3)
208-217: LGTM: Simple and effective subprogram lookup
Linear search is appropriate given the small number of programs typically present.
Line range hint 493-498: LGTM: Proper line info propagation
Correctly propagates line info to instructions that lack it.
502-523: LGTM: Clean program filtering implementation
Good use of reverse iteration for vector erasure and proper error handling for missing subprograms.
Signed-off-by: Dave Thaler <[email protected]>
Fixes #792
Summary by CodeRabbit
New Features
Bug Fixes
Tests