Skip to contentSkip to conversation

Backported amdgpu ttm deadlock fixes for 4.14

PATCH In review Lyude Paul 3 participants 7 messages Dec 1, 2017 – Dec 4, 2017

Subsystems (7)
lore
Share & tools
lore

Lyude Paul

author
#0 lore raw

cover Backported amdgpu ttm deadlock fixes for 4.14

I haven't gone to see where it started, but as of late a good number of pretty nasty deadlock issues have appeared with the kernel. Easy reproduction recipe on a laptop with i915/amdgpu prime with lockdep enabled:

DRI_PRIME=1 glxinfo

Additionally, some more race conditions exist that I've managed to trigger with piglit and lockdep enabled after applying these patches:

    =============================
    WARNING: suspicious RCU usage
    4.14.3Lyude-Test+ #2 Not tainted
    -----------------------------
    ./include/linux/reservation.h:216 suspicious rcu_dereference_protected() usage!

    other info that might help us debug this:

    rcu_scheduler_active = 2, debug_locks = 1
    1 lock held by ext_image_dma_b/27451:
     #0:  (reservation_ww_class_mutex){+.+.}, at: [<ffffffffa034f2ff>] ttm_bo_unref+0x9f/0x3c0 [ttm]

    stack backtrace:
    CPU: 0 PID: 27451 Comm: ext_image_dma_b Not tainted 4.14.3Lyude-Test+ #2
    Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.02 06/09/2017
    Call Trace:
     dump_stack+0x8e/0xce
     lockdep_rcu_suspicious+0xc5/0x100
     reservation_object_copy_fences+0x292/0x2b0
     ? ttm_bo_unref+0x9f/0x3c0 [ttm]
     ttm_bo_unref+0xbd/0x3c0 [ttm]
     amdgpu_bo_unref+0x2a/0x50 [amdgpu]
     amdgpu_gem_object_free+0x4b/0x50 [amdgpu]
     drm_gem_object_free+0x1f/0x40 [drm]
     drm_gem_object_put_unlocked+0x40/0xb0 [drm]
     drm_gem_object_handle_put_unlocked+0x6c/0xb0 [drm]
     drm_gem_object_release_handle+0x51/0x90 [drm]
     drm_gem_handle_delete+0x5e/0x90 [drm]
     ? drm_gem_handle_create+0x40/0x40 [drm]
     drm_gem_close_ioctl+0x20/0x30 [drm]
     drm_ioctl_kernel+0x5d/0xb0 [drm]
     drm_ioctl+0x2f7/0x3b0 [drm]
     ? drm_gem_handle_create+0x40/0x40 [drm]
     ? trace_hardirqs_on_caller+0xf4/0x190
     ? trace_hardirqs_on+0xd/0x10
     amdgpu_drm_ioctl+0x4f/0x90 [amdgpu]
     do_vfs_ioctl+0x93/0x670
     ? __fget+0x108/0x1f0
     SyS_ioctl+0x79/0x90
     entry_SYSCALL_64_fastpath+0x23/0xc2

I've also added the relevant fixes for the issue mentioned above.

Christian König (3):
drm/ttm: fix ttm_bo_cleanup_refs_or_queue once more
dma-buf: make reservation_object_copy_fences rcu save drm/amdgpu: reserve root PD while releasing it

Michel Dänzer (1):
drm/ttm: Always and only destroy bo->ttm_resv in ttm_bo_release_list

 drivers/dma-buf/reservation.c          | 56 +++++++++++++++++++++++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++--
 drivers/gpu/drm/ttm/ttm_bo.c           | 43 +++++++++++++-------------
 3 files changed, 74 insertions(+), 38 deletions(-)

--
2.14.3

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

author
#1 lore raw

PATCH 1/4 drm/ttm: fix ttm_bo_cleanup_refs_or_queue once more

From: Christian König <[email protected]>

With shared reservation objects __ttm_bo_reserve() can easily fail even on destroyed BOs. This prevents correct handling when we need to individualize the reservation object.

Fix this by individualizing the object before even trying to reserve it.

commit 378e2d5b504fe0231c557751e58b80fcf717cc20 upstream

---
 drivers/gpu/drm/ttm/ttm_bo.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
