From da000ea2efc794e26a7b24e51437b1b31822ea3b Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Fri, 6 Aug 2021 11:50:41 -0700 Subject: [PATCH] venus: free queues after vkDestroyDevice is emitted Otherwise, another thread might reuse their object ids for other objects. For example, T1: free queue with object id X T2: reuse id X T2: emit vkCreateFoo with id X T1: emit vkDestroyDevice virglrenderer happily accepts that which leads to double frees of the queue: once when X is updated to point to another object and once when vkDestroyDevice is executed. virglrenderer should be fixed to catch such invalid object id reuse as well. Fixes dEQP-VK.api.object_management.multithreaded_shared_resources.device_group. Fixes: ddd75330559 ("venus: initial support for queue/fence/semaphore") Signed-off-by: Chia-I Wu Reviewed-by: Ryan Neph Reviewed-by: Yiwei Zhang Part-of: --- src/virtio/vulkan/vn_command_buffer.c | 5 +++++ src/virtio/vulkan/vn_descriptor_set.c | 4 ++++ src/virtio/vulkan/vn_device.c | 7 ++++++- 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/virtio/vulkan/vn_command_buffer.c b/src/virtio/vulkan/vn_command_buffer.c index afb2ed71299..de6bd7cac05 100644 --- a/src/virtio/vulkan/vn_command_buffer.c +++ b/src/virtio/vulkan/vn_command_buffer.c @@ -474,6 +474,11 @@ vn_DestroyCommandPool(VkDevice device, alloc = pAllocator ? pAllocator : &pool->allocator; + /* We must emit vkDestroyCommandPool before freeing the command buffers in + * pool->command_buffers. Otherwise, another thread might reuse their + * object ids while they still refer to the command buffers in the + * renderer. + */ vn_async_vkDestroyCommandPool(dev->instance, device, commandPool, NULL); list_for_each_entry_safe(struct vn_command_buffer, cmd, diff --git a/src/virtio/vulkan/vn_descriptor_set.c b/src/virtio/vulkan/vn_descriptor_set.c index 1f6376e8f32..d50c07056f9 100644 --- a/src/virtio/vulkan/vn_descriptor_set.c +++ b/src/virtio/vulkan/vn_descriptor_set.c @@ -189,6 +189,10 @@ vn_DestroyDescriptorPool(VkDevice device, alloc = pAllocator ? pAllocator : &pool->allocator; + /* We must emit vkDestroyDescriptorPool before freeing the sets in + * pool->descriptor_sets. Otherwise, another thread might reuse their + * object ids while they still refer to the sets in the renderer. + */ vn_async_vkDestroyDescriptorPool(dev->instance, device, descriptorPool, NULL); diff --git a/src/virtio/vulkan/vn_device.c b/src/virtio/vulkan/vn_device.c index e502a85315f..98f22013d33 100644 --- a/src/virtio/vulkan/vn_device.c +++ b/src/virtio/vulkan/vn_device.c @@ -358,10 +358,15 @@ vn_DestroyDevice(VkDevice device, const VkAllocationCallbacks *pAllocator) for (uint32_t i = 0; i < dev->queue_count; i++) vn_queue_fini(&dev->queues[i]); - vk_free(alloc, dev->queues); + /* We must emit vkDestroyDevice before freeing dev->queues. Otherwise, + * another thread might reuse their object ids while they still refer to + * the queues in the renderer. + */ vn_async_vkDestroyDevice(dev->instance, device, NULL); + vk_free(alloc, dev->queues); + vn_device_base_fini(&dev->base); vk_free(alloc, dev); }