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