drivers/gpu/drm/ttm/ttm_bo.c +17 -15
index 180ce6296416..bee77d31895b 100644
@@ -440,28 +440,29 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 	struct ttm_bo_global *glob = bo->glob;
 	int ret;
 
+	ret = ttm_bo_individualize_resv(bo);
+	if (ret) {
+		/* Last resort, if we fail to allocate memory for the
+		 * fences block for the BO to become idle
+		 */
+		reservation_object_wait_timeout_rcu(bo->resv, true, false,
+						    30 * HZ);
+		spin_lock(&glob->lru_lock);
+		goto error;
+	}
+
 	spin_lock(&glob->lru_lock);
 	ret = __ttm_bo_reserve(bo, false, true, NULL);
-
 	if (!ret) {
-		if (!ttm_bo_wait(bo, false, true)) {
+		if (reservation_object_test_signaled_rcu(&bo->ttm_resv, true)) {
 			ttm_bo_del_from_lru(bo);
 			spin_unlock(&glob->lru_lock);
+			if (bo->resv != &bo->ttm_resv)
+				reservation_object_unlock(&bo->ttm_resv);
 			ttm_bo_cleanup_memtype_use(bo);
-
 			return;
 		}
 
-		ret = ttm_bo_individualize_resv(bo);
-		if (ret) {
-			/* Last resort, if we fail to allocate memory for the
-			 * fences block for the BO to become idle and free it.
-			 */
-			spin_unlock(&glob->lru_lock);
-			ttm_bo_wait(bo, true, true);
-			ttm_bo_cleanup_memtype_use(bo);
-			return;
-		}
 		ttm_bo_flush_all_fences(bo);
 
 		/*
@@ -474,11 +475,12 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 			ttm_bo_add_to_lru(bo);
 		}
 
-		if (bo->resv != &bo->ttm_resv)
-			reservation_object_unlock(&bo->ttm_resv);
 		__ttm_bo_unreserve(bo);
 	}
+	if (bo->resv != &bo->ttm_resv)
+		reservation_object_unlock(&bo->ttm_resv);
 
+error:
 	kref_get(&bo->list_kref);
 	list_add_tail(&bo->ddestroy, &bdev->ddestroy);
 	spin_unlock(&glob->lru_lock);
Signature · 6 lines
2.14.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#2 lore raw

PATCH 2/4 dma-buf: make reservation_object_copy_fences rcu save

In reply to Lyude Paul

From: Christian König <[email protected]>

Stop requiring that the src reservation object is locked for this operation.

commit 39e16ba16c147e662bf9fbcee9a99d70d420382f upstream

---
 drivers/dma-buf/reservation.c | 56 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 14 deletions(-)
drivers/dma-buf/reservation.c +42 -14
index dec3a815455d..b44d9d7db347 100644
@@ -266,8 +266,7 @@ EXPORT_SYMBOL(reservation_object_add_excl_fence);
 * @dst: the destination reservation object
 * @src: the source reservation object
 *
-* Copy all fences from src to dst. Both src->lock as well as dst-lock must be
-* held.
+* Copy all fences from src to dst. dst-lock must be held.
 */
 int reservation_object_copy_fences(struct reservation_object *dst,
 				   struct reservation_object *src)
@@ -277,33 +276,62 @@ int reservation_object_copy_fences(struct reservation_object *dst,
 	size_t size;
 	unsigned i;
 
-	src_list = reservation_object_get_list(src);
+	rcu_read_lock();
+	src_list = rcu_dereference(src->fence);
 
+retry:
 	if (src_list) {
-		size = offsetof(typeof(*src_list),
-				shared[src_list->shared_count]);
+		unsigned shared_count = src_list->shared_count;
+
+		size = offsetof(typeof(*src_list), shared[shared_count]);
+		rcu_read_unlock();
+
 		dst_list = kmalloc(size, GFP_KERNEL);
 		if (!dst_list)
 			return -ENOMEM;
 
-		dst_list->shared_count = src_list->shared_count;
-		dst_list->shared_max = src_list->shared_count;
-		for (i = 0; i < src_list->shared_count; ++i)
-			dst_list->shared[i] =
-				dma_fence_get(src_list->shared[i]);
+		rcu_read_lock();
+		src_list = rcu_dereference(src->fence);
+		if (!src_list || src_list->shared_count > shared_count) {
+			kfree(dst_list);
+			goto retry;
+		}
+
+		dst_list->shared_count = 0;
+		dst_list->shared_max = shared_count;
+		for (i = 0; i < src_list->shared_count; ++i) {
+			struct dma_fence *fence;
+
+			fence = rcu_dereference(src_list->shared[i]);
+			if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+				     &fence->flags))
+				continue;
+
+			if (!dma_fence_get_rcu(fence)) {
+				kfree(dst_list);
+				src_list = rcu_dereference(src->fence);
+				goto retry;
+			}
+
+			if (dma_fence_is_signaled(fence)) {
+				dma_fence_put(fence);
+				continue;
+			}
+
+			dst_list->shared[dst_list->shared_count++] = fence;
+		}
 	} else {
 		dst_list = NULL;
 	}
 
+	new = dma_fence_get_rcu_safe(&src->fence_excl);
+	rcu_read_unlock();
+
 	kfree(dst->staged);
 	dst->staged = NULL;
 
 	src_list = reservation_object_get_list(dst);
-
 	old = reservation_object_get_excl(dst);
-	new = reservation_object_get_excl(src);
-
-	dma_fence_get(new);
 
 	preempt_disable();
 	write_seqcount_begin(&dst->seq);
Signature · 6 lines
2.14.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#3 lore raw

PATCH 3/4 drm/amdgpu: reserve root PD while releasing it

In reply to Lyude Paul

From: Christian König <[email protected]>

Otherwise somebody could try to evict it at the same time and try to use half torn down structures.

commit 2642cf110d08a403f585a051e4cbf45a90b3adea upstream

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +11 -2
index bd20ff018512..591c6dc2c1f7 100644
@@ -2586,7 +2586,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 {
 	struct amdgpu_bo_va_mapping *mapping, *tmp;
 	bool prt_fini_needed = !!adev->gart.gart_funcs->set_prt;
-	int i;
+	struct amdgpu_bo *root;
+	int i, r;
 
 	amd_sched_entity_fini(vm->entity.sched, &vm->entity);
 
@@ -2609,7 +2610,15 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		amdgpu_vm_free_mapping(adev, vm, mapping, NULL);
 	}
 
-	amdgpu_vm_free_levels(&vm->root);
+	root = amdgpu_bo_ref(vm->root.bo);
+	r = amdgpu_bo_reserve(root, true);
+	if (r) {
+		dev_err(adev->dev, "Leaking page tables because BO reservation failed\n");
+	} else {
+		amdgpu_vm_free_levels(&vm->root);
+		amdgpu_bo_unreserve(root);
+	}
+	amdgpu_bo_unref(&root);
 	dma_fence_put(vm->last_dir_update);
 	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
 		amdgpu_vm_free_reserved_vmid(adev, vm, i);
Signature · 6 lines
2.14.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#4 lore raw

PATCH 4/4 drm/ttm: Always and only destroy bo->ttm_resv in ttm_bo_release_list

In reply to Lyude Paul

From: Michel Dänzer <[email protected]>

Fixes a use-after-free due to a race condition in ttm_bo_cleanup_refs_and_unlock, which allows one task to reserve a BO and destroy its ttm_resv while another task is waiting for it to signal in reservation_object_wait_timeout_rcu.

commit e1fc12c5d9ad06a2a74e97a91f1b0c5f4c723b50 upstream

v2:
* Always initialize bo->ttm_resv in ttm_bo_init_reserved (Christian König)

Fixes 0d2bd2ae045d "drm/ttm: fix memory leak while individualizing BOs"
---
 drivers/gpu/drm/ttm/ttm_bo.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/gpu/drm/ttm/ttm_bo.c +4 -7
index bee77d31895b..c088703777e2 100644
@@ -150,8 +150,7 @@ static void ttm_bo_release_list(struct kref *list_kref)
 	ttm_tt_destroy(bo->ttm);
 	atomic_dec(&bo->glob->bo_count);
 	dma_fence_put(bo->moving);
-	if (bo->resv == &bo->ttm_resv)
-		reservation_object_fini(&bo->ttm_resv);
+	reservation_object_fini(&bo->ttm_resv);
 	mutex_destroy(&bo->wu_mutex);
 	if (bo->destroy)
 		bo->destroy(bo);
@@ -402,14 +401,11 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
 	if (bo->resv == &bo->ttm_resv)
 		return 0;
 
-	reservation_object_init(&bo->ttm_resv);
 	BUG_ON(!reservation_object_trylock(&bo->ttm_resv));
 
 	r = reservation_object_copy_fences(&bo->ttm_resv, bo->resv);
-	if (r) {
+	if (r)
 		reservation_object_unlock(&bo->ttm_resv);
-		reservation_object_fini(&bo->ttm_resv);
-	}
 
 	return r;
 }
