Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add versioning to native module NMR interface #4256

Conversation

saxena-anurag
Copy link
Contributor

@saxena-anurag saxena-anurag commented Mar 1, 2025

Fixes #4266

This pull request includes significant changes to the include/bpf2c.h and libs/shared/shared_common.c files to add version headers to various eBPF native module data structures. The changes also introduce new validation functions and update existing structures to include these headers.

Introduction of Version Headers:

  • Added ebpf_native_module_header_t to multiple data structures in include/bpf2c.h to ensure backward compatibility and facilitate future extensions. [1] [2] [3] [4]
  • Defined version headers for various eBPF native module data structures in include/bpf2c.h.

Validation Functions:

  • Introduced validation functions for eBPF native module data structures in libs/shared/ebpf_shared_framework.h.
  • Implemented these validation functions in libs/shared/shared_common.c.

Updates to Existing Structures:

  • Updated _ebpf_pe_get_map_definitions in libs/api/ebpf_api.cpp to handle new alignment requirements for map entries.
  • Added version headers to struct bpf_prog_info in include/ebpf_structs.h.

Additional Changes:

  • Moved ARRAY_ELEM_INDEX macro from libs/shared/shared_common.c to libs/shared/ebpf_shared_framework.h. [1] [2]
  • Updated test files to include version headers in data structures. [1] [2] [3]

These changes ensure that the eBPF native module data structures are backward compatible and can be extended in the future without breaking existing functionality.

saxena-anurag and others added 30 commits February 3, 2025 08:51
dthaler

This comment was marked as outdated.

@saxena-anurag saxena-anurag requested a review from dthaler March 21, 2025 00:58
@saxena-anurag saxena-anurag enabled auto-merge March 21, 2025 00:58
dthaler
dthaler previously approved these changes Mar 21, 2025
Copy link
Collaborator

@dthaler dthaler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but there's some more places that could benefit from "const" if you have to make other changes for any reason.

Comment on lines 1526 to 1527
helper_function_entry_t* entry =
(helper_function_entry_t*)ARRAY_ELEMENT_INDEX(helper_info, i, helper_entry_size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
helper_function_entry_t* entry =
(helper_function_entry_t*)ARRAY_ELEMENT_INDEX(helper_info, i, helper_entry_size);
const helper_function_entry_t* entry =
(const helper_function_entry_t*)ARRAY_ELEMENT_INDEX(helper_info, i, helper_entry_size);

Comment on lines +1299 to +1300
global_variable_section_info_t* global_variable_section_info =
(global_variable_section_info_t*)ARRAY_ELEMENT_INDEX(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
global_variable_section_info_t* global_variable_section_info =
(global_variable_section_info_t*)ARRAY_ELEMENT_INDEX(
const global_variable_section_info_t* global_variable_section_info =
(const global_variable_section_info_t*)ARRAY_ELEMENT_INDEX(

@@ -718,22 +1002,26 @@ _ebpf_native_initialize_maps(
}

for (uint32_t i = 0; i < map_count; i++) {
if (maps[i].definition.pinning != LIBBPF_PIN_NONE && maps[i].definition.pinning != LIBBPF_PIN_BY_NAME) {
// Copy the map_entry_t from native module to ebpf_native_map_t.
map_entry_t* map_entry = (map_entry_t*)ARRAY_ELEMENT_INDEX(maps, i, map_entry_size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
map_entry_t* map_entry = (map_entry_t*)ARRAY_ELEMENT_INDEX(maps, i, map_entry_size);
const map_entry_t* map_entry = (const map_entry_t*)ARRAY_ELEMENT_INDEX(maps, i, map_entry_size);

@@ -496,43 +792,30 @@ _ebpf_native_provider_attach_client_callback(
ebpf_native_module_t** module = NULL;
bool lock_acquired = false;
metadata_table_t* table = NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
metadata_table_t* table = NULL;


table = (metadata_table_t*)client_dispatch;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
table = (metadata_table_t*)client_dispatch;
const metadata_table_t* table = (const metadata_table_t*)client_dispatch;

memcpy(&local_global_section_info, global_variable_section_info, global_variable_section_info_size);
global_variable_section_info = NULL;

ebpf_native_map_t* native_map = _ebpf_native_find_map_by_name(instance, local_global_section_info.name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ebpf_native_map_t* native_map = _ebpf_native_find_map_by_name(instance, local_global_section_info.name);
const ebpf_native_map_t* native_map = _ebpf_native_find_map_by_name(instance, local_global_section_info.name);

@saxena-anurag saxena-anurag added this pull request to the merge queue Mar 21, 2025
Merged via the queue into microsoft:main with commit a6c82d6 Mar 21, 2025
95 of 97 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add versioning to Native module NMR interface structs.
3 participants