From 85e74944c69a41736def2e945b62959ec7d805de Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Sun, 6 Aug 2023 15:58:54 -0700 Subject: [PATCH] winsys/amdgpu: fix a race between import and destroy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit amdgpu_bo_destroy is called when the bo ref count reaches 0. But if the bo is on bo_export_table, amdgpu_bo_from_handle can race with amdgpu_bo_destroy and increments the bo ref count. When that happens, amdgpu_bo_destroy should bail. v2: - reorder amdgpu_bo_free and amdgpu_bo_unmap - fix an assert Reviewed-by: Marek Olšák (v1) Reviewed-by: Pierre-Eric Pelloux-Prayer Part-of: --- src/gallium/auxiliary/pipebuffer/pb_buffer.h | 6 +++- src/gallium/winsys/amdgpu/drm/amdgpu_bo.c | 29 +++++++++++++------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/gallium/auxiliary/pipebuffer/pb_buffer.h b/src/gallium/auxiliary/pipebuffer/pb_buffer.h index 495584604e6..ea153f219a4 100644 --- a/src/gallium/auxiliary/pipebuffer/pb_buffer.h +++ b/src/gallium/auxiliary/pipebuffer/pb_buffer.h @@ -255,7 +255,11 @@ pb_destroy(void *winsys, struct pb_buffer *buf) assert(buf); if (!buf) return; - assert(!pipe_is_referenced(&buf->reference)); + + /* we can't assert(!pipe_is_referenced(&buf->reference)) because the winsys + * might have means to revive a buf whose refcount reaches 0, such as when + * destroy and import race against each other + */ buf->vtbl->destroy(winsys, buf); } diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c index be795334b25..1e7743123c5 100644 --- a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c @@ -154,12 +154,31 @@ void amdgpu_bo_destroy(struct amdgpu_winsys *ws, struct pb_buffer *_buf) assert(bo->bo && "must not be called for slab entries"); + simple_mtx_lock(&ws->bo_export_table_lock); + + /* amdgpu_bo_from_handle might have revived the bo */ + if (p_atomic_read(&bo->base.reference.count)) { + simple_mtx_unlock(&ws->bo_export_table_lock); + return; + } + + _mesa_hash_table_remove_key(ws->bo_export_table, bo->bo); + + if (bo->base.placement & RADEON_DOMAIN_VRAM_GTT) { + amdgpu_bo_va_op(bo->bo, 0, bo->base.size, bo->va, 0, AMDGPU_VA_OP_UNMAP); + amdgpu_va_range_free(bo->u.real.va_handle); + } + + simple_mtx_unlock(&ws->bo_export_table_lock); + if (!bo->u.real.is_user_ptr && bo->u.real.cpu_ptr) { bo->u.real.cpu_ptr = NULL; amdgpu_bo_unmap(&ws->dummy_ws.base, &bo->base); } assert(bo->u.real.is_user_ptr || bo->u.real.map_count == 0); + amdgpu_bo_free(bo->bo); + #if DEBUG if (ws->debug_all_bos) { simple_mtx_lock(&ws->global_bo_list_lock); @@ -187,16 +206,6 @@ void amdgpu_bo_destroy(struct amdgpu_winsys *ws, struct pb_buffer *_buf) } simple_mtx_unlock(&ws->sws_list_lock); - simple_mtx_lock(&ws->bo_export_table_lock); - _mesa_hash_table_remove_key(ws->bo_export_table, bo->bo); - simple_mtx_unlock(&ws->bo_export_table_lock); - - if (bo->base.placement & RADEON_DOMAIN_VRAM_GTT) { - amdgpu_bo_va_op(bo->bo, 0, bo->base.size, bo->va, 0, AMDGPU_VA_OP_UNMAP); - amdgpu_va_range_free(bo->u.real.va_handle); - } - amdgpu_bo_free(bo->bo); - amdgpu_bo_remove_fences(bo); if (bo->base.placement & RADEON_DOMAIN_VRAM)