add bpf2c support for non-inlined local function calls #3506

dthaler · 2024-04-28T22:30:05Z

Description

Previous to this PR, bpf2c only supported inlined function calls, not bpf2bpf function calls.
PREVAIL support for non-inlined function calls is in vbpf/ebpf-verifier#608.
This PR adds support in bpf2c, as well as a bindmonitor_bpf2bpf.c test sample.

The "gsl" changes in include paths are due to vbpf/ebpf-verifier#707 now requiring GSL for the verifier.

Fixes #3388
Fixes #3885

Note: stack variables in the callee are not yet supported. Those will come in a future PR once vbpf/ebpf-verifier#720 is fixed. For now, this works when the callee only needs registers.

Testing

This PR adds a bindmonitor_bpf2bpf.c test sample
It also adds tests for issue #3885

Documentation

This PR includes a doc update to isa-support.rst

Installation

No impact

hawkinsw

One minor comment about ubpf support for calling local functions.

docs/isa-support.rst

Alan-Jowett · 2024-05-10T20:54:32Z

I strongly suspect this change won't pass conformance tests once Alan-Jowett/bpf_conformance#245 merges.

The BPF ISA appears to assume that the JIT compiler adjusts R10 when calling the child function. The code needs to be modified to do one of two things:

Each frame gets its own 512 of stack space.
Caller and callee share stack space, but caller adjusts R10 to highest used address before calling child.

dthaler · 2024-05-11T15:02:45Z

I strongly suspect this change won't pass conformance tests once Alan-Jowett/bpf_conformance#245 merges.

If so, I think that PR should be fixed.

The BPF ISA appears to assume that the JIT compiler adjusts R10 when calling the child function.

The BPF ISA has no such assumption. See https://datatracker.ietf.org/doc/draft-ietf-bpf-isa/
The ISA itself does not define any specific meaning for R10 or any specific stack size.
That is the psABI not the ISA and in my opinion any conformance tests for ISA vs psABI should be separate.
That's because per https://datatracker.ietf.org/wg/bpf/about/ the ABI is intended to be a standard but the psABI is just informational so may vary by runtime.

The code needs to be modified to do one of two things:

Each frame gets its own 512 of stack space.

Caller and callee share stack space, but caller adjusts R10 to highest used address before calling child.

This might be needed to match the Linux psABI but not for conformance to the ABI. As such, I'd treat them as part of a separate pull request that may need additional verifier support.

tests/sample/bindmonitor_bpf2bpf.c

Alan-Jowett · 2024-06-20T15:13:18Z

@dthaler can you resolve the conflicts on this? We should be able to merge it after that.

shankarseal · 2024-07-29T16:19:38Z

@dthaler can you resolve the conflicts on this? We should be able to merge it after that.

@dthaler - can you please take a look?

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

tools/bpf2c/bpf_code_generator.cpp

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

saxena-anurag · 2024-10-15T00:02:33Z

libs/api/ebpf_api.cpp

-ebpf_program_next(_In_opt_ const struct bpf_program* previous, _In_ const struct bpf_object* object) NO_EXCEPT_TRY
+// This function is derived from libbpf's function of the same name.
+static _Ret_maybenull_ struct bpf_program*
+__bpf_program__iter(_In_opt_ const struct bpf_program* program, _In_ const struct bpf_object* object, bool forward)


nit: should this function be in one of the libbpf_* files?

It's static, I didn't see any value in exporting it just for use in 1 other file since it's also internal in libbpf.

saxena-anurag · 2024-10-15T00:14:47Z

tools/bpf2c/bpf2c.cpp

