Re: [PATCH 1/2] drm/amdgpu: make sure BOs are locked in amdgpu_vm_get_memory

[Christian König]

Am 06.06.23 um 03:11 schrieb Chen, Guchun:

[Public]

Acked-by: Guchun Chen <guchun.c...@amd.com> for this series.

A simple question is we don't need to hold the lock if bo locations are not 
changed?

Well we take the look to make sure that BO locations don't change.

Otherwise we potentially access freed memory when looking at the resource.

Regards,
Christian.

Regards,
Guchun

-----Original Message-----
From: Christian König <ckoenig.leichtzumer...@gmail.com>
Sent: Monday, June 5, 2023 5:11 PM
To: amd-gfx@lists.freedesktop.org; mikhail.v.gavri...@gmail.com; Chen,
Guchun <guchun.c...@amd.com>
Subject: [PATCH 1/2] drm/amdgpu: make sure BOs are locked in
amdgpu_vm_get_memory

We need to grab the lock of the BO or otherwise can run into a crash when
we try to inspect the current location.

Signed-off-by: Christian König <christian.koe...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 69 +++++++++++++++---------
--
  1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3c0310576b3b..2c8cafec48a4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -920,42 +920,51 @@ int amdgpu_vm_update_range(struct
amdgpu_device *adev, struct amdgpu_vm *vm,
       return r;
  }

+static void amdgpu_vm_bo_get_memory(struct amdgpu_bo_va *bo_va,
+                                 struct amdgpu_mem_stats *stats) {
+     struct amdgpu_vm *vm = bo_va->base.vm;
+     struct amdgpu_bo *bo = bo_va->base.bo;
+
+     if (!bo)
+             return;
+
+     /*
+      * For now ignore BOs which are currently locked and potentially
+      * changing their location.
+      */
+     if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv &&
+         !dma_resv_trylock(bo->tbo.base.resv))
+             return;
+
+     amdgpu_bo_get_memory(bo, stats);
+     if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv)
+         dma_resv_unlock(bo->tbo.base.resv);
+}
+
  void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
                         struct amdgpu_mem_stats *stats)
  {
       struct amdgpu_bo_va *bo_va, *tmp;

       spin_lock(&vm->status_lock);
-     list_for_each_entry_safe(bo_va, tmp, &vm->idle, base.vm_status) {
-             if (!bo_va->base.bo)
-                     continue;
-             amdgpu_bo_get_memory(bo_va->base.bo, stats);
-     }
-     list_for_each_entry_safe(bo_va, tmp, &vm->evicted, base.vm_status)
{
-             if (!bo_va->base.bo)
-                     continue;
-             amdgpu_bo_get_memory(bo_va->base.bo, stats);
-     }
-     list_for_each_entry_safe(bo_va, tmp, &vm->relocated,
base.vm_status) {
-             if (!bo_va->base.bo)
-                     continue;
-             amdgpu_bo_get_memory(bo_va->base.bo, stats);
-     }
-     list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status)
{
-             if (!bo_va->base.bo)
-                     continue;
-             amdgpu_bo_get_memory(bo_va->base.bo, stats);
-     }
-     list_for_each_entry_safe(bo_va, tmp, &vm->invalidated,
base.vm_status) {
-             if (!bo_va->base.bo)
-                     continue;
-             amdgpu_bo_get_memory(bo_va->base.bo, stats);
-     }
-     list_for_each_entry_safe(bo_va, tmp, &vm->done, base.vm_status) {
-             if (!bo_va->base.bo)
-                     continue;
-             amdgpu_bo_get_memory(bo_va->base.bo, stats);
-     }
+     list_for_each_entry_safe(bo_va, tmp, &vm->idle, base.vm_status)
+             amdgpu_vm_bo_get_memory(bo_va, stats);
+
+     list_for_each_entry_safe(bo_va, tmp, &vm->evicted, base.vm_status)
+             amdgpu_vm_bo_get_memory(bo_va, stats);
+
+     list_for_each_entry_safe(bo_va, tmp, &vm->relocated,
base.vm_status)
+             amdgpu_vm_bo_get_memory(bo_va, stats);
+
+     list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status)
+             amdgpu_vm_bo_get_memory(bo_va, stats);
+
+     list_for_each_entry_safe(bo_va, tmp, &vm->invalidated,
base.vm_status)
+             amdgpu_vm_bo_get_memory(bo_va, stats);
+
+     list_for_each_entry_safe(bo_va, tmp, &vm->done, base.vm_status)
+             amdgpu_vm_bo_get_memory(bo_va, stats);
       spin_unlock(&vm->status_lock);
  }

--
2.34.1