Re: [PATCH v2] drm/amdgpu: wait for tlb flush before freeing BO

["Christian König" <christian.koenig@amd.com>]

On 11/6/26 17:07, Benjamin Cheng wrote:
> 
> On 2026-11-06 11:04, Christian König wrote:
>>
>> On 11/6/26 16:20, Benjamin Cheng wrote:
>>> On gem object close, we previously only attached the fence of its own
>>> clear_freed() call to the BO's resv. If an unmap of this BO happened
>>> prior to the close call, clear_freed() would be invoked, leaving
>>> vm->freed empty, and no meaningful fence to be attached to the resv.
>>> Because of that the memory may be freed before the TLB flush is
>>> complete, and give a running userq potential access to a stale
>>> address translation.
>>>
>>> Fix this by attaching the last_tlb_inv fence to the BO's resv.
>>>
>>> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
>>> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
>>> ---
>>> v2:
>>> - Rebase on "drm/amdgpu: fix TLB invalidation fence handling v4" to get
>>>   last_tlb_inv
>>> - Move the fence after the clear_freed call to make sure we wait for the
>>>   right invalidation.
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 6137ab4211be..36b0bd85616f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -371,11 +371,14 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>>  		dev_err(adev->dev, "failed to clear page "
>>>  			"tables on GEM object close (%ld)\n", r);
>>>  	if (r || !fence)
>>> -		goto out_unlock;
>>> +		goto out_tlb;
>>>  
>>>  	amdgpu_bo_fence(bo, fence, true);
>>>  	dma_fence_put(fence);
>>>  
>>> +out_tlb:
>>> +	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
>> You need to change the drm_exec_prepare_obj() call above from 1 to 2 since we are now attaching 2 fences.
> 
> 
> As I said in v1 of this patch, amdgpu_bo_fence calls resv itself.

Mhm, looks like my response was lost.

Absolutely don't use amdgpu_bo_fence(), reserving the fence slots there is a broken approach which is why the function is deprecated.

That needs to be done once after locking.

Regards,
Christian.

> 
> 
>> Apart from that looks good to me.
>>
>> Regards,
>> Christian.
>>
>>> +
>>>  out_unlock:
>>>  	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))
>>>  		dev_err(adev->dev, "leaking bo va (%ld)\n", r);