Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Natalie Vock <nat@pixelcluster.dev>]

On 9/15/26 10:33, Christian König wrote:
> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
>> the clearing of that mapping will not be attempted again. Later on, when
>> an IB tries to read that mapping, the read succeeds, leading to a
>> potential info leak, or even data corruption, if it attempts to write to
>> it.
>>
>> If the mapping is left in the freed list, then clearing will be
>> attempted again during amdgpu_cs_ioctl, which will either fail and not
>> submit the job or will succeed in clearing the mapping, preventing the
>> invalid access.
> 
> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
> 
> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.

Actually, how so? The SDMA update path has lots of allocations that 
happen at random points throughout the update process (think the SDMA IB 
being exhausted, then vm_funcs->update() will allocate a new one and can 
fail on that, or various fence allocations. Some paths allocate from the 
delayed IB pool which is just regular GFP_KERNEL, why couldn't these 
allocations fail?

Aside from that, the more trivial failure path that I think Thadeu is 
also talking about here is that amdgpu_vm_ptes_update will call 
amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and 
these allocations can fail. I'm not even sure if drm_exec can save this 
one because we need to be ready to drop locks and retry.

I get that failing is unacceptable for the cases inside a pagefault 
handler (though I'm not sure if anything of what I mentioned applies 
here, pagefault VM updates are immediate so they get GFP_ATOMIC, but 
maybe the pagefault handler could still allocate PTs?) but for anything 
else, failure seems like a possibility and maybe we should think about 
handling it in some way.

I'm not sure I see a problem in keeping BOs invalidated and backing off 
later, since new submissions should be rejected unless the VM manages to 
complete all updates successfully.

Best,
Natalie

> 
> Regards,
> Christian.
> 
>>
>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 09037df76400..5bde36754607 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>   	while (!list_empty(&vm->freed)) {
>>   		mapping = list_first_entry(&vm->freed,
>>   			struct amdgpu_bo_va_mapping, list);
>> -		list_del(&mapping->list);
>>   
>>   		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) {
>>   			dma_fence_put(f);
>>   			goto error_free;
>>   		}
>> +		list_del(&mapping->list);
>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>   	}
>>   
>>   	if (fence && f) {
>>
>