[PATCH] drm/amdgpu: grab a BO reference in vm_lock_done_list.

[PATCH] drm/amdgpu: grab a BO reference in vm_lock_done_list.

[Christian König]

Otherwise it is possible that between dropping the status lock and
locking the BO that the BO is freed up.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index db66b4232de0..c3dfb949a9b8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -484,15 +484,19 @@ int amdgpu_vm_lock_done_list(struct amdgpu_vm *vm, struct drm_exec *exec,
 	spin_lock(&vm->status_lock);
 	while (!list_is_head(prev->next, &vm->done)) {
 		bo_va = list_entry(prev->next, typeof(*bo_va), base.vm_status);
-		spin_unlock(&vm->status_lock);
 
 		bo = bo_va->base.bo;
 		if (bo) {
+			amdgpu_bo_ref(bo);
+			spin_unlock(&vm->status_lock);
+
 			ret = drm_exec_prepare_obj(exec, &bo->tbo.base, 1);
+			amdgpu_bo_unref(&bo);
 			if (unlikely(ret))
 				return ret;
+
+			spin_lock(&vm->status_lock);
 		}
-		spin_lock(&vm->status_lock);
 		prev = prev->next;
 	}
 	spin_unlock(&vm->status_lock);
-- 
2.43.0

Re: [PATCH] drm/amdgpu: grab a BO reference in vm_lock_done_list.

[Khatri, Sunil]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]


On 31-10-2025 01:55 pm, Christian König wrote:
> Otherwise it is possible that between dropping the status lock and
> locking the BO that the BO is freed up.
>
> Signed-off-by: Christian König<christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index db66b4232de0..c3dfb949a9b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -484,15 +484,19 @@ int amdgpu_vm_lock_done_list(struct amdgpu_vm *vm, struct drm_exec *exec,
>   	spin_lock(&vm->status_lock);
>   	while (!list_is_head(prev->next, &vm->done)) {
>   		bo_va = list_entry(prev->next, typeof(*bo_va), base.vm_status);
> -		spin_unlock(&vm->status_lock);
>   
>   		bo = bo_va->base.bo;
>   		if (bo) {
> +			amdgpu_bo_ref(bo);
> +			spin_unlock(&vm->status_lock);
> +
>   			ret = drm_exec_prepare_obj(exec, &bo->tbo.base, 1);

Just for my understanding, there is a possibility that a bo is free 
after spin_unlock and we might be using a stale/NULL ptr of bo.  So we 
are taking a reference before releasing the lock to make

sure the bo is valid. Now calling drm_exec_prepare_obj take a recount to 
make sure the bo is always from now on. Reviewed-by: Sunil Khatri 
<sunil.khatri@amd.com>

> +			amdgpu_bo_unref(&bo);
>   			if (unlikely(ret))
>   				return ret;
> +
> +			spin_lock(&vm->status_lock);
>   		}
> -		spin_lock(&vm->status_lock);
>   		prev = prev->next;
>   	}
>   	spin_unlock(&vm->status_lock);

[-- Attachment #2: Type: text/html, Size: 2380 bytes --]

Re: [PATCH] drm/amdgpu: grab a BO reference in vm_lock_done_list.

[Christian König]

On 10/31/25 09:39, Khatri, Sunil wrote:
> 
> On 31-10-2025 01:55 pm, Christian König wrote:
>> Otherwise it is possible that between dropping the status lock and
>> locking the BO that the BO is freed up.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index db66b4232de0..c3dfb949a9b8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -484,15 +484,19 @@ int amdgpu_vm_lock_done_list(struct amdgpu_vm *vm, struct drm_exec *exec,
>>  	spin_lock(&vm->status_lock);
>>  	while (!list_is_head(prev->next, &vm->done)) {
>>  		bo_va = list_entry(prev->next, typeof(*bo_va), base.vm_status);
>> -		spin_unlock(&vm->status_lock);
>>  
>>  		bo = bo_va->base.bo;
>>  		if (bo) {
>> +			amdgpu_bo_ref(bo);
>> +			spin_unlock(&vm->status_lock);
>> +
>>  			ret = drm_exec_prepare_obj(exec, &bo->tbo.base, 1);
> 
> Just for my understanding, there is a possibility that a bo is free after spin_unlock and we might be using a stale/NULL ptr of bo.  So we are taking a reference before releasing the lock to make
> 
> sure the bo is valid. Now calling drm_exec_prepare_obj take a recount to make sure the bo is always from now on.

Yes, exactly that.

> Reviewed-by: Sunil Khatri <sunil.khatri@amd.com>

Thanks!

Christian.

> 
>> +			amdgpu_bo_unref(&bo);
>>  			if (unlikely(ret))
>>  				return ret;
>> +
>> +			spin_lock(&vm->status_lock);
>>  		}
>> -		spin_lock(&vm->status_lock);
>>  		prev = prev->next;
>>  	}
>>  	spin_unlock(&vm->status_lock);