aya-obj: expand bpf_map_def to 36 bytes for tc maps#1497
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Expands the `bpf_map_def` struct from 28 to 36 bytes by adding the `inner_id` and `inner_idx` fields. This aligns the struct with the `bpf_elf_map` ABI used by the tc/iproute2 toolchain for inner-map resolution. Because the struct size dynamically determines the parsing stride via `size_of::<bpf_map_def>()`, this ABI correction automatically enables the parser to safely zero-pad older 28-byte legacy maps, ensuring strict backward compatibility. Includes V1 (28-byte) and V2 (36-byte) legacy map tests to validate the new memory stride and prevent future regressions. Fixes: aya-rs#156
ramos-99
force-pushed
the
fix-legacy-maps-tc
branch
from
c778ad9 to
a714f49
Compare
|
Ping @tamird, can you check this when you have time? |
|
Looks to me like this needs an integration test. @codex review |
There was a problem hiding this comment.
Pull request overview
This PR updates aya-obj’s legacy map definition parsing to support TC/iproute2-style ELF map symbols by expanding bpf_map_def to include the inner_id and inner_idx fields (36 bytes total), preventing silent truncation when loading such objects.
Changes:
- Expanded
aya_obj::maps::bpf_map_defwithinner_idandinner_idxto match the 36-bytebpf_elf_maplayout used by tc/iproute2. - Updated object parsing/tests to initialize the new fields and validate parsing of both legacy 28-byte and TC-style 36-byte map definitions.
- Updated the
aya-objpublic API snapshot to reflect the new struct fields.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
xtask/public-api/aya-obj.txt |
Updates the public API dump to include inner_id/inner_idx in bpf_map_def. |
aya-obj/src/obj.rs |
Initializes new fields where bpf_map_def is constructed and adds/adjusts tests for 28-byte legacy vs 36-byte TC-style map defs. |
aya-obj/src/maps.rs |
Expands the bpf_map_def struct with inner_id and inner_idx (C layout) and documents the fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fake_sym(&mut obj, 0, 0, "foo", size_of::<bpf_map_def>() as u64); | ||
| fake_sym(&mut obj, 0, 28, "bar", size_of::<bpf_map_def>() as u64); | ||
| fake_sym(&mut obj, 0, 60, "baz", size_of::<bpf_map_def>() as u64); | ||
| fake_sym(&mut obj, 0, 36, "bar", size_of::<bpf_map_def>() as u64); | ||
| fake_sym(&mut obj, 0, 76, "baz", size_of::<bpf_map_def>() as u64); |
There was a problem hiding this comment.
In test_parse_section_multiple_maps_v2_legacy_tc, the symbol addresses are hard-coded to 36 and 76 while symbol sizes use size_of::<bpf_map_def>(). If bpf_map_def ever changes again, these offsets can become inconsistent with the struct size and make the test fail for the wrong reason. Consider deriving the offsets from size_of::<bpf_map_def>() (and the padding length) to keep the test self-consistent.
| fake_sym(&mut obj, 0, 0, "foo", 28); | ||
| fake_sym(&mut obj, 0, 28, "bar", 28); | ||
| fake_sym(&mut obj, 0, 60, "baz", 28); |
There was a problem hiding this comment.
test_parse_section_multiple_maps_v1_legacy uses several magic numbers (28, 60) for legacy map-def sizes/offsets. Since the intent is to validate the legacy 28-byte format, consider introducing a named constant (e.g., legacy map-def size and padding) and computing the offsets from it to make the test intent clearer and reduce the chance of accidental inconsistency.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| #[test_log::test] | ||
| fn legacy_36_byte_maps() { | ||
| let mut bpf = Ebpf::load(crate::TC_LEGACY_MAP).unwrap(); | ||
|
|
||
| let map_1: Array<_, u32> = bpf.take_map("tc_map_1").unwrap().try_into().unwrap(); | ||
| let map_2: Array<_, u32> = bpf.take_map("tc_map_2").unwrap().try_into().unwrap(); | ||
|
|
||
| assert_eq!(map_1.len(), 1); | ||
| assert_eq!(map_2.len(), 2); | ||
| } |
There was a problem hiding this comment.
what is being tested here? reading the change description and this test name i am not understanding what i should be checking here.
There was a problem hiding this comment.
each map has a distinct max_elem (1 and 2) to verify both are parsed correctly from the 36-byte struct. the parser is backwards compatible, so it also handles the 28-byte variant without needing separate test cases
Adds a dedicated C program and Rust integration test to prevent
regressions in the ELF parser regarding legacy SEC("maps") strides.
The test includes two 36-byte map definitions with initialized
`inner_id` and `inner_idx` fields to explicitly enforce the full
struct size in the compiled object. It verifies both maps are
correctly parsed and distinct by asserting their `max_entries`.
ramos-99
force-pushed
the
fix-legacy-maps-tc
branch
from
2010967 to
909a02a
Compare
|
Thanks for the patch! We should probably document that we don't support iproute2's style of map-of-maps, or maybe make it work after #1446 lands. With the PR as is I think we have UB because of this https://kernel.googlesource.com/pub/scm/network/iproute2/iproute2-next/%2B/refs/tags/v6.10.0/include/bpf_elf.h#29 In general iproute2 pinning seems to work differently from aya/libbpf, the paths don't match. Maybe we should reject loading if pinning != 0? |
|
Thanks for the review and the context! Totally agree that silently ignoring pinning is a problem. Since iproute2's pinning paths differ from aya's, returning an error if pinning != 0 makes a lot of sense to fail fast. On the map-of-maps side: since iproute2's style of map-of-maps isn't supported yet, would it also make sense to reject inner_id != 0 and inner_idx != 0 for now? That way users get a clear error instead of silent failure, and the restriction can be dropped once support is added. Happy to implement both checks and add a documentation note if that sounds good. |
Reject legacy maps with pinning != 0, since iproute2 pinning paths differ from aya's and silently ignoring them leads to undefined behavior. Add documentation noting that pinning and map-of-maps are not supported for legacy maps. Refactor parse_map_def to use a single binding for the map_def value, then add the pinning check before returning. Update two existing parse_map_def tests to use PinningType::None and add a new test for the rejection path.
|
I've gone ahead and implemented the pinning fail-fast and added documentation notes for pinning and map-of-maps. Let me know if it aligns with what you were thinking. |
|
Friendly ping @alessandrod, could you take a look at this when you have the time? Thanks! |
The
bpf_map_defstruct in aya-obj was defined with 28 bytes (7 fields),but the iproute2/tc toolchain produces ELF objects using
bpf_elf_mapwhich has 36 bytes (9 fields). The two missing fields,
inner_idandinner_idx, caused silent truncation when loading TC-style BPF objects.parse_map_defalready usessize_of::<bpf_map_def>()dynamically todetermine the parsing stride, so expanding the struct to 36 bytes
automatically zero-pads legacy 28-byte maps without any changes to the
parsing logic, maintaining full backward compatibility.
Updated existing tests to initialize the new fields and added
test_parse_section_multiple_maps_v2_legacy_tcto validate nativeparsing of 36-byte ELF symbols with non-zero
inner_idandinner_idx.Fixes: #156
This change is