Make bpf2c maps parser match verifier's maps parser #2543

dthaler · 2023-06-03T20:09:51Z

Description

The verifier will parse "maps/*" sections, and is able to deal with maps sections with various record sizes. This PR updates bpf2c to use the same algorithm. In the future it would be good to refactor the verifier so the same code can be used.

Before this change, such a program would pass the verifier but bpf2c couldn't convert it.
Also upgrading to the latest verifier will fail without this PR.

Also the PR to add BTF support regressed the map_in_map_v2.c sample which stopped testing the legacy reference-by-id paths. This PR changes map_in_map_v2.c back and renames it to map_in_map_legacy_id.c for clarity. Similarly, it restores map_in_map_legacy_idx.c with the original map_in_map.c and renames the BTF map_in_map.c to map_in_map_btf.c. This ensures all three code paths for finding the inner map are tested.

Fixes #900
Fixes #2580

Note to code reviewers: this change is easiest to review by ignoring whitespace changes (since it removes an indentation level for a bunch of code). To do so in github, click "Files changed", then the gear button dropdown button just under the "Checks" and "Files changed" labels, click "Hide whitespace" then "Apply and reload".

Testing

The update to map_in_map.c results in testing this functionality.

Documentation

No impact.

saxena-anurag

I think there are some more places in ebpf-for-windows code, where it either assumes the maps section will always be "maps" or hard codes the maps section size. Should we fix those also in this PR?

_get_program_and_map_names() (verifier.cpp) only looks for "maps" section to find the map names, and fails if any map name is not found in "maps" section.
parse_maps_section_windows() assumes the map size to be sizeof(ebpf_map_definition_in_file_t)
read_elf() (elfhelper.cpp) only ignores "maps" section, it should also ignore any "maps/*" sections.

saxena-anurag · 2023-06-05T19:06:14Z

tools/bpf2c/bpf_code_generator.cpp