@@ -459,6 +455,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 			spin_unlock(&glob->lru_lock);
 			if (bo->resv != &bo->ttm_resv)
 				reservation_object_unlock(&bo->ttm_resv);
+
 			ttm_bo_cleanup_memtype_use(bo);
 			return;
 		}
@@ -1205,8 +1202,8 @@ int ttm_bo_init_reserved(struct ttm_bo_device *bdev,
 		lockdep_assert_held(&bo->resv->lock.base);
 	} else {
 		bo->resv = &bo->ttm_resv;
-		reservation_object_init(&bo->ttm_resv);
 	}
+	reservation_object_init(&bo->ttm_resv);
 	atomic_inc(&bo->glob->bo_count);
 	drm_vma_node_reset(&bo->vma_node);
 	bo->priority = 0;
Signature · 6 lines
2.14.3

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Acked-by on the v1 series
#5 lore raw
In reply to Lyude Paul

Am 01.12.2017 um 01:23 schrieb Lyude Paul:

Quoted 5 lines

I haven't gone to see where it started, but as of late a good number of pretty nasty deadlock issues have appeared with the kernel. Easy reproduction recipe on a laptop with i915/amdgpu prime with lockdep enabled:

DRI_PRIME=1 glxinfo

Thanks for taking care of this,
Christian.

