On 9/15/26 12:37, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
>> 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 <[email protected]>, 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
>>
>
> Thanks, Natalie, those are exactly the possible scenarios here:
> amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.
>
> Christian,
>
> I have started this investigation once I observed the kmemleak that is
> fixed by the other 3 patches. Natalie brings up the exact case where this
> might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
> they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.
Yeah and on a clear there is never a single page table allocated. What could be
is that temporary memory allocations fail, but we need to make sure that never
happens by using the emergency reserves and/or pools.
> Here is a log showing up that something failed during the update:
> [ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update
> BO_VA (-12)
That is failed memory allocation while trying to map something which we also
ignore and delegate to the next CS.
>
> The investigation then led to a reproducer, which is able to trigger the
> list corruption that is mentioned in patch 4.
>
> And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
> trace, that should clearly be where the failure has happened. Looking at
> the function, then I noticed it ignores the return code and still removes
> the mappings from the list. I was able to write a second reproducer that
> triggers the failure, then releases some memory and demonstrate that
> amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
> empty.
Unmap/clear operations are also used in MMU notifiers who can't give an error
back to higher levels but rather need to unmap the area no matter what.
So basically that is an operation which can never fail in the first place
(except for device is completely gone and then we don't care any more).
I'm about to split the map/unmap into separate functions to make it clear that
unmap operations can't fail.
When you have a reproducer and can pinpoint where exactly the memory allocation
fails that would be rather helpful here.
Regards,
Christian.
>
> Regards.
> Cascardo.
>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
>>>> ---
>>>> 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) {
>>>>
>>>
>>
