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.
You also need to increase the amount of reserved fence slots.
> + 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.
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;
>
