[PATCH v1] drm/amdgpu: keep freed VM mappings on clear failure

[PATCH v1] drm/amdgpu: keep freed VM mappings on clear failure

[oushinnyo]

amdgpu_vm_clear_freed() removes a mapping from vm->freed before
clearing its page table entries. If amdgpu_vm_update_range() fails,
the mapping is currently freed even though the PTE update might not
have completed.

This is particularly problematic with CPU VM updates. Waiting for an
earlier submission is interruptible, so a signal can abort the wait
before any PTE is cleared. The old mapping then exists neither in the
VA tree nor on the freed list, allowing a later VM update to reuse the
virtual address while an earlier submission still references it.

Put the mapping back on vm->freed on every update error. This keeps the
pending clear operation alive and makes later VM updates retry it before
installing new PTEs. It also covers partial CPU or SDMA updates
conservatively.

Fixes: b4d42511b7f2 ("drm/amdgpu: Support page table update via CPU")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5
Signed-off-by: oushinnyo <oushinnyo@163.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 47cde5459469..e20905d2d90c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1606,11 +1606,12 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
 					   &sync, mapping->start, mapping->last,
 					   0, 0, 0, NULL, NULL, &f);
-		amdgpu_vm_free_mapping(adev, vm, mapping, f);
 		if (r) {
+			list_add(&mapping->list, &vm->freed);
 			dma_fence_put(f);
 			goto error_free;
 		}
+		amdgpu_vm_free_mapping(adev, vm, mapping, f);
 	}
 
 	if (fence && f) {
-- 
2.25.1

Re: [PATCH v1] drm/amdgpu: keep freed VM mappings on clear failure

[Christian König]

On 9/5/26 04:31, oushinnyo wrote:
> amdgpu_vm_clear_freed() removes a mapping from vm->freed before
> clearing its page table entries. If amdgpu_vm_update_range() fails,
> the mapping is currently freed even though the PTE update might not
> have completed.
>
> This is particularly problematic with CPU VM updates. Waiting for an
> earlier submission is interruptible, so a signal can abort the wait
> before any PTE is cleared. The old mapping then exists neither in the
> VA tree nor on the freed list, allowing a later VM update to reuse the
> virtual address while an earlier submission still references it.

That would be indeed problematic, where exactly do you see that? Do you have a test case for this?
> Put the mapping back on vm->freed on every update error. This keeps the
> pending clear operation alive and makes later VM updates retry it before
> installing new PTEs. It also covers partial CPU or SDMA updates
> conservatively.

Clear NAK to that approach.

Freed up areas of the page tables must be cleared when that function is called or otherwise we run into tons of problems reverting previous changes to the housekeeping structures.

The only valid reason this function can fail is because the device is hot removed and in that case it doesn't matter any more what we do here.

Regards,
Christian.

> 
> Fixes: b4d42511b7f2 ("drm/amdgpu: Support page table update via CPU")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5
> Signed-off-by: oushinnyo <oushinnyo@163.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 47cde5459469..e20905d2d90c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1606,11 +1606,12 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>                 r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>                                            &sync, mapping->start, mapping->last,
>                                            0, 0, 0, NULL, NULL, &f);
> -               amdgpu_vm_free_mapping(adev, vm, mapping, f);
>                 if (r) {
> +                       list_add(&mapping->list, &vm->freed);
>                         dma_fence_put(f);
>                         goto error_free;
>                 }
> +               amdgpu_vm_free_mapping(adev, vm, mapping, f);
>         }
> 
>         if (fence && f) {
> --
> 2.25.1
> 

Re: [PATCH v1] drm/amdgpu: keep freed VM mappings on clear failure

[Christian König]

Hi oushinnyo,

On 9/14/26 17:00, oushinnyo wrote:
>   Hi Christian,
> 
>   thanks for the review. I understand that re-adding the mapping to
>   vm->freed is not a valid approach, so I will drop this patch.

Keeping the freed mappings on the list is actually ok when the updated fails.

The problem is that we then need to revert all other changes to housekeeping structures as well to still have a consistent picture and I don't think that this would be easily possible.

> 
>   The failure path I had in mind was the interruptible CPU update wait in
>   amdgpu_vm_cpu_prepare():
> 
>     amdgpu_sync_wait(sync, true)
>       -> dma_fence_wait(fence, true)

Mhm that wait should probably not be interruptible, but it is extremely unlikely that this actually matters. The wait is there just in case.

The only thing which can potentially go wrong is that the device was hot plugged and in that case it doesn't matter any more what we do with the page tables.

Regards,
Christian.

> 
>   I do not have a minimal reproducer yet. I will investigate the actual
>   call path further before proposing another change.
> 
>   Regards,
>   oushinnyo
> 
> At 2026-09-07 20:04:16, "Christian König" <christian.koenig@amd.com> wrote:
>>On 9/5/26 04:31, oushinnyo wrote:
>>> amdgpu_vm_clear_freed() removes a mapping from vm->freed before
>>> clearing its page table entries. If amdgpu_vm_update_range() fails,
>>> the mapping is currently freed even though the PTE update might not
>>> have completed.
>>>
>>> This is particularly problematic with CPU VM updates. Waiting for an
>>> earlier submission is interruptible, so a signal can abort the wait
>>> before any PTE is cleared. The old mapping then exists neither in the
>>> VA tree nor on the freed list, allowing a later VM update to reuse the
>>> virtual address while an earlier submission still references it.
>>
>>That would be indeed problematic, where exactly do you see that? Do you have a test case for this?
>>> Put the mapping back on vm->freed on every update error. This keeps the
>>> pending clear operation alive and makes later VM updates retry it before
>>> installing new PTEs. It also covers partial CPU or SDMA updates
>>> conservatively.
>>
>>Clear NAK to that approach.
>>
>>Freed up areas of the page tables must be cleared when that function is called or otherwise we run into tons of problems reverting previous changes to the housekeeping structures.
>>
>>The only valid reason this function can fail is because the device is hot removed and in that case it doesn't matter any more what we do here.
>>
>>Regards,
>>Christian.
>>
>>> 
>>> Fixes: b4d42511b7f2 ("drm/amdgpu: Support page table update via CPU")
>>> Cc: stable@vger.kernel.org
>>> Assisted-by: Codex:gpt-5
>>> Signed-off-by: oushinnyo <oushinnyo@163.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 47cde5459469..e20905d2d90c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1606,11 +1606,12 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>>                 r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>>>                                            &sync, mapping->start, mapping->last,
>>>                                            0, 0, 0, NULL, NULL, &f);
>>> -               amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>                 if (r) {
>>> +                       list_add(&mapping->list, &vm->freed);
>>>                         dma_fence_put(f);
>>>                         goto error_free;
>>>                 }
>>> +               amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>         }
>>> 
>>>         if (fence && f) {
>>> --
>>> 2.25.1
>>> 
>