From 5082727e550583dd1c64d6aa12ea8d06d7296f6c Mon Sep 17 00:00:00 2001 From: Yunchao He Date: Fri, 16 Dec 2022 16:46:12 +0000 Subject: [PATCH] D3D12: Remove the 24K extra memory for texture corruption 2D array texture may corrupt on some Intel devices, making out-of-bound texture access and memory information leak. It's a critical security issue. Intel driver team suggested the 24K extra memory approach in order to mitigate the security issue before. However, the texture corruption issue (and even the correctness issue) can be worked around via allocating a few extra layers. And patches have already been merged in Dawn, with a lot tests for verification. The 24K extra memory for each texture is actually incorrect and unnecessary. So this patch removes relevant code in Dawn. This patch mainly reverts some code of this patch below: https://dawn-review.googlesource.com/c/dawn/+/96220 Bug: dawn:1507 Change-Id: Ic3239115ad4c74bdee928577ccbb20f1e35d13c3 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/114641 Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Kokoro: Kokoro Commit-Queue: Yunchao He --- .../d3d12/ResourceAllocatorManagerD3D12.cpp | 70 ++++--------------- .../d3d12/ResourceAllocatorManagerD3D12.h | 9 --- 2 files changed, 14 insertions(+), 65 deletions(-) diff --git a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp index 5bcbaf1192..a4329e6592 100644 --- a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp +++ b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.cpp @@ -25,8 +25,6 @@ #include "dawn/native/d3d12/ResidencyManagerD3D12.h" #include "dawn/native/d3d12/UtilsD3D12.h" -static constexpr uint32_t kExtraMemoryToMitigateTextureCorruption = 24576u; - namespace dawn::native::d3d12 { namespace { MemorySegment GetMemorySegment(Device* device, D3D12_HEAP_TYPE heapType) { @@ -475,8 +473,6 @@ ResultOrError ResourceAllocatorManager::CreatePlacedReso mDevice->GetD3D12Device()->GetResourceAllocationInfo(0, 1, &resourceDescriptor); } - resourceInfo.SizeInBytes += GetResourcePadding(resourceDescriptor); - // If d3d tells us the resource size is invalid, treat the error as OOM. // Otherwise, creating the resource could cause a device loss (too large). // This is because NextPowerOfTwo(UINT64_MAX) overflows and proceeds to @@ -499,21 +495,6 @@ ResultOrError ResourceAllocatorManager::CreatePlacedReso Heap* heap = ToBackend(allocation.GetResourceHeap()); - ComPtr placedResource; - DAWN_TRY_ASSIGN(placedResource, - CreatePlacedResourceInHeap(heap, allocation.GetOffset(), resourceDescriptor, - optimizedClearValue, initialUsage)); - return ResourceHeapAllocation{allocation.GetInfo(), allocation.GetOffset(), - std::move(placedResource), heap}; -} - -ResultOrError> ResourceAllocatorManager::CreatePlacedResourceInHeap( - Heap* heap, - const uint64_t offset, - const D3D12_RESOURCE_DESC& resourceDescriptor, - const D3D12_CLEAR_VALUE* optimizedClearValue, - D3D12_RESOURCE_STATES initialUsage) { - ComPtr placedResource; // Before calling CreatePlacedResource, we must ensure the target heap is resident. // CreatePlacedResource will fail if it is not. DAWN_TRY(mDevice->GetResidencyManager()->LockAllocation(heap)); @@ -525,16 +506,19 @@ ResultOrError> ResourceAllocatorManager::CreatePlacedReso // within the same command-list and does not require additional synchronization (aliasing // barrier). // https://docs.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12device-createplacedresource - DAWN_TRY( - CheckOutOfMemoryHRESULT(mDevice->GetD3D12Device()->CreatePlacedResource( - heap->GetD3D12Heap(), offset, &resourceDescriptor, initialUsage, - optimizedClearValue, IID_PPV_ARGS(&placedResource)), - "ID3D12Device::CreatePlacedResource")); + ComPtr placedResource; + DAWN_TRY(CheckOutOfMemoryHRESULT( + mDevice->GetD3D12Device()->CreatePlacedResource( + heap->GetD3D12Heap(), allocation.GetOffset(), &resourceDescriptor, initialUsage, + optimizedClearValue, IID_PPV_ARGS(&placedResource)), + "ID3D12Device::CreatePlacedResource")); // After CreatePlacedResource has finished, the heap can be unlocked from residency. This // will insert it into the residency LRU. mDevice->GetResidencyManager()->UnlockAllocation(heap); - return std::move(placedResource); + + return ResourceHeapAllocation{allocation.GetInfo(), allocation.GetOffset(), + std::move(placedResource), heap}; } ResultOrError ResourceAllocatorManager::CreateCommittedResource( @@ -556,9 +540,6 @@ ResultOrError ResourceAllocatorManager::CreateCommittedR D3D12_RESOURCE_ALLOCATION_INFO resourceInfo = mDevice->GetD3D12Device()->GetResourceAllocationInfo(0, 1, &resourceDescriptor); - uint64_t extraMemory = GetResourcePadding(resourceDescriptor); - resourceInfo.SizeInBytes += extraMemory; - if (resourceInfo.SizeInBytes == 0 || resourceInfo.SizeInBytes == std::numeric_limits::max()) { return DAWN_OUT_OF_MEMORY_ERROR("Resource allocation size was invalid."); @@ -577,23 +558,11 @@ ResultOrError ResourceAllocatorManager::CreateCommittedR // Note: Heap flags are inferred by the resource descriptor and do not need to be explicitly // provided to CreateCommittedResource. ComPtr committedResource; - if (extraMemory > 0) { - const ResourceHeapKind resourceHeapKind = GetResourceHeapKind( - resourceDescriptor.Dimension, heapType, resourceDescriptor.Flags, mResourceHeapTier); - std::unique_ptr heapBase; - DAWN_TRY_ASSIGN(heapBase, mPooledHeapAllocators[resourceHeapKind]->AllocateResourceHeap( - resourceInfo.SizeInBytes)); - Heap* heap = ToBackend(heapBase.get()); - DAWN_TRY_ASSIGN(committedResource, - CreatePlacedResourceInHeap(heap, 0, resourceDescriptor, optimizedClearValue, - initialUsage)); - } else { - DAWN_TRY(CheckOutOfMemoryHRESULT( - mDevice->GetD3D12Device()->CreateCommittedResource( - &heapProperties, D3D12_HEAP_FLAG_NONE, &resourceDescriptor, initialUsage, - optimizedClearValue, IID_PPV_ARGS(&committedResource)), - "ID3D12Device::CreateCommittedResource")); - } + DAWN_TRY(CheckOutOfMemoryHRESULT( + mDevice->GetD3D12Device()->CreateCommittedResource( + &heapProperties, D3D12_HEAP_FLAG_NONE, &resourceDescriptor, initialUsage, + optimizedClearValue, IID_PPV_ARGS(&committedResource)), + "ID3D12Device::CreateCommittedResource")); // When using CreateCommittedResource, D3D12 creates an implicit heap that contains the // resource allocation. Because Dawn's memory residency management occurs at the resource @@ -614,17 +583,6 @@ ResultOrError ResourceAllocatorManager::CreateCommittedR /*offset*/ 0, std::move(committedResource), heap}; } -uint64_t ResourceAllocatorManager::GetResourcePadding( - const D3D12_RESOURCE_DESC& resourceDescriptor) const { - // If we are allocating memory for a 2D array texture on D3D12 backend, we need to allocate - // extra memory on some devices, see crbug.com/dawn/949 for details. - if (mDevice->IsToggleEnabled(Toggle::D3D12AllocateExtraMemoryFor2DArrayColorTexture) && - resourceDescriptor.Dimension == D3D12_RESOURCE_DIMENSION_TEXTURE2D && - resourceDescriptor.DepthOrArraySize > 1) { - return kExtraMemoryToMitigateTextureCorruption; - } - return 0; -} void ResourceAllocatorManager::DestroyPool() { for (auto& alloc : mPooledHeapAllocators) { alloc->DestroyPool(); diff --git a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h index 5d9847398c..363e590f91 100644 --- a/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h +++ b/src/dawn/native/d3d12/ResourceAllocatorManagerD3D12.h @@ -87,15 +87,6 @@ class ResourceAllocatorManager { const D3D12_CLEAR_VALUE* optimizedClearValue, D3D12_RESOURCE_STATES initialUsage); - ResultOrError> CreatePlacedResourceInHeap( - Heap* heap, - const uint64_t offset, - const D3D12_RESOURCE_DESC& resourceDescriptor, - const D3D12_CLEAR_VALUE* optimizedClearValue, - D3D12_RESOURCE_STATES initialUsage); - - uint64_t GetResourcePadding(const D3D12_RESOURCE_DESC& resourceDescriptor) const; - void DestroyPool(); Device* mDevice;