Hi, On 7/1/26 15:13, Natalie Vock wrote: > Hi, > > On 7/1/26 14:26, Christian König wrote: >> On 7/1/26 13:33, Natalie Vock wrote: >>> We claimed vm_bo->moved was protected by the BO being reserved, but then >>> accessed it in a bunch of places without a reservation to the BO anyway. >> >> Can you point out where? That clearly doesn't sound correct to me. > > Right, actually on rechecking it's just one place, see below. > >> >> We have some questionable uses for userptrs, but those should be irrelevant. >> >>> >>> It's not sensible to protect this by BO reservation in any case - it's a >>> property relating to VM state, just like vm_status. Let's protect this >>> by vm->status_lock as well. We usually grab the lock at some point when >>> we access the field anyway, so it fits well with the current usage of >>> the field. >> >> That won't work. You can't hold the status lock while the moved flag can't >> be modified. > > I know holding the status spinlock throughout the entire runtime of > amdgpu_vm_bo_update is not viable, and I'm not trying to do that here. Is > there another period where the moved flag absolutely can't be modified? > >> >>> We also need to remove some unprotected accesses of the field when >>> removing a mapping. Checking for the field there was a microoptimization >>> anyway. >>> >>> Lastly, and most critically, we also need to handle buffer >>> moves/invalidations racing with amdgpu_vm_bo_update. Otherwise we might >>> accidentally undo the invalidation without the PTs actually being >>> properly updated. >> >> That's not correct as far as I can see the moves/invalidations can perfectly >> happen in paralell and are handled before the next CS. > > The race requires some setup: > 1. Assume we're talking about a BO with its own resv, i.e. a > not-VM-always-valid buffer. > 2. Assume the BO is already in the invalidated list. > 3. Assume one thread is trying to evict the buffer and therefore holds the > reservation object. > 4. Assume another thread is trying to submit a CS that does *not* have this > buffer in the BO list, that is, it is not reserved by the CS ioctl. > > Now, imagine the submitting thread is currently in amdgpu_vm_handle_moved, > and ends up encountering that BO while iterating through the invalidated list > here: > https://elixir.bootlin.com/linux/v7.0-rc7/source/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c#L1639. > > dma_resv_trylock() fails, and the caller is not holding the reservation lock. > Still, as intended, we try updating the PTs and enter amdgpu_vm_bo_update > (while *not* holding the resv). > > For one, vm_bo->moved is accessed here: > https://elixir.bootlin.com/linux/v7.0-rc7/source/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c#L1345
No it isn't. The clear flag is true when the BOs reservation lock isn't held. So we never depend on the moved flag here. > and set here: > https://elixir.bootlin.com/linux/v7.0-rc7/source/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c#L1396 Correct, but that is irrelevant. The cleared flag is set which overrides the moved flag on the next update. We could add something like "if (!cleared) bo_va->base.moved = false;", but that doesn't really matter. As soon as the cleared flag is set to true the moved flag becomes irrelevant. > > AFAIU, both of these may race with something else. > > Also, the evicting thread may finish calling amdgpu_bo_move_notify before the > thread running amdgpu_vm_bo_update reaches the amdgpu_vm_bo_done here: > https://elixir.bootlin.com/linux/v7.0-rc7/source/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c#L1391 > > If that happens, then the BO is marked as idle again even though the > concurrent eviction had just invalidated the mappings. I'm like 98% sure I've > run into page faults on supposedly mapped addresses precisely due to this. :) Ah! Yes, absolutely great catch! That is certainly possible and potentially a bug I have been searching for ages. If I'm not completely mistaken we need to do the status update before dropping the lock in amdgpu_vm_handle_moved() (BTW you forgot to rename that one in your rename patch). Thanks a lot for finding that one, Christian. > > Best, > Natalie > >> >> Regards, >> Christian. >> >>> >>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") >>> Signed-off-by: Natalie Vock <[email protected]> >>> --- >>> FWIW, I'm not 100% positive on whether the Fixes tag is correct - I'm >>> fairly certain the VM update <-> invalidation race existed ever since >>> the driver's inception, but it may have been hidden in the initial >>> revision. I suspect backporting it all the way to wherever the race >>> started manifesting is roughly similarly painful, though. >>> >>> Also, this patchset is based on my previous one to rename the "moved" VM >>> state to "needs_update". I think it got reviewed and should've been >>> picked up, but I'm not sure I see it in amd-staging-drm-next? >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 108 +++++++++++++++++-------- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 +- >>> 2 files changed, 75 insertions(+), 36 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> index 32719f31b6c9e..3451dca7de194 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> @@ -214,11 +214,12 @@ static void amdgpu_vm_bo_evicted(struct >>> amdgpu_vm_bo_base *vm_bo) >>> * amdgpu_vm_bo_needs_update - vm_bo needs pagetable update >>> * >>> * @vm_bo: vm_bo which is out of date >>> + * @moved: whether the vm_bo was moved >>> * >>> * State for vm_bo objects meaning the underlying BO had mapping changes >>> (move, PRT bind/unbind) >>> * but the new location is not yet reflected in the page tables. >>> */ >>> -static void amdgpu_vm_bo_needs_update(struct amdgpu_vm_bo_base *vm_bo) >>> +static void amdgpu_vm_bo_needs_update(struct amdgpu_vm_bo_base *vm_bo, >>> bool moved) >>> { >>> struct amdgpu_vm_bo_status *lists; >>> struct amdgpu_bo *bo = vm_bo->bo; >>> @@ -232,11 +233,28 @@ static void amdgpu_vm_bo_needs_update(struct >>> amdgpu_vm_bo_base *vm_bo) >>> vm_bo->moved = false; >>> list_move(&vm_bo->vm_status, &lists->idle); >>> } else { >>> + if (moved) >>> + vm_bo->moved = true; >>> list_move(&vm_bo->vm_status, &lists->needs_update); >>> } >>> amdgpu_vm_bo_unlock_lists(vm_bo); >>> } >>> +/** >>> + * amdgpu_vm_bo_idle_locked - vm_bo is idle, already-locked version >>> + * >>> + * @vm_bo: vm_bo which is now idle >>> + * >>> + * State for vm_bo objects meaning we are done with the state machine and >>> no >>> + * further action is necessary. Needs to supply a locked status list. >>> + */ >>> +static void amdgpu_vm_bo_idle_locked(struct amdgpu_vm_bo_base *vm_bo, >>> + struct amdgpu_vm_bo_status *lists) >>> +{ >>> + if (!amdgpu_vm_is_bo_always_valid(vm_bo->vm, vm_bo->bo)) >>> + vm_bo->moved = false; >>> + list_move(&vm_bo->vm_status, &lists->idle); >>> +} >>> /** >>> * amdgpu_vm_bo_idle - vm_bo is idle >>> * >>> @@ -250,9 +268,7 @@ static void amdgpu_vm_bo_idle(struct amdgpu_vm_bo_base >>> *vm_bo) >>> struct amdgpu_vm_bo_status *lists; >>> lists = amdgpu_vm_bo_lock_lists(vm_bo); >>> - if (!amdgpu_vm_is_bo_always_valid(vm_bo->vm, vm_bo->bo)) >>> - vm_bo->moved = false; >>> - list_move(&vm_bo->vm_status, &lists->idle); >>> + amdgpu_vm_bo_idle_locked(vm_bo, lists); >>> amdgpu_vm_bo_unlock_lists(vm_bo); >>> } >>> @@ -273,9 +289,9 @@ static void amdgpu_vm_bo_reset_state_machine(struct >>> amdgpu_vm *vm) >>> */ >>> amdgpu_vm_assert_locked(vm); >>> list_for_each_entry_safe(vm_bo, tmp, &vm->kernel.idle, vm_status) >>> - amdgpu_vm_bo_needs_update(vm_bo); >>> + amdgpu_vm_bo_needs_update(vm_bo, false); >>> list_for_each_entry_safe(vm_bo, tmp, &vm->always_valid.idle, >>> vm_status) >>> - amdgpu_vm_bo_needs_update(vm_bo); >>> + amdgpu_vm_bo_needs_update(vm_bo, false); >>> spin_lock(&vm->individual_lock); >>> list_for_each_entry_safe(vm_bo, tmp, &vm->individual.idle, vm_status) >>> { >>> @@ -435,7 +451,7 @@ void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base >>> *base, >>> */ >>> if (bo->preferred_domains & >>> amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type)) >>> - amdgpu_vm_bo_needs_update(base); >>> + amdgpu_vm_bo_needs_update(base, false); >>> else >>> amdgpu_vm_bo_evicted(base); >>> } >>> @@ -607,8 +623,7 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, >>> struct amdgpu_vm *vm, >>> return r; >>> vm->update_funcs->map_table(to_amdgpu_bo_vm(bo_base->bo)); >>> - bo_base->moved = true; >>> - amdgpu_vm_bo_needs_update(bo_base); >>> + amdgpu_vm_bo_needs_update(bo_base, true); >>> } >>> /* >>> @@ -625,8 +640,7 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, >>> struct amdgpu_vm *vm, >>> if (r) >>> return r; >>> - bo_base->moved = true; >>> - amdgpu_vm_bo_needs_update(bo_base); >>> + amdgpu_vm_bo_needs_update(bo_base, true); >>> } >>> if (!ticket) >>> @@ -646,8 +660,7 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, >>> struct amdgpu_vm *vm, >>> if (r) >>> return r; >>> - bo_base->moved = true; >>> - amdgpu_vm_bo_needs_update(bo_base); >>> + amdgpu_vm_bo_needs_update(bo_base, true); >>> /* It's a bit inefficient to always jump back to the start, but >>> * we would need to re-structure the KFD for properly fixing >>> @@ -1266,16 +1279,38 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, >>> struct amdgpu_bo_va *bo_va, >>> struct amdgpu_bo *bo = bo_va->base.bo; >>> struct amdgpu_vm *vm = bo_va->base.vm; >>> struct amdgpu_bo_va_mapping *mapping; >>> + struct amdgpu_vm_bo_status *lists; >>> struct dma_fence **last_update; >>> dma_addr_t *pages_addr = NULL; >>> struct ttm_resource *mem; >>> struct amdgpu_sync sync; >>> bool flush_tlb = clear; >>> + bool uncached, moved; >>> uint64_t vram_base; >>> uint64_t flags; >>> - bool uncached; >>> int r; >>> + /* Everything we access in here is protected by the VM PD lock. */ >>> + amdgpu_vm_assert_locked(vm); >>> + >>> + lists = amdgpu_vm_bo_lock_lists(&bo_va->base); >>> + /* >>> + * We can't hold the spinlock for the entire VM update, so temporarily >>> remove >>> + * the BO from the state machine entirely. This does not prevent all >>> types of >>> + * races: We might not hold the BO's resv here, so TTM is free to move >>> the buffer >>> + * and thereby invalidate it. However, it allows us to detect if we >>> raced with >>> + * something that invalidated the BO again and handle that >>> appropriately below. >>> + */ >>> + list_del_init(&bo_va->base.vm_status); >>> + >>> + /* >>> + * The moved flag is also protected by the status lock. It's fine if >>> some buffer >>> + * update changes bo_va->base.moved while we're updating the PTs after >>> unlocking >>> + * the status lock, since that also invalidates the BO's VM status >>> again. >>> + */ >>> + moved = bo_va->base.moved; >>> + amdgpu_vm_bo_unlock_lists(&bo_va->base); >>> + >>> amdgpu_sync_create(&sync); >>> if (clear) { >>> mem = NULL; >>> @@ -1343,7 +1378,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, >>> struct amdgpu_bo_va *bo_va, >>> else >>> last_update = &bo_va->last_pt_update; >>> - if (!clear && bo_va->base.moved) { >>> + if (!clear && moved) { >>> flush_tlb = true; >>> list_splice_init(&bo_va->valids, &bo_va->invalids); >>> @@ -1389,20 +1424,38 @@ int amdgpu_vm_bo_update(struct amdgpu_device >>> *adev, struct amdgpu_bo_va *bo_va, >>> else >>> amdgpu_vm_bo_idle(&bo_va->base); >>> } else { >>> - amdgpu_vm_bo_idle(&bo_va->base); >>> + lists = amdgpu_vm_bo_lock_lists(&bo_va->base); >>> + /* >>> + * Now that we're holding the lock again, check if the >>> + * buffer got invalidated while we weren't looking. >>> + * We initialized vm_status to an empty list head above, >>> + * if that's still the case we can safely mark the BO as >>> + * done. >>> + * >>> + * Note: We only need to do this for BOs that are not >>> + * VM-always-valid, because we hold the VM's reservation >>> + * which by definition reserves all VM-always-valid BOs. >>> + */ >>> + if (list_empty(&bo_va->base.vm_status)) >>> + amdgpu_vm_bo_idle_locked(&bo_va->base, lists); >>> + amdgpu_vm_bo_unlock_lists(&bo_va->base); >>> } >>> list_splice_init(&bo_va->invalids, &bo_va->valids); >>> bo_va->cleared = clear; >>> - bo_va->base.moved = false; >>> if (trace_amdgpu_vm_bo_mapping_enabled()) { >>> list_for_each_entry(mapping, &bo_va->valids, list) >>> trace_amdgpu_vm_bo_mapping(mapping); >>> } >>> + amdgpu_sync_free(&sync); >>> + return 0; >>> + >>> error_free: >>> amdgpu_sync_free(&sync); >>> + if (r) >>> + amdgpu_vm_bo_needs_update(&bo_va->base, false); >>> return r; >>> } >>> @@ -1779,7 +1832,6 @@ static void amdgpu_vm_bo_insert_map(struct >>> amdgpu_device *adev, >>> struct amdgpu_bo_va_mapping *mapping) >>> { >>> struct amdgpu_vm *vm = bo_va->base.vm; >>> - struct amdgpu_bo *bo = bo_va->base.bo; >>> mapping->bo_va = bo_va; >>> list_add(&mapping->list, &bo_va->invalids); >>> @@ -1788,8 +1840,7 @@ static void amdgpu_vm_bo_insert_map(struct >>> amdgpu_device *adev, >>> if (mapping->flags & AMDGPU_VM_PAGE_PRT) >>> amdgpu_vm_prt_get(adev); >>> - if (amdgpu_vm_is_bo_always_valid(vm, bo) && !bo_va->base.moved) >>> - amdgpu_vm_bo_needs_update(&bo_va->base); >>> + amdgpu_vm_bo_needs_update(&bo_va->base, false); >>> trace_amdgpu_vm_bo_map(bo_va, mapping); >>> } >>> @@ -2090,30 +2141,22 @@ int amdgpu_vm_bo_clear_mappings(struct >>> amdgpu_device *adev, >>> /* Insert partial mapping before the range */ >>> if (!list_empty(&before->list)) { >>> - struct amdgpu_bo *bo = before->bo_va->base.bo; >>> - >>> amdgpu_vm_it_insert(before, &vm->va); >>> if (before->flags & AMDGPU_VM_PAGE_PRT) >>> amdgpu_vm_prt_get(adev); >>> - if (amdgpu_vm_is_bo_always_valid(vm, bo) && >>> - !before->bo_va->base.moved) >>> - amdgpu_vm_bo_needs_update(&before->bo_va->base); >>> + amdgpu_vm_bo_needs_update(&before->bo_va->base, false); >>> } else { >>> kfree(before); >>> } >>> /* Insert partial mapping after the range */ >>> if (!list_empty(&after->list)) { >>> - struct amdgpu_bo *bo = after->bo_va->base.bo; >>> - >>> amdgpu_vm_it_insert(after, &vm->va); >>> if (after->flags & AMDGPU_VM_PAGE_PRT) >>> amdgpu_vm_prt_get(adev); >>> - if (amdgpu_vm_is_bo_always_valid(vm, bo) && >>> - !after->bo_va->base.moved) >>> - amdgpu_vm_bo_needs_update(&after->bo_va->base); >>> + amdgpu_vm_bo_needs_update(&after->bo_va->base, false); >>> } else { >>> kfree(after); >>> } >>> @@ -2285,10 +2328,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_bo *bo, >>> bool evicted) >>> continue; >>> } >>> - if (bo_base->moved) >>> - continue; >>> - bo_base->moved = true; >>> - amdgpu_vm_bo_needs_update(bo_base); >>> + amdgpu_vm_bo_needs_update(bo_base, true); >>> } >>> } >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> index c1bd4d35831e0..0acd889568a38 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h >>> @@ -212,8 +212,7 @@ struct amdgpu_vm_bo_base { >>> * protected by vm BO being reserved */ >>> bool shared; >>> - /* if the BO was moved and all mappings are invalid >>> - * protected by the BO being reserved */ >>> + /* protected by the vm's status lock */ >>> bool moved; >>> }; >>> >> >
