Remove JIT and interpreter code from NativeOnly builds #2475

dthaler · 2023-05-16T21:29:13Z

Description

Remove JIT and interpreter code from NativeOnly builds

Fixes #2030 (security cleanup)
Fixes #2488 (bug)

Also fixes version number typo in GettingStarted.md

Testing

Updated CI/CD

Note that issue #2281 only updated kernel tests to use sys files, it did not update the user-mode tests in tests/unit/libbpf_test.cpp and tests/end_to_end/end_to_end.cpp, so in this PR the relevant ones are commented out. Issue #1121 covers updating the relevant ones to use dlls, and also covers updating port_quota.exe.

Documentation

Updated docs

codecov · 2023-05-19T19:15:31Z

Codecov Report

Merging #2475 (f981580) into main (d6bcf19) will decrease coverage by 0.03%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main    #2475      +/-   ##
==========================================
- Coverage   83.99%   83.96%   -0.03%     
==========================================
  Files         157      157              
  Lines       29116    29119       +3     
==========================================
- Hits        24456    24450       -6     
- Misses       4660     4669       +9

Impacted Files	Coverage Δ
libs/execution_context/ebpf_native.c	`91.60% <ø> (ø)`
...ution_context/unit/execution_context_unit_test.cpp	`98.88% <ø> (ø)`
libs/thunk/mock/mock.cpp	`100.00% <ø> (ø)`
tests/bpf2c_tests/raw_bpf.cpp	`96.14% <ø> (ø)`
tests/end_to_end/end_to_end.cpp	`97.71% <ø> (ø)`
tests/sample/ext/app/sample_ext_app.cpp	`98.61% <ø> (ø)`
tests/unit/libbpf_test.cpp	`99.94% <ø> (ø)`
libs/api/libbpf_program.cpp	`83.24% <33.33%> (-0.47%)`	⬇️
libs/api/ebpf_api.cpp	`87.49% <100.00%> (-0.05%)`	⬇️
libs/execution_context/ebpf_core.c	`91.83% <100.00%> (ø)`
... and 1 more

... and 5 files with indirect coverage changes

saxena-anurag · 2023-05-31T18:07:19Z

libs/api/libbpf_program.cpp

@@ -25,6 +25,7 @@ bpf_load_program_xattr(const struct bpf_load_program_attr* load_attr, char* log_
        return libbpf_err(-EINVAL);
    }

+#if !defined(CONFIG_BPF_JIT_DISABLED) && !defined(CONFIG_BPF_INTERPRETER_DISABLED)


nit: why do we want to block this at API layer also if it is already blocked in ebpfcore? Anyone can modify and recompile this dll to override this.

Will having the checks in user mode block us to test the ebpfcore code where it fails the jit and interpret IOCTL calls for native_only case?

nit: why do we want to block this at API layer also if it is already blocked in ebpfcore? Anyone can modify and recompile this dll to override this.

footprint reduction, and also changes the error return from "invalid value" to the more specific "not supported" meaning.

Will having the checks in user mode block us to test the ebpfcore code where it fails the jit and interpret IOCTL calls for native_only case?

The UM unit tests test that code path.

saxena-anurag · 2023-05-31T20:10:49Z

tests/end_to_end/end_to_end.cpp

@@ -1139,6 +1158,7 @@ DECLARE_CGROUP_SOCK_ADDR_LOAD_TEST(
 DECLARE_CGROUP_SOCK_ADDR_LOAD_TEST(
    SAMPLE_PATH "cgroup_sock_addr", "authorize_recv_accept6", EBPF_ATTACH_TYPE_CGROUP_INET6_RECV_ACCEPT);

+#if !defined(CONFIG_BPF_JIT_DISABLED)


nit: instead of removing these tests from native build, we could possibly use native_module_helper_t helper to get an appropriate file name (.sys or .o) based on whether JIT is enabled or disabled.

Yes that's what #1121 already covers.

saxena-anurag · 2023-06-02T01:32:06Z

libs/api/ebpf_api.cpp

@@ -2528,6 +2538,7 @@ _Requires_lock_not_held_(_ebpf_state_mutex) static ebpf_result_t
    EBPF_RETURN_RESULT(result);
 }

+#if !defined(CONFIG_BPF_JIT_DISABLED) && !defined(CONFIG_BPF_INTERPRETER_DISABLED)


