On 10/14/26 14:46, Benjamin Cheng wrote:
> On 2026-10-14 03:17, Christian König wrote:
>>
>> On 10/14/26 04:21, 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 on
>>> close. 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.
>> Good catch.
>>
>>> Fix it by tracking the last TLB fence of each VM and attaching it to the
>>> BO on close.
>>>
>>> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
>>> Signed-off-by: Benjamin Cheng <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 ++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>>> 3 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 8dbd7bb77305..7484c52e5850 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -360,6 +360,8 @@ static void amdgpu_gem_object_close(struct
>>> drm_gem_object *obj,
>>>
>>> amdgpu_vm_bo_del(adev, bo_va);
>>> amdgpu_vm_bo_update_shared(bo);
>>> + if (!dma_fence_is_signaled(vm->last_tlb_flush_complete))
>> Drop that extra check, it is actually harmfull.
>
> I will drop it in the next revision, but could you explain why it's harmful?
> (Sorry, still somewhat new to dma_fence semantics).
The dma_resv_add_fence() function can handle signaled fences perfectly fine,
but when you skip that the code path is only exercised in rare cases.
>>
>> You also need to increase the amount of reserved fence slots.
AH, good point! Yeah don't do that.
The fence slots you need must be pre-allocated by drm_exec, amdgpu_bo_fence()
is just an ancient wrapper we should probably remove.
Regards,
Christian.
>
>
> amdgpu_bo_fence already does that.
>
>
>>
>>
>>> + amdgpu_bo_fence(bo, vm->last_tlb_flush_complete, true);
>>> if (!amdgpu_vm_ready(vm))
>>> goto out_unlock;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 7d3659ef11e5..c5411933a76c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1097,6 +1097,9 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params
>>> *params,
>>> if (!params->unlocked && vm->need_tlb_fence) {
>>> amdgpu_vm_tlb_fence_create(params->adev, vm, fence);
>>>
>>> + dma_fence_put(vm->last_tlb_flush_complete);
>>> + vm->last_tlb_flush_complete = dma_fence_get(*fence);
>> I already added that in another patch, looks like amd-staging-drm-next is
>> not up to date.
>
>
> I guess it's last_tlb_inv from "drm/amdgpu: fix TLB invalidation fence
> handling"? I'll rebase against that and use it if there are no other concerns.
>
>
>>
>> Let me double check that.
>>
>> Regards,
>> Christian.
>>
>>> +
>>> /* Makes sure no PD/PT is freed before the flush */
>>> dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
>>> DMA_RESV_USAGE_BOOKKEEP);
>>> @@ -2648,6 +2651,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct
>>> amdgpu_vm *vm,
>>> vm->last_update = dma_fence_get_stub();
>>> vm->last_unlocked = dma_fence_get_stub();
>>> vm->last_tlb_flush = dma_fence_get_stub();
>>> + vm->last_tlb_flush_complete = dma_fence_get_stub();
>>> vm->generation = amdgpu_vm_generation(adev, NULL);
>>>
>>> mutex_init(&vm->eviction_lock);
>>> @@ -2693,6 +2697,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct
>>> amdgpu_vm *vm,
>>> dma_fence_put(vm->last_update);
>>> dma_fence_put(vm->last_tlb_flush);
>>> dma_fence_put(vm->last_unlocked);
>>> + dma_fence_put(vm->last_tlb_flush_complete);
>>> ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
>>> amdgpu_vm_fini_entities(vm);
>>>
>>> @@ -2839,6 +2844,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
>>> struct amdgpu_vm *vm)
>>> dma_fence_lock_irqsave(vm->last_tlb_flush, flags);
>>> dma_fence_unlock_irqrestore(vm->last_tlb_flush, flags);
>>> dma_fence_put(vm->last_tlb_flush);
>>> + dma_fence_put(vm->last_tlb_flush_complete);
>>>
>>> list_for_each_entry_safe(mapping, tmp, &vm->freed, list) {
>>> if (mapping->flags & AMDGPU_VM_PAGE_PRT && prt_fini_needed) {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index ddcdc7256ef0..b9932c5e066f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -331,6 +331,7 @@ struct amdgpu_vm {
>>> /* Last finished delayed update */
>>> atomic64_t tlb_seq;
>>> struct dma_fence *last_tlb_flush;
>>> + struct dma_fence *last_tlb_flush_complete;
>>> atomic64_t kfd_last_flushed_seq;
>>> uint64_t tlb_fence_context;
>>>
