From da689bc8f91759092bf009589d1f318eda02b6f3 Mon Sep 17 00:00:00 2001 From: Aizal Khan Date: Fri, 2 Oct 2026 19:27:49 +0530 Subject: [PATCH 1/2] take loader_lock when adding unknown functions vkGetInstanceProcAddr holds no lock, so registering an unknown extension function raced vkCreateDevice and vkDestroyDevice on each driver's logical device list, and raced other queries on the instance's unknown function name tables. --- loader/unknown_function_handling.c | 28 ++++++++-- tests/loader_threading_tests.cpp | 84 ++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/loader/unknown_function_handling.c b/loader/unknown_function_handling.c index 23e75fac8..09f2e57d5 100644 --- a/loader/unknown_function_handling.c +++ b/loader/unknown_function_handling.c @@ -63,6 +63,7 @@ void loader_free_phys_dev_ext_table(struct loader_instance *inst) { (void)inst; #else #include "allocation.h" +#include "loader.h" #include "log.h" // Forward declarations @@ -210,12 +211,22 @@ void *loader_dev_ext_gpa_impl(struct loader_instance *inst, const char *funcName return out_function; } +// Main interface functions. Both hold loader_lock while registering a function: the implementation appends to the +// instance's unknown function name table and walks the driver and logical device lists, all of which +// vkCreateDevice/vkDestroyDevice/vkEnumeratePhysicalDevices mutate under that lock, while vkGetInstanceProcAddr takes +// no lock of its own. loader_lock is recursive, so callers which already hold it are unaffected. void *loader_dev_ext_gpa_tramp(struct loader_instance *inst, const char *funcName) { - return loader_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } void *loader_dev_ext_gpa_term(struct loader_instance *inst, const char *funcName) { - return loader_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } // Physical Device function handling @@ -367,12 +378,19 @@ void *loader_phys_dev_ext_gpa_impl(struct loader_instance *inst, const char *fun } return loader_get_phys_dev_ext_termin(new_function_index); } -// Main interface functions, makes it clear whether it is getting a terminator or trampoline +// Main interface functions, makes it clear whether it is getting a terminator or trampoline. Both hold loader_lock for +// the same reason as the device variants above. void *loader_phys_dev_ext_gpa_tramp(struct loader_instance *inst, const char *funcName) { - return loader_phys_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_phys_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } void *loader_phys_dev_ext_gpa_term(struct loader_instance *inst, const char *funcName) { - return loader_phys_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_phys_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } #endif diff --git a/tests/loader_threading_tests.cpp b/tests/loader_threading_tests.cpp index 499cfae55..65206ccb4 100644 --- a/tests/loader_threading_tests.cpp +++ b/tests/loader_threading_tests.cpp @@ -199,3 +199,87 @@ TEST(Threading, SurfaceCreateDestroyLoop) { surface_threads[i].join(); } } + +// The driver reports these through vk_icdGetPhysicalDeviceProcAddr, so vkGetInstanceProcAddr has to append each one to +// the loader's instance wide unknown function tables before it can hand back a trampoline. +VKAPI_ATTR uint32_t VKAPI_CALL test_unknown_phys_dev_function(VkPhysicalDevice, uint32_t foo) { return foo; } + +void get_unknown_function_loop(InstWrapper* inst, std::vector const* func_names) { + for (auto const& name : *func_names) { + PFN_vkVoidFunction func = inst->load(name.c_str()); + ASSERT_NE(func, nullptr); + } +} + +TEST(Threading, GetUnknownPhysicalDeviceFunctionLoop) { + // Capped so that thread_count * funcs_per_thread stays under MAX_NUM_UNKNOWN_EXTS on a machine with many cores + uint32_t thread_count = std::thread::hardware_concurrency(); + if (thread_count < 2) thread_count = 2; + if (thread_count > 8) thread_count = 8; + const uint32_t funcs_per_thread = 16; + + FrameworkEnvironment env{FrameworkSettings{}.set_log_filter("")}; + auto& phys_dev = env.add_icd(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).add_and_get_physical_device({}); + + // Each thread queries its own set of names, so every query appends a new entry rather than finding an existing one + std::vector> func_names{thread_count}; + for (uint32_t i = 0; i < thread_count; i++) { + for (uint32_t j = 0; j < funcs_per_thread; j++) { + func_names[i].push_back("vkNotRealFuncTEST_" + std::to_string(i) + "_" + std::to_string(j)); + phys_dev.custom_physical_device_functions.push_back( + VulkanFunction{func_names[i].back(), to_vkVoidFunction(test_unknown_phys_dev_function)}); + } + } + + InstWrapper inst{env.vulkan_functions}; + inst.CheckCreate(); + + std::vector function_query_threads; + for (uint32_t i = 0; i < thread_count; i++) { + function_query_threads.emplace_back(get_unknown_function_loop, &inst, &func_names[i]); + } + for (uint32_t i = 0; i < thread_count; i++) { + function_query_threads[i].join(); + } +} + +// Reported through the driver's vkGetInstanceProcAddr, which puts them on the unknown device function path. Registering +// one walks every driver's logical device list, which vkCreateDevice and vkDestroyDevice are editing in the other threads. +VKAPI_ATTR uint32_t VKAPI_CALL test_unknown_device_function(VkDevice, uint32_t foo) { return foo; } + +void create_destroy_device_only_loop(InstWrapper* inst, uint32_t num_loops) { + for (uint32_t i = 0; i < num_loops; i++) { + DeviceWrapper dev{*inst}; + dev.CheckCreate(inst->GetPhysDev()); + } +} + +TEST(Threading, GetUnknownDeviceFunctionWhileCreatingDevices) { + const uint32_t thread_count = 4; + const uint32_t funcs_per_thread = 16; + const uint32_t num_loops_create_destroy_device = 50; + + FrameworkEnvironment env{FrameworkSettings{}.set_log_filter("")}; + auto& phys_dev = env.add_icd(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).add_and_get_physical_device({}); + + std::vector> func_names{thread_count}; + for (uint32_t i = 0; i < thread_count; i++) { + for (uint32_t j = 0; j < funcs_per_thread; j++) { + func_names[i].push_back("vkNotRealDeviceFuncTEST_" + std::to_string(i) + "_" + std::to_string(j)); + phys_dev.known_device_functions.push_back( + VulkanFunction{func_names[i].back(), to_vkVoidFunction(test_unknown_device_function)}); + } + } + + InstWrapper inst{env.vulkan_functions}; + inst.CheckCreate(); + + std::vector threads; + for (uint32_t i = 0; i < thread_count; i++) { + threads.emplace_back(get_unknown_function_loop, &inst, &func_names[i]); + threads.emplace_back(create_destroy_device_only_loop, &inst, num_loops_create_destroy_device); + } + for (auto& thread : threads) { + thread.join(); + } +} From f20c51bbc1fd151b45646975f37ae75d8bb3e9f8 Mon Sep 17 00:00:00 2001 From: Charles Giessen Date: Fri, 2 Oct 2026 11:04:08 -0500 Subject: [PATCH 2/2] Disable threading unknown function tests in no-asm CI builds --- .github/workflows/build.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1abfe5bac..7b1ccf114 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -112,7 +112,7 @@ jobs: -D CMAKE_CXX_COMPILER=clang++ - run: cmake --build build - run: cmake --install build --prefix /tmp - - run: ctest --parallel --output-on-failure -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ linux-32: needs: codegen @@ -198,7 +198,7 @@ jobs: # https://gitlab.kitware.com/cmake/cmake/-/issues/25317 PKG_CONFIG_PATH: /usr/lib/i386-linux-gnu/pkgconfig - run: cmake --build build - - run: ctest --parallel --output-on-failure -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ linux-arm: needs: codegen @@ -310,7 +310,7 @@ jobs: -A ${{ matrix.arch }} ` -D BUILD_WERROR=ON - run: cmake --build build/ --config Release - - run: ctest --parallel --output-on-failure -C Release -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure -C Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ windows_arm: # Native Windows on Arm: covers the arm64 and arm64ec MARMASM paths the x64 runners can't. @@ -531,7 +531,7 @@ jobs: env: LDFLAGS: -Wl,-fatal_warnings - run: cmake --build build --config Release - - run: ctest --parallel --output-on-failure --build-config Release -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure --build-config Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ - run: cmake --install build --config Release --prefix /tmp - name: Verify Universal Binary if: ${{ matrix.static == 'OFF' }} @@ -568,7 +568,7 @@ jobs: env: LDFLAGS: -Wl,-fatal_warnings - run: cmake --build build --config Release - - run: ctest --parallel --output-on-failure --build-config Release -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure --build-config Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ - run: cmake --install build --config Release --prefix /tmp - name: Verify Universal Binary if: ${{ matrix.static == 'OFF' }}