On 9/23/26 15:56, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Sep 15, 2026 at 03:56:00PM +0200, Christian König wrote:
>> On 9/15/26 13:34, Thadeu Lima de Souza Cascardo wrote:
>>> On Tue, Sep 15, 2026 at 01:22:48PM +0200, Christian König wrote:
>>>> 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.
>>>>
>>>
>>> Yep, that is a possibility as well and amdgpu_vm_bo_update is not ignoring
>>> the failure here and leaves the mappings in the invalids list, which the
>>> next CS will try to map.
>>>
>>>>>
>>>>> 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.
>>>>
>>>
>>> What I understand that happens in the case of clear_freed is that we are
>>> breaking a PDB/hugepage and descending, as the clear happened in the middle
>>> of the hugepage. With the specific reproducer I had to go through lengths
>>> of making sure that the original mapping landed on a hugepage-aligned
>>> address, then used CLEAR/amdgpu_vm_clear_mappings to unmap a single page in
>>> the middle of it.
>>
>> Yeah that is exactly the broken case I'm already working on.
>>
>> In that case the clear should just unmap the whole hugepage and not allocate
>> new PDs.
>>
>
> Agreed that we should also do this. Because my patchset does not prevent
> the issue and if there is no relief on memory pressure, the failure will
> end up in amdgpu_cs_ioctl, resulting in a submission failure.
>
> In any case, given that other causes could still result in the similar
> symptoms (memory leak, list corruption), would you consider applying this
> patchset?
No, the code changed by this patch here at least is actually fully correct.
Regards,
Christian.
>
>> Re-mapping the two fragments before and after the unmapped area is supposed
>> to allocate the new PDs.
>>
>> Without that we run into tons of problems with userptrs and MMU notifiers.
>>
>> I will CC you on the patches I have so far.
>>
>
> Please, do. I will be happy to test it and review it.
>
> Thanks.
> Cascardo.
>
>> Regards,
>> Christian.
>>
>>>
>>> The other reproducer that I have required that I used REPLACE instead of
>>> MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
>>> amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
>>> instead of MAP, so that explains that we can see this in production.
>>>
>>> Regards.
>>> Cascardo.
>>>
>>>> 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) {
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