Quoted 67 lines

Additionally, some more race conditions exist that I've managed to trigger with piglit and lockdep enabled after applying these patches:

     =============================
     WARNING: suspicious RCU usage
     4.14.3Lyude-Test+ #2 Not tainted
     -----------------------------
     ./include/linux/reservation.h:216 suspicious rcu_dereference_protected() usage!

     other info that might help us debug this:

     rcu_scheduler_active = 2, debug_locks = 1
     1 lock held by ext_image_dma_b/27451:
      #0:  (reservation_ww_class_mutex){+.+.}, at: [<ffffffffa034f2ff>] ttm_bo_unref+0x9f/0x3c0 [ttm]

     stack backtrace:
     CPU: 0 PID: 27451 Comm: ext_image_dma_b Not tainted 4.14.3Lyude-Test+ #2
     Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.02 06/09/2017
     Call Trace:
      dump_stack+0x8e/0xce
      lockdep_rcu_suspicious+0xc5/0x100
      reservation_object_copy_fences+0x292/0x2b0
      ? ttm_bo_unref+0x9f/0x3c0 [ttm]
      ttm_bo_unref+0xbd/0x3c0 [ttm]
      amdgpu_bo_unref+0x2a/0x50 [amdgpu]
      amdgpu_gem_object_free+0x4b/0x50 [amdgpu]
      drm_gem_object_free+0x1f/0x40 [drm]
      drm_gem_object_put_unlocked+0x40/0xb0 [drm]
      drm_gem_object_handle_put_unlocked+0x6c/0xb0 [drm]
      drm_gem_object_release_handle+0x51/0x90 [drm]
      drm_gem_handle_delete+0x5e/0x90 [drm]
      ? drm_gem_handle_create+0x40/0x40 [drm]
      drm_gem_close_ioctl+0x20/0x30 [drm]
      drm_ioctl_kernel+0x5d/0xb0 [drm]
      drm_ioctl+0x2f7/0x3b0 [drm]
      ? drm_gem_handle_create+0x40/0x40 [drm]
      ? trace_hardirqs_on_caller+0xf4/0x190
      ? trace_hardirqs_on+0xd/0x10
      amdgpu_drm_ioctl+0x4f/0x90 [amdgpu]
      do_vfs_ioctl+0x93/0x670
      ? __fget+0x108/0x1f0
      SyS_ioctl+0x79/0x90
      entry_SYSCALL_64_fastpath+0x23/0xc2