+    size_t data_size = map_section->get_size();
+    size_t map_record_size = data_size / map_count;
+
+    if (data_size % map_record_size != 0) {


Should we have a check before this line that map_record_size != 0 ?

dthaler · 2023-06-09T21:22:51Z

I think there are some more places in ebpf-for-windows code, where it either assumes the maps section will always be "maps" or hard codes the maps section size. Should we fix those also in this PR?

_get_program_and_map_names() (verifier.cpp) only looks for "maps" section to find the map names, and fails if any map name is not found in "maps" section.

parse_maps_section_windows() assumes the map size to be sizeof(ebpf_map_definition_in_file_t)

Thanks. Both of those are now updated.

read_elf() (elfhelper.cpp) only ignores "maps" section, it should also ignore any "maps/*" sections.

ElfHelper appears to be dead code. Filed issue #2572 for removal.

dthaler · 2023-06-11T22:33:08Z

Test requires vbpf/ebpf-verifier#510

codecov · 2023-06-12T18:33:27Z

Codecov Report

Merging #2543 (2213908) into main (ec27659) will increase coverage by 0.02%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #2543      +/-   ##
==========================================
+ Coverage   81.18%   81.21%   +0.02%     
==========================================
  Files         146      147       +1     
  Lines       27655    27738      +83     
==========================================
+ Hits        22452    22527      +75     
- Misses       5203     5211       +8

Impacted Files	Coverage Δ
libs/service/api_service.cpp	`80.00% <ø> (ø)`
tests/bpftool_tests/bpftool_tests.cpp	`97.95% <ø> (ø)`
tests/end_to_end/test_helper.cpp	`84.63% <ø> (ø)`
tests/sample/map_in_map_btf.c	`100.00% <ø> (ø)`
tools/bpf2c/bpf_code_generator.h	`93.54% <ø> (ø)`
libs/api_common/api_common.hpp	`81.25% <61.53%> (-2.54%)`	⬇️
tools/bpf2c/bpf_code_generator.cpp	`96.06% <90.42%> (-0.31%)`	⬇️
libs/api/Verifier.cpp	`77.82% <95.65%> (+0.90%)`	⬆️
libs/api/windows_platform.cpp	`97.05% <96.66%> (+1.22%)`	⬆️
libs/api/ebpf_api.cpp	`84.28% <100.00%> (+0.13%)`	⬆️
... and 7 more

... and 2 files with indirect coverage changes

dthaler · 2023-06-12T20:53:21Z

blocked on vbpf/ebpf-verifier#506 which is fixed in vbpf/ebpf-verifier#511

shpalani

The curious question behind the changes:
Is the goal to support more than one verifier (from different vendors)?
Or,
Is the goal to support different versions of the same vendor's verifier?

Reason: I see the map query definition by id. So, how is the id determined?

dthaler · 2023-06-27T18:17:27Z

The curious question behind the changes: Is the goal to support more than one verifier (from different vendors)? Or, Is the goal to support different versions of the same vendor's verifier?

No. The goal is just to support PREVAIL and have ebpf-for-windows and prevail match in terms of what map capabilities they support.

Reason: I see the map query definition by id. So, how is the id determined?

In a file, it's specified in the BPF program code. See https://github.com/microsoft/ebpf-for-windows/pull/2543/files#diff-c0a7a085a58201871e764452242c7f7fbc723b2dbd8762f3ae879d8c9faddf8cR16 (which is not new code, this restores code that used to be there that was incorrectly deleted in a previous PR).

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

The verifier will parse "maps/*" sections, and is able to deal with maps sections with various record sizes. This PR updates bpf2c to use the same algorithm. In the future it would be good to refactor the verifier so the same code can be used. Fixes microsoft#900 Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Temporarily point to fork until vbpf/ebpf-verifier#515 is merged, so we can verify the fix in CI/CD. Signed-off-by: Dave Thaler <dthaler@microsoft.com>

saxena-anurag · 2023-06-26T20:03:08Z

tests/unit/libbpf_test.cpp

@@ -1631,13 +1631,13 @@ TEST_CASE("simple hash of maps", "[libbpf]") { _ebpf_test_map_in_map(BPF_MAP_TYP

 // Verify an app can communicate with an eBPF program via an array of maps.
 static void
-_array_of_maps_test(ebpf_execution_type_t execution_type)
+_array_of_btf_maps_test(ebpf_execution_type_t execution_type)


_array_of_btf_maps_test() and _array_of_id_maps_test() look identical except for the file name used. Maybe we can refactor them into a single function and pass file name as input?

Good idea, done

saxena-anurag · 2023-06-26T20:07:49Z

tests/unit/libbpf_test.cpp

@@ -1745,15 +1746,15 @@ _array_of_maps2_test(ebpf_execution_type_t execution_type)
    bpf_object__close(xdp_object);
 }

-DECLARE_JIT_TEST_CASES("array of maps2", "[libbpf]", _array_of_maps2_test);
+DECLARE_JIT_TEST_CASES("array of id maps", "[libbpf]", _array_of_id_maps_test);


Should we add an idx version of the test also?

Good idea, done

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

dthaler requested review from poornagmsft, Alan-Jowett, saxena-anurag, shankarseal, dv-msft, delaramamiri and gtrevi as code owners June 3, 2023 20:09

dthaler added the bug Something isn't working label Jun 3, 2023

dthaler added this to the 2306 milestone Jun 3, 2023

saxena-anurag reviewed Jun 5, 2023

View reviewed changes

dthaler force-pushed the maps branch from 4fda276 to 763a34c Compare June 6, 2023 17:26

dthaler marked this pull request as draft June 9, 2023 18:22

dthaler force-pushed the maps branch from a12af73 to 28d1051 Compare June 9, 2023 21:01

dthaler force-pushed the maps branch 2 times, most recently from a52dca8 to 71919bd Compare June 10, 2023 21:04

dthaler mentioned this pull request Jun 10, 2023

Bump external/ebpf-verifier from 7a052d4 to af9045e #2577

Closed

dthaler force-pushed the maps branch 2 times, most recently from 571a115 to 2ae7d4e Compare June 11, 2023 19:23

dthaler force-pushed the maps branch 2 times, most recently from 5373aba to c28a0b7 Compare June 12, 2023 17:41

dthaler added the blocked Blocked on another issue that must be done first label Jun 12, 2023

dthaler mentioned this pull request Jun 13, 2023

Bump external/ebpf-verifier from 7a052d4 to 551026c #2588

Closed

dthaler force-pushed the maps branch from c28a0b7 to ab95496 Compare June 15, 2023 20:05

dthaler removed the blocked Blocked on another issue that must be done first label Jun 15, 2023

dthaler mentioned this pull request Jun 19, 2023

Bump external/ebpf-verifier from 7a052d4 to 243a1ad #2602

Closed

dthaler force-pushed the maps branch from f7fdcdc to 91869f8 Compare June 19, 2023 16:44

dthaler enabled auto-merge June 26, 2023 19:45

shpalani reviewed Jun 27, 2023

View reviewed changes

dthaler mentioned this pull request Jun 27, 2023

Bump external/ebpf-verifier from 7a052d4 to 5f0e73f #2639

Closed

dthaler added 14 commits June 28, 2023 09:04

Add map_in_map_legacy.c

fc532d9

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Make tests pass

cdc2e3a

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

PR feedback

61b4755

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Rename map_in_map_* samples

fd46117

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Fix BTF ID resolution

5daaeed

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

PR feedback

cdf49c6

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Cleanup

36976ba

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Bug fixes

72ace56

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Add doc fix

41cec1e

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Update verifier

9d8d5a0

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Updates for libbtf

e73616f

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Use libbtf

e0400e5

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Fix sanitize build by using updated libbtf

db74121

Temporarily point to fork until vbpf/ebpf-verifier#515 is merged, so we can verify the fix in CI/CD. Signed-off-by: Dave Thaler <dthaler@microsoft.com>

dthaler dismissed Alan-Jowett’s stale review via 68e7a80 June 28, 2023 16:09

dthaler force-pushed the maps branch from 6703038 to 68e7a80 Compare June 28, 2023 16:09

saxena-anurag reviewed Jun 28, 2023

View reviewed changes

Update ebpf-verifier

543bf6e

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

dthaler force-pushed the maps branch from 68e7a80 to 543bf6e Compare June 28, 2023 16:34

PR feedback

2213908

Signed-off-by: Dave Thaler <dthaler@microsoft.com>

saxena-anurag approved these changes Jun 28, 2023

View reviewed changes

Alan-Jowett approved these changes Jun 28, 2023

View reviewed changes

dthaler added this pull request to the merge queue Jun 28, 2023

Merged via the queue into microsoft:main with commit 8fecc2e Jun 28, 2023

dthaler deleted the maps branch June 28, 2023 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make bpf2c maps parser match verifier's maps parser #2543

Make bpf2c maps parser match verifier's maps parser #2543

dthaler commented Jun 3, 2023 •

edited

Loading

saxena-anurag left a comment

saxena-anurag Jun 5, 2023

dthaler Jun 9, 2023

dthaler commented Jun 9, 2023 •

edited

Loading

dthaler commented Jun 11, 2023

codecov bot commented Jun 12, 2023 •

edited

Loading

dthaler commented Jun 12, 2023 •

edited

Loading

shpalani left a comment

dthaler commented Jun 27, 2023 •

edited

Loading

saxena-anurag Jun 26, 2023

dthaler Jun 28, 2023

saxena-anurag Jun 26, 2023

dthaler Jun 28, 2023

Make bpf2c maps parser match verifier's maps parser #2543

Make bpf2c maps parser match verifier's maps parser #2543

Conversation

dthaler commented Jun 3, 2023 • edited Loading

Description

Testing

Documentation

saxena-anurag left a comment

Choose a reason for hiding this comment

saxena-anurag Jun 5, 2023

Choose a reason for hiding this comment

dthaler Jun 9, 2023

Choose a reason for hiding this comment

dthaler commented Jun 9, 2023 • edited Loading

dthaler commented Jun 11, 2023

codecov bot commented Jun 12, 2023 • edited Loading

Codecov Report

dthaler commented Jun 12, 2023 • edited Loading

shpalani left a comment

Choose a reason for hiding this comment

dthaler commented Jun 27, 2023 • edited Loading

saxena-anurag Jun 26, 2023

Choose a reason for hiding this comment

dthaler Jun 28, 2023

Choose a reason for hiding this comment

saxena-anurag Jun 26, 2023

Choose a reason for hiding this comment

dthaler Jun 28, 2023

Choose a reason for hiding this comment

dthaler commented Jun 3, 2023 •

edited

Loading

dthaler commented Jun 9, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

dthaler commented Jun 12, 2023 •

edited

Loading

dthaler commented Jun 27, 2023 •

edited

Loading