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?
> 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) {
> >>>>>>
> >>>>>
> >>>>
> >>
>