If JIT is enabled but INTERPRET is disabled (which is the case of Regular/Release build), do we still want to disable this API?

no. I think you're saying the && should be || and I think you're right

yes I meant to say that only -- that it should be || instead of &&

saxena-anurag · 2023-06-03T00:20:48Z

libs/execution_context/ebpf_protocol.h

@@ -9,11 +9,17 @@

 typedef enum _ebpf_operation_id
 {
+#if !defined(CONFIG_BPF_JIT_DISABLED)


nit: removing the operation ids from the middle will cause the value of the other ids to change. So the same operation ID will have different value in native-only vs. regular build. Not sure if this can cause any issues.

Since the IDs are not used in apps (they should be calling libbpf) it's ok to have breaking changes in my view

Fixes microsoft#2030 Fixes microsoft#2488 Signed-off-by: Dave Thaler <dthaler@microsoft.com>

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 May 16, 2023 21:29

dthaler marked this pull request as draft May 16, 2023 21:29

dthaler added cleanup Affects API usability or code maintainability but not correctness or applicability security Related to security hardening labels May 16, 2023

dthaler force-pushed the native-only branch 7 times, most recently from 88396ec to f7a53a1 Compare May 19, 2023 17:25

dthaler added bug Something isn't working and removed security Related to security hardening labels May 19, 2023

dthaler force-pushed the native-only branch 2 times, most recently from 5fc1ceb to c5d50c5 Compare May 19, 2023 18:16

dthaler force-pushed the native-only branch 8 times, most recently from 23ed3a1 to 2ae2e07 Compare May 21, 2023 04:41

dthaler force-pushed the native-only branch 2 times, most recently from b9a693c to 57aa29d Compare May 21, 2023 18:03

dthaler force-pushed the native-only branch from 57aa29d to 3ea9828 Compare May 30, 2023 14:59

dthaler marked this pull request as ready for review May 31, 2023 02:46

dthaler changed the title ~~WIP: Remove JIT and interpreter code from NativeOnly builds~~ Remove JIT and interpreter code from NativeOnly builds May 31, 2023

dthaler added the security Related to security hardening label May 31, 2023

Alan-Jowett approved these changes May 31, 2023

View reviewed changes

Alan-Jowett previously approved these changes May 31, 2023

View reviewed changes

saxena-anurag reviewed May 31, 2023

View reviewed changes

saxena-anurag reviewed Jun 2, 2023

View reviewed changes

dthaler dismissed Alan-Jowett’s stale review via 05489c2 June 2, 2023 17:46

Alan-Jowett approved these changes Jun 2, 2023

View reviewed changes

saxena-anurag approved these changes Jun 3, 2023

View reviewed changes

dthaler added 8 commits June 6, 2023 10:27

Remove JIT and interpreter code from NativeOnly builds

5ff97d1

Fixes microsoft#2030 Fixes microsoft#2488 Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Try conditional project references

c602a92

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

Various test fixes

7158f7e

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

Native-only fixes to deploy-ebpf.ps1

7b61b1a

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

Native-only support for setup-ebpf.ps1

1b8cff1

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

Native-only support for api_test.exe

34ab8c7

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

Add connect_redirect_tests to deploy-ebpf.ps1

16270ff

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

PR feedback from Anurag

f981580

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

dthaler force-pushed the native-only branch from 05489c2 to f981580 Compare June 6, 2023 17:27

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

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 7, 2023

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

Merged via the queue into microsoft:main with commit 87b6520 Jun 7, 2023

dthaler deleted the native-only branch June 7, 2023 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove JIT and interpreter code from NativeOnly builds #2475

Remove JIT and interpreter code from NativeOnly builds #2475

dthaler commented May 16, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

saxena-anurag May 31, 2023

saxena-anurag May 31, 2023

dthaler May 31, 2023

dthaler May 31, 2023

saxena-anurag May 31, 2023

dthaler Jun 1, 2023

saxena-anurag Jun 2, 2023

dthaler Jun 2, 2023

saxena-anurag Jun 2, 2023

dthaler Jun 2, 2023

saxena-anurag Jun 3, 2023

dthaler Jun 3, 2023

Remove JIT and interpreter code from NativeOnly builds #2475

Remove JIT and interpreter code from NativeOnly builds #2475

Conversation

dthaler commented May 16, 2023 • edited Loading

Description

Testing

Documentation

codecov bot commented May 19, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dthaler commented May 16, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading