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