On 2026-10-06 09:56, Philip Yang wrote:
prange->svm_bo stays set after the range drops its reference in
svm_range_vram_node_free(), it is only cleared later by
svm_range_bo_release(). Taking a new reference based on old->svm_bo can
therefore hit a svm_bo with refcount 0 and svm_range_bo_release_async()
pending, leaving the clone with a freed svm_bo.
Use svm_bo_ref_unless_zero() under old->lock to only share the svm_bo if
it is still alive, same as svm_range_validate_svm_bo() does.
A range only holds a svm_bo reference while ttm_res is set, so drop the
reference again if old had already dropped its own. The clone keeps the
svm_bo pointer and stays on svm_bo->range_list, so svm_range_bo_release()
clears the pointer when the svm_bo goes away. The unref must be done
outside old->lock because svm_range_bo_release() takes the lock of every
range on the list, including old.
v3: drop the reference if ttm_res is not set, don't leak it.
Good catch. See one nit-pick inline.
v2: use svm_bo_ref_unless_zero instead of checking old->ttm_res, to keep
sharing the svm_bo when the range dropped its own reference (Felix).
Signed-off-by: Philip Yang <[email protected]>
Signed-off-by: Mukul Joshi <[email protected]>
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index cfb428c72686..5dda163cf4ae 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2158,14 +2158,23 @@ static struct svm_range *svm_range_clone(struct
svm_range *old)
svm_range_free(new, false);
return NULL;
}
- if (old->svm_bo) {
+
+ /* old->lock keeps old->svm_bo alive while taking the reference */
+ mutex_lock(&old->lock);
+ if (svm_bo_ref_unless_zero(old->svm_bo)) {
new->ttm_res = old->ttm_res;
new->offset = old->offset;
- new->svm_bo = svm_range_bo_ref(old->svm_bo);
+ new->svm_bo = old->svm_bo;
spin_lock(&new->svm_bo->list_lock);
list_add(&new->svm_bo_list, &new->svm_bo->range_list);
spin_unlock(&new->svm_bo->list_lock);
}
+ mutex_unlock(&old->lock);
+
+ /* prange only holds an svm_bo reference while ttm_res is set */
+ if (new->svm_bo && !new->ttm_res)
+ svm_range_bo_unref(new->svm_bo);
I guess you're doing this after mutex_unlock to prevent recursive
locking in case svm_range_bo_release gets called. Maybe mention that in
the comment. With that, the patch is
Reviewed-by: Felix Kuehling <[email protected]>
+
new->flags = old->flags;
new->preferred_loc = old->preferred_loc;
new->prefetch_loc = old->prefetch_loc;
