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