I've also added the relevant fixes for the issue mentioned above.

Christian König (3):
drm/ttm: fix ttm_bo_cleanup_refs_or_queue once more
dma-buf: make reservation_object_copy_fences rcu save drm/amdgpu: reserve root PD while releasing it

Michel Dänzer (1):
drm/ttm: Always and only destroy bo->ttm_resv in ttm_bo_release_list

  drivers/dma-buf/reservation.c          | 56 +++++++++++++++++++++++++---------
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++--
  drivers/gpu/drm/ttm/ttm_bo.c           | 43 +++++++++++++-------------
  3 files changed, 74 insertions(+), 38 deletions(-)

--
2.14.3

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

#6 lore raw
In reply to Lyude Paul
Quoted 52 lines · Lyude Paul

I haven't gone to see where it started, but as of late a good number of pretty nasty deadlock issues have appeared with the kernel. Easy reproduction recipe on a laptop with i915/amdgpu prime with lockdep enabled:

DRI_PRIME=1 glxinfo

Additionally, some more race conditions exist that I've managed to trigger with piglit and lockdep enabled after applying these patches:

    =============================
    WARNING: suspicious RCU usage
    4.14.3Lyude-Test+ #2 Not tainted
    -----------------------------
    ./include/linux/reservation.h:216 suspicious rcu_dereference_protected() usage!

    other info that might help us debug this:

    rcu_scheduler_active = 2, debug_locks = 1
    1 lock held by ext_image_dma_b/27451:
     #0:  (reservation_ww_class_mutex){+.+.}, at: [<ffffffffa034f2ff>] ttm_bo_unref+0x9f/0x3c0 [ttm]

    stack backtrace:
    CPU: 0 PID: 27451 Comm: ext_image_dma_b Not tainted 4.14.3Lyude-Test+ #2
    Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.02 06/09/2017
    Call Trace:
     dump_stack+0x8e/0xce
     lockdep_rcu_suspicious+0xc5/0x100
     reservation_object_copy_fences+0x292/0x2b0
     ? ttm_bo_unref+0x9f/0x3c0 [ttm]
     ttm_bo_unref+0xbd/0x3c0 [ttm]
     amdgpu_bo_unref+0x2a/0x50 [amdgpu]
     amdgpu_gem_object_free+0x4b/0x50 [amdgpu]
     drm_gem_object_free+0x1f/0x40 [drm]
     drm_gem_object_put_unlocked+0x40/0xb0 [drm]
     drm_gem_object_handle_put_unlocked+0x6c/0xb0 [drm]
     drm_gem_object_release_handle+0x51/0x90 [drm]
     drm_gem_handle_delete+0x5e/0x90 [drm]
     ? drm_gem_handle_create+0x40/0x40 [drm]
     drm_gem_close_ioctl+0x20/0x30 [drm]
     drm_ioctl_kernel+0x5d/0xb0 [drm]
     drm_ioctl+0x2f7/0x3b0 [drm]
     ? drm_gem_handle_create+0x40/0x40 [drm]
     ? trace_hardirqs_on_caller+0xf4/0x190
     ? trace_hardirqs_on+0xd/0x10
     amdgpu_drm_ioctl+0x4f/0x90 [amdgpu]
     do_vfs_ioctl+0x93/0x670
     ? __fget+0x108/0x1f0
     SyS_ioctl+0x79/0x90
     entry_SYSCALL_64_fastpath+0x23/0xc2

I've also added the relevant fixes for the issue mentioned above.

Quoted 7 lines

Christian König (3):
drm/ttm: fix ttm_bo_cleanup_refs_or_queue once more
dma-buf: make reservation_object_copy_fences rcu save drm/amdgpu: reserve root PD while releasing it

Michel Dänzer (1):
drm/ttm: Always and only destroy bo->ttm_resv in ttm_bo_release_list

All now queued up, thanks.

greg k-h