From 97650ef6608c57a58756a12543c4cff995e96417 Mon Sep 17 00:00:00 2001 From: Callum Fare Date: Thu, 29 Aug 2024 10:06:43 +0100 Subject: [PATCH] Revert "Merge pull request #1843 from AllanZyne/review/yang/invalid_arguments" This reverts commit 61a67b3e04d604d6eba601eeb30f2b75a29db903. --- source/loader/CMakeLists.txt | 2 - .../layers/sanitizer/asan_interceptor.cpp | 23 +---- .../layers/sanitizer/asan_interceptor.hpp | 5 - .../loader/layers/sanitizer/asan_options.hpp | 2 - .../loader/layers/sanitizer/asan_report.cpp | 92 ++++++------------- .../loader/layers/sanitizer/asan_report.hpp | 5 - .../layers/sanitizer/asan_validator.cpp | 77 ---------------- .../layers/sanitizer/asan_validator.hpp | 50 ---------- source/loader/layers/sanitizer/ur_sanddi.cpp | 44 +-------- .../layers/sanitizer/ur_sanitizer_utils.cpp | 17 ---- .../layers/sanitizer/ur_sanitizer_utils.hpp | 3 - 11 files changed, 28 insertions(+), 292 deletions(-) delete mode 100644 source/loader/layers/sanitizer/asan_validator.cpp delete mode 100644 source/loader/layers/sanitizer/asan_validator.hpp diff --git a/source/loader/CMakeLists.txt b/source/loader/CMakeLists.txt index edfd8b055d..5b5cfea201 100644 --- a/source/loader/CMakeLists.txt +++ b/source/loader/CMakeLists.txt @@ -142,8 +142,6 @@ if(UR_ENABLE_SANITIZER) ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/asan_report.hpp ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/asan_shadow_setup.cpp ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/asan_shadow_setup.hpp - ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/asan_validator.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/asan_validator.hpp ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/common.hpp ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/stacktrace.cpp ${CMAKE_CURRENT_SOURCE_DIR}/layers/sanitizer/stacktrace.hpp diff --git a/source/loader/layers/sanitizer/asan_interceptor.cpp b/source/loader/layers/sanitizer/asan_interceptor.cpp index ec1d5e8fad..1ccc85aa86 100644 --- a/source/loader/layers/sanitizer/asan_interceptor.cpp +++ b/source/loader/layers/sanitizer/asan_interceptor.cpp @@ -16,7 +16,6 @@ #include "asan_quarantine.hpp" #include "asan_report.hpp" #include "asan_shadow_setup.hpp" -#include "asan_validator.hpp" #include "stacktrace.hpp" #include "ur_sanitizer_utils.hpp" @@ -628,9 +627,6 @@ SanitizerInterceptor::insertDevice(ur_device_handle_t Device, DI = std::make_shared(Device); - DI->IsSupportSharedSystemUSM = GetDeviceUSMCapability( - Device, UR_DEVICE_INFO_USM_SYSTEM_SHARED_SUPPORT); - // Query alignment UR_CALL(getContext()->urDdiTable.Device.pfnGetInfo( Device, UR_DEVICE_INFO_MEM_BASE_ADDR_ALIGN, sizeof(DI->Alignment), @@ -699,25 +695,8 @@ ur_result_t SanitizerInterceptor::prepareLaunch( auto Program = GetProgram(Kernel); do { - auto KernelInfo = getKernelInfo(Kernel); - - // Validate pointer arguments - if (Options(logger).DetectKernelArguments) { - for (const auto &[ArgIndex, PtrPair] : KernelInfo->PointerArgs) { - auto Ptr = PtrPair.first; - if (Ptr == nullptr) { - continue; - } - if (auto ValidateResult = ValidateUSMPointer( - Context, DeviceInfo->Handle, (uptr)Ptr)) { - ReportInvalidKernelArgument(Kernel, ArgIndex, (uptr)Ptr, - ValidateResult, PtrPair.second); - exit(1); - } - } - } - // Set membuffer arguments + auto KernelInfo = getKernelInfo(Kernel); for (const auto &[ArgIndex, MemBuffer] : KernelInfo->BufferArgs) { char *ArgPointer = nullptr; UR_CALL(MemBuffer->getHandle(DeviceInfo->Handle, ArgPointer)); diff --git a/source/loader/layers/sanitizer/asan_interceptor.hpp b/source/loader/layers/sanitizer/asan_interceptor.hpp index 1c87cdc8e1..890e038fe7 100644 --- a/source/loader/layers/sanitizer/asan_interceptor.hpp +++ b/source/loader/layers/sanitizer/asan_interceptor.hpp @@ -42,9 +42,6 @@ struct DeviceInfo { uptr ShadowOffset = 0; uptr ShadowOffsetEnd = 0; - // Device features - bool IsSupportSharedSystemUSM = false; - ur_mutex Mutex; std::queue> Quarantine; size_t QuarantineSize = 0; @@ -81,8 +78,6 @@ struct KernelInfo { ur_shared_mutex Mutex; std::atomic RefCount = 1; std::unordered_map> BufferArgs; - std::unordered_map> - PointerArgs; // Need preserve the order of local arguments std::map LocalArgs; diff --git a/source/loader/layers/sanitizer/asan_options.hpp b/source/loader/layers/sanitizer/asan_options.hpp index 298639b73c..ad4bc00c8b 100644 --- a/source/loader/layers/sanitizer/asan_options.hpp +++ b/source/loader/layers/sanitizer/asan_options.hpp @@ -38,7 +38,6 @@ struct AsanOptions { uint32_t MaxQuarantineSizeMB = 0; bool DetectLocals = true; bool DetectPrivates = true; - bool DetectKernelArguments = true; private: AsanOptions(logger::Logger &logger) { @@ -94,7 +93,6 @@ struct AsanOptions { SetBoolOption("debug", Debug); SetBoolOption("detect_locals", DetectLocals); SetBoolOption("detect_privates", DetectPrivates); - SetBoolOption("detect_kernel_arguments", DetectKernelArguments); auto KV = OptionsEnvMap->find("quarantine_size_mb"); if (KV != OptionsEnvMap->end()) { diff --git a/source/loader/layers/sanitizer/asan_report.cpp b/source/loader/layers/sanitizer/asan_report.cpp index a92e93f979..54e1da40e9 100644 --- a/source/loader/layers/sanitizer/asan_report.cpp +++ b/source/loader/layers/sanitizer/asan_report.cpp @@ -11,32 +11,16 @@ */ #include "asan_report.hpp" +#include "asan_options.hpp" + #include "asan_allocator.hpp" #include "asan_interceptor.hpp" #include "asan_libdevice.hpp" -#include "asan_options.hpp" -#include "asan_validator.hpp" #include "ur_sanitizer_layer.hpp" #include "ur_sanitizer_utils.hpp" namespace ur_sanitizer_layer { -namespace { - -void PrintAllocateInfo(uptr Addr, const AllocInfo *AI) { - getContext()->logger.always("{} is located inside of {} region [{}, {})", - (void *)Addr, ToString(AI->Type), - (void *)AI->UserBegin, (void *)AI->UserEnd); - getContext()->logger.always("allocated here:"); - AI->AllocStack.print(); - if (AI->IsReleased) { - getContext()->logger.always("freed here:"); - AI->ReleaseStack.print(); - } -} - -} // namespace - void ReportBadFree(uptr Addr, const StackTrace &stack, const std::shared_ptr &AI) { getContext()->logger.always( @@ -50,7 +34,11 @@ void ReportBadFree(uptr Addr, const StackTrace &stack, assert(AI && !AI->IsReleased && "Chunk must be not released"); - PrintAllocateInfo(Addr, AI.get()); + getContext()->logger.always("{} is located inside of {} region [{}, {})", + (void *)Addr, ToString(AI->Type), + (void *)AI->UserBegin, (void *)AI->UserEnd); + getContext()->logger.always("allocated here:"); + AI->AllocStack.print(); } void ReportBadContext(uptr Addr, const StackTrace &stack, @@ -60,7 +48,16 @@ void ReportBadContext(uptr Addr, const StackTrace &stack, (void *)Addr); stack.print(); - PrintAllocateInfo(Addr, AI.get()); + getContext()->logger.always("{} is located inside of {} region [{}, {})", + (void *)Addr, ToString(AI->Type), + (void *)AI->UserBegin, (void *)AI->UserEnd); + getContext()->logger.always("allocated here:"); + AI->AllocStack.print(); + + if (AI->IsReleased) { + getContext()->logger.always("freed here:"); + AI->ReleaseStack.print(); + } } void ReportDoubleFree(uptr Addr, const StackTrace &Stack, @@ -142,10 +139,16 @@ void ReportUseAfterFree(const DeviceSanitizerReport &Report, "Failed to find which chunck {} is allocated", (void *)Report.Address); } - assert(AllocInfo->IsReleased && - "It must be released since it's use-after-free"); + assert(AllocInfo->IsReleased); - PrintAllocateInfo(Report.Address, AllocInfo.get()); + getContext()->logger.always( + "{} is located inside of {} region [{}, {})", + (void *)Report.Address, ToString(AllocInfo->Type), + (void *)AllocInfo->UserBegin, (void *)AllocInfo->UserEnd); + getContext()->logger.always("allocated here:"); + AllocInfo->AllocStack.print(); + getContext()->logger.always("released here:"); + AllocInfo->ReleaseStack.print(); } } else { getContext()->logger.always( @@ -154,47 +157,4 @@ void ReportUseAfterFree(const DeviceSanitizerReport &Report, } } -void ReportInvalidKernelArgument(ur_kernel_handle_t Kernel, uint32_t ArgIndex, - uptr Addr, const ValidateUSMResult &VR, - StackTrace Stack) { - getContext()->logger.always("\n====ERROR: DeviceSanitizer: " - "invalid-argument on kernel <{}>", - DemangleName(GetKernelName(Kernel))); - Stack.print(); - auto &AI = VR.AI; - switch (VR.Type) { - case ValidateUSMResult::MAYBE_HOST_POINTER: - getContext()->logger.always("The {}th argument {} is not a USM pointer", - ArgIndex + 1, (void *)Addr); - break; - case ValidateUSMResult::RELEASED_POINTER: - getContext()->logger.always( - "The {}th argument {} is a released USM pointer", ArgIndex, - (void *)Addr); - PrintAllocateInfo(Addr, AI.get()); - break; - case ValidateUSMResult::BAD_CONTEXT: - getContext()->logger.always( - "The {}th argument {} is allocated in other context", ArgIndex, - (void *)Addr); - PrintAllocateInfo(Addr, AI.get()); - break; - case ValidateUSMResult::BAD_DEVICE: - getContext()->logger.always( - "The {}th argument {} is allocated in other device", ArgIndex, - (void *)Addr); - PrintAllocateInfo(Addr, AI.get()); - break; - case ValidateUSMResult::OUT_OF_BOUNDS: - getContext()->logger.always( - "The {}th argument {} is located outside of its region [{}, {})", - ArgIndex, (void *)Addr, (void *)AI->UserBegin, (void *)AI->UserEnd); - getContext()->logger.always("allocated here:"); - AI->AllocStack.print(); - break; - default: - break; - } -} - } // namespace ur_sanitizer_layer diff --git a/source/loader/layers/sanitizer/asan_report.hpp b/source/loader/layers/sanitizer/asan_report.hpp index 0dd8f346d0..77a182b0e6 100644 --- a/source/loader/layers/sanitizer/asan_report.hpp +++ b/source/loader/layers/sanitizer/asan_report.hpp @@ -21,7 +21,6 @@ namespace ur_sanitizer_layer { struct DeviceSanitizerReport; struct AllocInfo; struct StackTrace; -struct ValidateUSMResult; void ReportBadFree(uptr Addr, const StackTrace &stack, const std::shared_ptr &AllocInfo); @@ -41,8 +40,4 @@ void ReportGenericError(const DeviceSanitizerReport &Report, void ReportUseAfterFree(const DeviceSanitizerReport &Report, ur_kernel_handle_t Kernel, ur_context_handle_t Context); -void ReportInvalidKernelArgument(ur_kernel_handle_t Kernel, uint32_t ArgIndex, - uptr Addr, const ValidateUSMResult &VR, - StackTrace Stack); - } // namespace ur_sanitizer_layer diff --git a/source/loader/layers/sanitizer/asan_validator.cpp b/source/loader/layers/sanitizer/asan_validator.cpp deleted file mode 100644 index a9f2bd2b17..0000000000 --- a/source/loader/layers/sanitizer/asan_validator.cpp +++ /dev/null @@ -1,77 +0,0 @@ -/* - * - * Copyright (C) 2024 Intel Corporation - * - * Part of the Unified-Runtime Project, under the Apache License v2.0 with LLVM Exceptions. - * See LICENSE.TXT - * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - * - * @file asan_validator.cpp - * - */ - -#include "asan_validator.hpp" -#include "asan_interceptor.hpp" -#include "ur_sanitizer_utils.hpp" - -namespace ur_sanitizer_layer { - -namespace { - -bool IsSameDevice(ur_device_handle_t Device1, ur_device_handle_t Device2) { - if (Device1 == Device2) { - return true; - } - auto RootDevice1 = GetParentDevice(Device1); - RootDevice1 = RootDevice1 ? RootDevice1 : Device1; - auto RootDevice2 = GetParentDevice(Device2); - RootDevice2 = RootDevice2 ? RootDevice2 : Device2; - if (RootDevice1 == RootDevice2) { - return true; - } - return false; -} - -} // namespace - -ValidateUSMResult ValidateUSMPointer(ur_context_handle_t Context, - ur_device_handle_t Device, uptr Ptr) { - assert(Ptr != 0 && "Don't validate nullptr here"); - - auto AllocInfoItOp = getContext()->interceptor->findAllocInfoByAddress(Ptr); - if (!AllocInfoItOp) { - auto DI = getContext()->interceptor->getDeviceInfo(Device); - bool IsSupportSharedSystemUSM = DI->IsSupportSharedSystemUSM; - if (IsSupportSharedSystemUSM) { - // maybe it's host pointer - return ValidateUSMResult::success(); - } - return ValidateUSMResult::fail(ValidateUSMResult::MAYBE_HOST_POINTER); - } - - auto AllocInfo = AllocInfoItOp.value()->second; - - if (AllocInfo->Context != Context) { - return ValidateUSMResult::fail(ValidateUSMResult::BAD_CONTEXT, - AllocInfo); - } - - if (AllocInfo->Device && !IsSameDevice(AllocInfo->Device, Device)) { - return ValidateUSMResult::fail(ValidateUSMResult::BAD_DEVICE, - AllocInfo); - } - - if (AllocInfo->IsReleased) { - return ValidateUSMResult::fail(ValidateUSMResult::RELEASED_POINTER, - AllocInfo); - } - - if (Ptr < AllocInfo->UserBegin || Ptr >= AllocInfo->UserEnd) { - return ValidateUSMResult::fail(ValidateUSMResult::OUT_OF_BOUNDS, - AllocInfo); - } - - return ValidateUSMResult::success(); -} - -} // namespace ur_sanitizer_layer diff --git a/source/loader/layers/sanitizer/asan_validator.hpp b/source/loader/layers/sanitizer/asan_validator.hpp deleted file mode 100644 index 52db966562..0000000000 --- a/source/loader/layers/sanitizer/asan_validator.hpp +++ /dev/null @@ -1,50 +0,0 @@ -/* - * - * Copyright (C) 2024 Intel Corporation - * - * Part of the Unified-Runtime Project, under the Apache License v2.0 with LLVM Exceptions. - * See LICENSE.TXT - * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - * - * @file asan_validator.hpp - * - */ -#pragma once - -#include "asan_allocator.hpp" - -namespace ur_sanitizer_layer { - -struct ValidateUSMResult { - enum ErrorType { - SUCCESS, - NULL_POINTER, - MAYBE_HOST_POINTER, - RELEASED_POINTER, - BAD_CONTEXT, - BAD_DEVICE, - OUT_OF_BOUNDS - }; - ErrorType Type; - std::shared_ptr AI; - - operator bool() { return Type != SUCCESS; } - - static ValidateUSMResult success() { return {SUCCESS, nullptr}; } - - static ValidateUSMResult fail(ErrorType Type, - const std::shared_ptr &AI) { - assert(Type != SUCCESS && "The error type shouldn't be SUCCESS"); - return {Type, AI}; - } - - static ValidateUSMResult fail(ErrorType Type) { - assert(Type != SUCCESS && "The error type shouldn't be SUCCESS"); - return {Type, nullptr}; - } -}; - -ValidateUSMResult ValidateUSMPointer(ur_context_handle_t Context, - ur_device_handle_t Device, uptr Ptr); - -} // namespace ur_sanitizer_layer diff --git a/source/loader/layers/sanitizer/ur_sanddi.cpp b/source/loader/layers/sanitizer/ur_sanddi.cpp index e5e963806b..69dbe823b5 100644 --- a/source/loader/layers/sanitizer/ur_sanddi.cpp +++ b/source/loader/layers/sanitizer/ur_sanddi.cpp @@ -11,13 +11,9 @@ */ #include "asan_interceptor.hpp" -#include "asan_options.hpp" -#include "stacktrace.hpp" #include "ur_sanitizer_layer.hpp" #include "ur_sanitizer_utils.hpp" -#include - namespace ur_sanitizer_layer { namespace { @@ -35,11 +31,7 @@ ur_result_t setupContext(ur_context_handle_t Context, uint32_t numDevices, getContext()->logger.error("Unsupport device"); return UR_RESULT_ERROR_INVALID_DEVICE; } - getContext()->logger.info( - "DeviceInfo {} (Type={}, IsSupportSharedSystemUSM={})", - (void *)DI->Handle, ToString(DI->Type), - DI->IsSupportSharedSystemUSM); - getContext()->logger.info("Add {} into context {}", (void *)DI->Handle, + getContext()->logger.info("Add {} into context {}", ToString(DI->Type), (void *)Context); if (!DI->ShadowOffset) { UR_CALL(DI->allocShadowMemory(Context)); @@ -1336,39 +1328,6 @@ __urdlllocal ur_result_t UR_APICALL urKernelSetArgLocal( return result; } -/////////////////////////////////////////////////////////////////////////////// -/// @brief Intercept function for urKernelSetArgPointer -__urdlllocal ur_result_t UR_APICALL urKernelSetArgPointer( - ur_kernel_handle_t hKernel, ///< [in] handle of the kernel object - uint32_t argIndex, ///< [in] argument index in range [0, num args - 1] - const ur_kernel_arg_pointer_properties_t - *pProperties, ///< [in][optional] pointer to USM pointer properties. - const void * - pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory - ///< mapping operation. If null then argument value is considered null. -) { - auto pfnSetArgPointer = getContext()->urDdiTable.Kernel.pfnSetArgPointer; - - if (nullptr == pfnSetArgPointer) { - return UR_RESULT_ERROR_UNSUPPORTED_FEATURE; - } - - getContext()->logger.debug( - "==== urKernelSetArgPointer (argIndex={}, pArgValue={})", argIndex, - pArgValue); - - if (Options(getContext()->logger).DetectKernelArguments) { - auto KI = getContext()->interceptor->getKernelInfo(hKernel); - std::scoped_lock Guard(KI->Mutex); - KI->PointerArgs[argIndex] = {pArgValue, GetCurrentBacktrace()}; - } - - ur_result_t result = - pfnSetArgPointer(hKernel, argIndex, pProperties, pArgValue); - - return result; -} - /////////////////////////////////////////////////////////////////////////////// /// @brief Exported function for filling application's Global table /// with current process' addresses @@ -1495,7 +1454,6 @@ __urdlllocal ur_result_t UR_APICALL urGetKernelProcAddrTable( pDdiTable->pfnSetArgValue = ur_sanitizer_layer::urKernelSetArgValue; pDdiTable->pfnSetArgMemObj = ur_sanitizer_layer::urKernelSetArgMemObj; pDdiTable->pfnSetArgLocal = ur_sanitizer_layer::urKernelSetArgLocal; - pDdiTable->pfnSetArgPointer = ur_sanitizer_layer::urKernelSetArgPointer; return result; } diff --git a/source/loader/layers/sanitizer/ur_sanitizer_utils.cpp b/source/loader/layers/sanitizer/ur_sanitizer_utils.cpp index feaff8757a..2dd98b945d 100644 --- a/source/loader/layers/sanitizer/ur_sanitizer_utils.cpp +++ b/source/loader/layers/sanitizer/ur_sanitizer_utils.cpp @@ -152,23 +152,6 @@ DeviceType GetDeviceType(ur_context_handle_t Context, } } -ur_device_handle_t GetParentDevice(ur_device_handle_t Device) { - ur_device_handle_t ParentDevice{}; - [[maybe_unused]] auto Result = getContext()->urDdiTable.Device.pfnGetInfo( - Device, UR_DEVICE_INFO_PARENT_DEVICE, sizeof(ur_device_handle_t), - &ParentDevice, nullptr); - assert(Result == UR_RESULT_SUCCESS && "getParentDevice() failed"); - return ParentDevice; -} - -bool GetDeviceUSMCapability(ur_device_handle_t Device, - ur_device_info_t USMInfo) { - ur_device_usm_access_capability_flags_t Flag; - [[maybe_unused]] auto Result = getContext()->urDdiTable.Device.pfnGetInfo( - Device, USMInfo, sizeof(Flag), &Flag, nullptr); - return (bool)Flag; -} - std::vector GetProgramDevices(ur_program_handle_t Program) { size_t PropSize; [[maybe_unused]] ur_result_t Result = diff --git a/source/loader/layers/sanitizer/ur_sanitizer_utils.hpp b/source/loader/layers/sanitizer/ur_sanitizer_utils.hpp index 44ddf46922..92cb4cebc4 100644 --- a/source/loader/layers/sanitizer/ur_sanitizer_utils.hpp +++ b/source/loader/layers/sanitizer/ur_sanitizer_utils.hpp @@ -36,9 +36,6 @@ ur_context_handle_t GetContext(ur_kernel_handle_t Kernel); ur_device_handle_t GetDevice(ur_queue_handle_t Queue); DeviceType GetDeviceType(ur_context_handle_t Context, ur_device_handle_t Device); -ur_device_handle_t GetParentDevice(ur_device_handle_t Device); -bool GetDeviceUSMCapability(ur_device_handle_t Device, - ur_device_info_t Feature); std::string GetKernelName(ur_kernel_handle_t Kernel); size_t GetDeviceLocalMemorySize(ur_device_handle_t Device); ur_program_handle_t GetProgram(ur_kernel_handle_t Kernel);