@@ -291,9 +291,14 @@ main(int argc, char** argv)

        // Parse per-program data.
        for (const ebpf_api_program_info_t* program = infos; program; program = program->next) {
+            // Skip if a subprogram.  A subprogram is defined by libbpf as any
+            // program in the .text section when multiple programs exist.
+            if (!strcmp(program->section_name, ".text") && infos->next != nullptr) {


question: does subprogram here mean the bpf-to-bpf functions? (which are not the entry BPF programs)? are they always guaranteed to be in ".text" section?

Libbpf only treats them as such if they are in the ".text" section. So apparently.

saxena-anurag · 2024-10-15T01:25:28Z

tools/bpf2c/bpf_code_generator.cpp

+    "r3",
+    "r4",
+    "r5",
+    "r10",


nit: why do we need to pass stack pointer to the sub-program? The current sample program does not use this? Should we enhance the sample programs to test cases where the sub-program uses r10 also?

Yes it's part of the future PR I referred to in the PR description, for which vbpf/ebpf-verifier#721 is the prerequisite.

saxena-anurag · 2024-10-31T06:17:52Z

tests/unit/libbpf_test.cpp

+    REQUIRE(bpf_object__prev_program(object, nullptr) == program);
+
+    // Load the program.
+    REQUIRE(bpf_object__load(object) == 0);


nit: should we validate that only one BPF program is loaded in kernel?

dthaler requested review from poornagmsft, Alan-Jowett, saxena-anurag, shankarseal, dv-msft, gtrevi, shpalani, matthewige, mtfriesen and rectified95 as code owners April 28, 2024 22:30

dthaler marked this pull request as draft April 28, 2024 22:30

hawkinsw reviewed Apr 29, 2024

View reviewed changes

docs/isa-support.rst Outdated Show resolved Hide resolved

dthaler force-pushed the calllocal branch 4 times, most recently from 56ad36d to e72b0a8 Compare April 30, 2024 22:14

dthaler force-pushed the calllocal branch 3 times, most recently from 514bc26 to c3486b3 Compare May 7, 2024 14:08

dthaler marked this pull request as ready for review May 7, 2024 15:03

dthaler mentioned this pull request May 11, 2024

Verify that bpf2bpf updates r10 Alan-Jowett/bpf_conformance#245

Closed

Alan-Jowett reviewed May 24, 2024

View reviewed changes

tests/sample/bindmonitor_bpf2bpf.c Show resolved Hide resolved

Alan-Jowett approved these changes May 29, 2024

View reviewed changes

Alan-Jowett previously approved these changes May 29, 2024

View reviewed changes

mikeagun previously approved these changes Jun 20, 2024

View reviewed changes

dthaler dismissed mikeagun’s stale review via c9abd5f September 3, 2024 21:21

dthaler added 11 commits October 10, 2024 09:14

Fixes for multiple programs per section

a6a8c8a

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Temporarily use dthaler fork of ebpf-verifier

a6ece56

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Fix bpf2c_fuzzer compilation

92ebf1e

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Update expected files for bpf2c_tests

fe5643f

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Fix bug hit by test

6b87c23

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Fix test

2bd83c8

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Fix test

e5672ce

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Fix test

d6f50dc

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Fix fuzzer

d6450a0

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Add support for calling helper functions from subprograms

fb1677a

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

Update comment per PR feedback

4cae6bc

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

dthaler force-pushed the calllocal branch from d224b81 to 4cae6bc Compare October 10, 2024 16:14

Alan-Jowett reviewed Oct 10, 2024

View reviewed changes

tools/bpf2c/bpf_code_generator.cpp Show resolved Hide resolved

Alan-Jowett reviewed Oct 10, 2024

View reviewed changes

tools/bpf2c/bpf_code_generator.cpp Outdated Show resolved Hide resolved

Alan-Jowett previously approved these changes Oct 10, 2024

View reviewed changes

Add more comments

7ea03ea

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>

dthaler dismissed Alan-Jowett’s stale review via 7ea03ea October 10, 2024 16:38

saxena-anurag reviewed Oct 15, 2024

View reviewed changes

Merge branch 'main' into calllocal

ea9ffa3

dthaler enabled auto-merge October 28, 2024 21:52

Alan-Jowett approved these changes Oct 28, 2024

View reviewed changes

dthaler added 2 commits October 29, 2024 11:30

Merge branch 'main' into calllocal

d35c619

Merge branch 'main' into calllocal

1481cf2

saxena-anurag approved these changes Oct 31, 2024

View reviewed changes

dthaler added this pull request to the merge queue Oct 31, 2024

Merged via the queue into microsoft:main with commit e71e7f7 Oct 31, 2024
90 checks passed

dthaler deleted the calllocal branch October 31, 2024 07:53

dthaler mentioned this pull request Oct 31, 2024

bpf2c should support bpf2bpf function calls #3388

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add bpf2c support for non-inlined local function calls #3506

add bpf2c support for non-inlined local function calls #3506

dthaler commented Apr 28, 2024 •

edited

Loading

hawkinsw left a comment

Alan-Jowett commented May 10, 2024

dthaler commented May 11, 2024

Alan-Jowett commented Jun 20, 2024

shankarseal commented Jul 29, 2024

saxena-anurag Oct 15, 2024

dthaler Oct 15, 2024

saxena-anurag Oct 15, 2024

dthaler Oct 15, 2024

saxena-anurag Oct 15, 2024

dthaler Oct 15, 2024

saxena-anurag Oct 31, 2024

add bpf2c support for non-inlined local function calls #3506

add bpf2c support for non-inlined local function calls #3506

Conversation

dthaler commented Apr 28, 2024 • edited Loading

Description

Testing

Documentation

Installation

hawkinsw left a comment

Choose a reason for hiding this comment

Alan-Jowett commented May 10, 2024

dthaler commented May 11, 2024

Alan-Jowett commented Jun 20, 2024

shankarseal commented Jul 29, 2024

saxena-anurag Oct 15, 2024

Choose a reason for hiding this comment

dthaler Oct 15, 2024

Choose a reason for hiding this comment

saxena-anurag Oct 15, 2024

Choose a reason for hiding this comment

dthaler Oct 15, 2024

Choose a reason for hiding this comment

saxena-anurag Oct 15, 2024

Choose a reason for hiding this comment

dthaler Oct 15, 2024

Choose a reason for hiding this comment

saxena-anurag Oct 31, 2024

Choose a reason for hiding this comment

dthaler commented Apr 28, 2024 •

edited

Loading