From 6bbf2b96837d43c89646ac04bb47e50774f6f7a2 Mon Sep 17 00:00:00 2001 From: Alan Jowett Date: Thu, 13 Apr 2023 14:54:14 -0700 Subject: [PATCH 1/3] Deregister for helper function change when deleting native module Signed-off-by: Alan Jowett --- libs/execution_context/ebpf_native.c | 25 +++---- libs/execution_context/ebpf_program.c | 103 +++++++++++++++++--------- libs/execution_context/ebpf_program.h | 4 +- 3 files changed, 82 insertions(+), 50 deletions(-) diff --git a/libs/execution_context/ebpf_native.c b/libs/execution_context/ebpf_native.c index 23f88911ee..8f7e43a321 100644 --- a/libs/execution_context/ebpf_native.c +++ b/libs/execution_context/ebpf_native.c @@ -126,7 +126,8 @@ typedef struct _ebpf_native_helper_address_changed_context } ebpf_native_helper_address_changed_context_t; static ebpf_result_t -_ebpf_native_helper_address_changed(_Inout_ ebpf_program_t* program, _Inout_opt_ void* context); +_ebpf_native_helper_address_changed( + size_t address_count, _In_reads_opt_(address_count) uintptr_t* addresses, _In_opt_ void* context); static void _ebpf_native_unload_work_item(_In_opt_ const void* service) @@ -180,6 +181,11 @@ _ebpf_native_clean_up_programs(_In_reads_(count_of_programs) ebpf_native_program { for (uint32_t i = 0; i < count_of_programs; i++) { if (programs[i].handle != ebpf_handle_invalid) { + ebpf_program_t* program_object = NULL; + ebpf_assert_success(ebpf_object_reference_by_handle( + programs[i].handle, EBPF_OBJECT_PROGRAM, (ebpf_core_object_t**)&program_object)); + ebpf_assert_success(ebpf_program_register_for_helper_changes(program_object, NULL, NULL)); + ebpf_object_release_reference((ebpf_core_object_t*)program_object); ebpf_assert_success(ebpf_handle_close(programs[i].handle)); } ebpf_free(programs[i].addresses_changed_callback_context); @@ -1462,7 +1468,8 @@ ebpf_native_get_count_of_maps(_In_ const GUID* module_id, _Out_ size_t* count_of } static ebpf_result_t -_ebpf_native_helper_address_changed(_Inout_ ebpf_program_t* program, _Inout_opt_ void* context) +_ebpf_native_helper_address_changed( + size_t address_count, _In_reads_opt_(address_count) uintptr_t* addresses, _In_opt_ void* context) { ebpf_result_t return_value; ebpf_native_helper_address_changed_context_t* helper_address_changed_context = @@ -1477,21 +1484,13 @@ _ebpf_native_helper_address_changed(_Inout_ ebpf_program_t* program, _Inout_opt_ goto Done; } - helper_function_addresses = ebpf_allocate(helper_count * sizeof(uint64_t)); - if (helper_function_addresses == NULL) { - return_value = EBPF_NO_MEMORY; - goto Done; - } - - return_value = ebpf_program_get_helper_function_addresses(program, helper_count, helper_function_addresses); - - if (return_value != EBPF_SUCCESS) { + if (helper_count != address_count || addresses == NULL) { + return_value = EBPF_INVALID_ARGUMENT; goto Done; } for (size_t i = 0; i < helper_count; i++) { - *(uint64_t*)&(helper_address_changed_context->native_program->entry->helpers[i].address) = - helper_function_addresses[i]; + *(uint64_t*)&(helper_address_changed_context->native_program->entry->helpers[i].address) = addresses[i]; } return_value = EBPF_SUCCESS; diff --git a/libs/execution_context/ebpf_program.c b/libs/execution_context/ebpf_program.c index 591f1f0e1d..316a40af00 100644 --- a/libs/execution_context/ebpf_program.c +++ b/libs/execution_context/ebpf_program.c @@ -82,17 +82,17 @@ typedef struct _ebpf_program _Guarded_by_(lock) void* helper_function_addresses_changed_context; } ebpf_program_t; -static ebpf_result_t -_ebpf_program_update_helpers(_Inout_ ebpf_program_t* program); +_Requires_lock_held_(program->lock) static ebpf_result_t _ebpf_program_update_helpers(_Inout_ ebpf_program_t* program); static ebpf_result_t -_ebpf_program_update_interpret_helpers(_Inout_ ebpf_program_t* program, _Inout_ void* context); +_ebpf_program_update_interpret_helpers( + size_t address_count, _In_reads_(address_count) uintptr_t* addresses, _Inout_ void* context); static ebpf_result_t -_ebpf_program_update_jit_helpers(_Inout_ ebpf_program_t* program, _Inout_ void* context); +_ebpf_program_update_jit_helpers( + size_t address_count, _In_reads_(address_count) uintptr_t* addresses, _Inout_ void* context); -static ebpf_result_t -_ebpf_program_get_helper_function_address( +_Requires_lock_held_(program->lock) static ebpf_result_t _ebpf_program_get_helper_function_address( _In_ const ebpf_program_t* program, const uint32_t helper_function_id, uint64_t* address); _Must_inspect_result_ ebpf_result_t @@ -105,8 +105,7 @@ void ebpf_program_terminate() {} -static void -_ebpf_program_detach_links(_Inout_ ebpf_program_t* program) +_Requires_lock_not_held_(program->lock) static void _ebpf_program_detach_links(_Inout_ ebpf_program_t* program) { EBPF_LOG_ENTRY(); while (!ebpf_list_is_empty(&program->links)) { @@ -117,8 +116,8 @@ _ebpf_program_detach_links(_Inout_ ebpf_program_t* program) EBPF_RETURN_VOID(); } -static ebpf_result_t -_ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program); +_Requires_lock_held_(program->lock) static ebpf_result_t + _ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program); static ebpf_result_t _ebpf_program_program_info_provider_changed( @@ -204,6 +203,8 @@ _ebpf_program_program_info_provider_changed( goto Exit; } + ebpf_lock_state_t state = ebpf_lock_lock(&program->lock); + return_value = _ebpf_program_update_helpers(program); if (return_value != EBPF_SUCCESS) { EBPF_LOG_MESSAGE( @@ -216,6 +217,8 @@ _ebpf_program_program_info_provider_changed( program->global_helper_function_count = global_helper_function_addresses ? global_helper_function_addresses->helper_function_count : 0; program->bpf_prog_type = program_data->program_info->program_type_descriptor.bpf_prog_type; + + ebpf_lock_unlock(&program->lock, state); return_value = EBPF_SUCCESS; } @@ -256,6 +259,7 @@ _ebpf_program_free(_In_opt_ _Post_invalid_ ebpf_core_object_t* object) program->cleanup_work_item = NULL; ebpf_epoch_schedule_work_item(cleanup_work_item); + EBPF_RETURN_VOID(); } @@ -323,8 +327,8 @@ _ebpf_program_epoch_free(_In_ _Post_invalid_ void* context) EBPF_RETURN_VOID(); } -static ebpf_result_t -_ebpf_program_load_providers(_Inout_ ebpf_program_t* program) +_Requires_lock_not_held_(program->lock) static ebpf_result_t + _ebpf_program_load_providers(_Inout_ ebpf_program_t* program) { EBPF_LOG_ENTRY(); ebpf_result_t return_value; @@ -647,8 +651,7 @@ ebpf_program_associate_maps(ebpf_program_t* program, ebpf_map_t** maps, uint32_t EBPF_RETURN_RESULT(result); } -static ebpf_result_t -_ebpf_program_load_machine_code( +_Requires_lock_held_(program->lock) static ebpf_result_t _ebpf_program_load_machine_code( _Inout_ ebpf_program_t* program, _In_opt_ const void* code_context, _In_reads_(machine_code_size) const uint8_t* machine_code, @@ -710,22 +713,48 @@ _ebpf_program_load_machine_code( EBPF_RETURN_RESULT(return_value); } -static ebpf_result_t -_ebpf_program_update_helpers(_Inout_ ebpf_program_t* program) +_Requires_lock_held_(program->lock) static ebpf_result_t _ebpf_program_update_helpers(_Inout_ ebpf_program_t* program) { + ebpf_result_t result = EBPF_SUCCESS; + uintptr_t* helper_function_addresses = NULL; + if (program->parameters.code_type == EBPF_CODE_NATIVE) { + helper_function_addresses = + ebpf_allocate_with_tag(program->helper_function_count * sizeof(uintptr_t), EBPF_POOL_TAG_PROGRAM); + if (helper_function_addresses == NULL) { + result = EBPF_NO_MEMORY; + goto Done; + } + + for (uint32_t index = 0; index < program->helper_function_count; index++) { + result = _ebpf_program_get_helper_function_address( + program, program->helper_function_ids[index], &helper_function_addresses[index]); + if (result != EBPF_SUCCESS) { + break; + } + } + } + if (program->helper_function_addresses_changed_callback != NULL) { - return program->helper_function_addresses_changed_callback( - program, program->helper_function_addresses_changed_context); - } else { - return EBPF_SUCCESS; + result = program->helper_function_addresses_changed_callback( + program->helper_function_count, + helper_function_addresses, + program->helper_function_addresses_changed_context); } +Done: + ebpf_free(helper_function_addresses); + return result; } static ebpf_result_t -_ebpf_program_update_interpret_helpers(_Inout_ ebpf_program_t* program, _Inout_ void* context) +_ebpf_program_update_interpret_helpers( + size_t address_count, _In_reads_(address_count) uintptr_t* addresses, _Inout_ void* context) { EBPF_LOG_ENTRY(); - UNREFERENCED_PARAMETER(context); + UNREFERENCED_PARAMETER(address_count); + UNREFERENCED_PARAMETER(addresses); + + ebpf_program_t* program = (ebpf_program_t*)context; + _Analysis_assume_lock_held_(program->lock); ebpf_result_t result = EBPF_SUCCESS; size_t index = 0; @@ -758,10 +787,13 @@ _ebpf_program_update_interpret_helpers(_Inout_ ebpf_program_t* program, _Inout_ } static ebpf_result_t -_ebpf_program_update_jit_helpers(_Inout_ ebpf_program_t* program, _Inout_ void* context) +_ebpf_program_update_jit_helpers( + size_t address_count, _In_reads_(address_count) uintptr_t* addresses, _Inout_ void* context) { ebpf_result_t return_value; - UNREFERENCED_PARAMETER(context); + UNREFERENCED_PARAMETER(address_count); + UNREFERENCED_PARAMETER(addresses); + ebpf_program_t* program = (ebpf_program_t*)context; ebpf_program_data_t* program_data = NULL; const ebpf_helper_function_addresses_t* helper_function_addresses = NULL; const ebpf_helper_function_addresses_t* global_helper_function_addresses = NULL; @@ -915,8 +947,7 @@ _ebpf_program_update_jit_helpers(_Inout_ ebpf_program_t* program, _Inout_ void* } #if !defined(CONFIG_BPF_INTERPRETER_DISABLED) -static ebpf_result_t -_ebpf_program_load_byte_code( +_Requires_lock_held_(program->lock) static ebpf_result_t _ebpf_program_load_byte_code( _Inout_ ebpf_program_t* program, _In_ const ebpf_instruction_t* instructions, size_t instruction_count) { EBPF_LOG_ENTRY(); @@ -954,7 +985,7 @@ _ebpf_program_load_byte_code( ubpf_set_error_print(program->code_or_vm.vm, ebpf_log_function); program->helper_function_addresses_changed_callback = _ebpf_program_update_interpret_helpers; - program->helper_function_addresses_changed_context = NULL; + program->helper_function_addresses_changed_context = program; return_value = _ebpf_program_update_helpers(program); if (return_value != EBPF_SUCCESS) { @@ -997,6 +1028,7 @@ ebpf_program_load_code( { EBPF_LOG_ENTRY(); ebpf_result_t result = EBPF_SUCCESS; + ebpf_lock_state_t state = ebpf_lock_lock(&program->lock); program->parameters.code_type = code_type; ebpf_assert( (code_type == EBPF_CODE_NATIVE && code_context != NULL) || @@ -1029,6 +1061,7 @@ ebpf_program_load_code( } } + ebpf_lock_unlock(&program->lock, state); EBPF_RETURN_RESULT(result); } @@ -1137,8 +1170,7 @@ ebpf_program_invoke( } } -static ebpf_result_t -_ebpf_program_get_helper_function_address( +_Requires_lock_held_(program->lock) static ebpf_result_t _ebpf_program_get_helper_function_address( _In_ const ebpf_program_t* program, const uint32_t helper_function_id, uint64_t* address) { ebpf_result_t return_value; @@ -1248,6 +1280,7 @@ ebpf_program_get_helper_function_addresses( { EBPF_LOG_ENTRY(); ebpf_result_t result = EBPF_SUCCESS; + ebpf_lock_state_t state = ebpf_lock_lock((ebpf_lock_t*)&program->lock); if (program->helper_function_count > addresses_count) { result = EBPF_INSUFFICIENT_BUFFER; @@ -1263,6 +1296,7 @@ ebpf_program_get_helper_function_addresses( } Exit: + ebpf_lock_unlock((ebpf_lock_t*)&program->lock, state); EBPF_RETURN_RESULT(result); } @@ -1564,8 +1598,8 @@ _ebpf_helper_id_to_index_compare(const void* lhs, const void* rhs) * @return EBPF_SUCCESS the program info hash matches. * @return EBPF_INVALID_ARGUMENT the program info hash does not match. */ -static ebpf_result_t -_ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program) +_Requires_lock_held_(program->lock) static ebpf_result_t + _ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program) { EBPF_LOG_ENTRY(); ebpf_result_t result; @@ -1939,15 +1973,14 @@ ebpf_program_execute_test_run( _Must_inspect_result_ ebpf_result_t ebpf_program_register_for_helper_changes( _Inout_ ebpf_program_t* program, - _In_ ebpf_helper_function_addresses_changed_callback_t callback, + _In_opt_ ebpf_helper_function_addresses_changed_callback_t callback, _In_opt_ void* context) { - if (program->helper_function_addresses_changed_callback != NULL) { - return EBPF_INVALID_ARGUMENT; - } + ebpf_lock_state_t state = ebpf_lock_lock((ebpf_lock_t*)&program->lock); program->helper_function_addresses_changed_callback = callback; program->helper_function_addresses_changed_context = context; + ebpf_lock_unlock((ebpf_lock_t*)&program->lock, state); return EBPF_SUCCESS; } diff --git a/libs/execution_context/ebpf_program.h b/libs/execution_context/ebpf_program.h index b80e3c7241..c0992ce42f 100644 --- a/libs/execution_context/ebpf_program.h +++ b/libs/execution_context/ebpf_program.h @@ -351,7 +351,7 @@ extern "C" _In_ ebpf_program_test_run_complete_callback_t callback); typedef ebpf_result_t (*ebpf_helper_function_addresses_changed_callback_t)( - _Inout_ ebpf_program_t* program, _In_opt_ void* context); + size_t address_count, _In_reads_opt_(address_count) uintptr_t* addresses, _In_opt_ void* context); /** * @brief Register to be notified when the helper function addresses change. @@ -365,7 +365,7 @@ extern "C" _Must_inspect_result_ ebpf_result_t ebpf_program_register_for_helper_changes( _Inout_ ebpf_program_t* program, - _In_ ebpf_helper_function_addresses_changed_callback_t callback, + _In_opt_ ebpf_helper_function_addresses_changed_callback_t callback, _In_opt_ void* context); /** From 6179329140fc8ee51e5b86a27431ad8668e00c26 Mon Sep 17 00:00:00 2001 From: Alan Jowett Date: Fri, 14 Apr 2023 09:06:15 -0700 Subject: [PATCH 2/3] Fix code analysis failure Signed-off-by: Alan Jowett --- libs/execution_context/ebpf_program.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/libs/execution_context/ebpf_program.c b/libs/execution_context/ebpf_program.c index 316a40af00..85652f25e0 100644 --- a/libs/execution_context/ebpf_program.c +++ b/libs/execution_context/ebpf_program.c @@ -25,7 +25,7 @@ typedef struct _ebpf_program { ebpf_core_object_t object; - ebpf_program_parameters_t parameters; + _Guarded_by_(lock) ebpf_program_parameters_t parameters; // determinant is parameters.code_type union @@ -116,8 +116,8 @@ _Requires_lock_not_held_(program->lock) static void _ebpf_program_detach_links(_ EBPF_RETURN_VOID(); } -_Requires_lock_held_(program->lock) static ebpf_result_t - _ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program); +static ebpf_result_t +_ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program); static ebpf_result_t _ebpf_program_program_info_provider_changed( @@ -1598,14 +1598,16 @@ _ebpf_helper_id_to_index_compare(const void* lhs, const void* rhs) * @return EBPF_SUCCESS the program info hash matches. * @return EBPF_INVALID_ARGUMENT the program info hash does not match. */ -_Requires_lock_held_(program->lock) static ebpf_result_t - _ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program) +static ebpf_result_t +_ebpf_program_initialize_or_verify_program_info_hash(_Inout_ ebpf_program_t* program) { EBPF_LOG_ENTRY(); ebpf_result_t result; ebpf_cryptographic_hash_t* cryptographic_hash = NULL; ebpf_helper_id_to_index_t* helper_id_to_index = NULL; ebpf_program_info_t* program_info = NULL; + ebpf_lock_state_t state = 0; + bool lock_held = false; result = ebpf_program_get_program_info(program, &program_info); if (result != EBPF_SUCCESS) { @@ -1727,7 +1729,8 @@ _Requires_lock_held_(program->lock) static ebpf_result_t if (result != EBPF_SUCCESS) { goto Exit; } - + state = ebpf_lock_lock(&program->lock); + lock_held = true; if (program->parameters.program_info_hash_length == 0) { // This is the first time the program info hash is being computed. uint8_t* new_hash = ebpf_allocate(output_hash_length); @@ -1753,6 +1756,10 @@ _Requires_lock_held_(program->lock) static ebpf_result_t result = EBPF_SUCCESS; Exit: + if (lock_held) { + ebpf_lock_unlock(&program->lock, state); + } + ebpf_free(helper_id_to_index); ebpf_cryptographic_hash_destroy(cryptographic_hash); ebpf_program_free_program_info((ebpf_program_info_t*)program_info); From ad63e6391baeae21d912ec5239a58bb356c9b8f4 Mon Sep 17 00:00:00 2001 From: Alan Jowett Date: Fri, 14 Apr 2023 13:14:52 -0700 Subject: [PATCH 3/3] Fix code analysis failure Signed-off-by: Alan Jowett --- libs/execution_context/ebpf_program.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/execution_context/ebpf_program.c b/libs/execution_context/ebpf_program.c index 85652f25e0..d930ff19c4 100644 --- a/libs/execution_context/ebpf_program.c +++ b/libs/execution_context/ebpf_program.c @@ -209,6 +209,7 @@ _ebpf_program_program_info_provider_changed( if (return_value != EBPF_SUCCESS) { EBPF_LOG_MESSAGE( EBPF_TRACELOG_LEVEL_ERROR, EBPF_TRACELOG_KEYWORD_PROGRAM, "Failed to update helpers for program"); + ebpf_lock_unlock(&program->lock, state); goto Exit; }