On Wed, Sep 23, 2026 at 03:58:27PM +0200, Christian König wrote:
> 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.
>
What about the other 3 patches? If there is less controversy around them, I
can submit them as a new patchset version.
Thanks.
Cascardo.
> >
> >> 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) {
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>
>
