Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/imagination: memory leak of GEM objects on unmap failure --
commit 5b471894c1a99c5f155ac8cf92583bac99156e77 Author: Gyeyoung Baek <[email protected]> drm/imagination: Don't retry a failed unmap in pvr_vm_unmap_all() This commit changes pvr_vm_unmap_all() to advance to the next GPUVA in the tree if pvr_vm_unmap_obj_locked() fails. This prevents an infinite loop and task hang under memory pressure when the unmap operation fails. Link: https://lore.kernel.org/r/[email protected]/ > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c > b/drivers/gpu/drm/imagination/pvr_vm.c > index cbdd15ed74fdb..8ea6e88c8bd7d 100644 > --- a/drivers/gpu/drm/imagination/pvr_vm.c > +++ b/drivers/gpu/drm/imagination/pvr_vm.c > @@ -887,22 +887,25 @@ pvr_vm_unmap(struct pvr_vm_context *vm_ctx, u64 > device_addr, u64 size) > void > pvr_vm_unmap_all(struct pvr_vm_context *vm_ctx) > { > + u64 addr = vm_ctx->gpuvm_mgr.mm_start; > + u64 end = vm_ctx->gpuvm_mgr.mm_start + vm_ctx->gpuvm_mgr.mm_range; > + u64 va_addr, va_range; > + > mutex_lock(&vm_ctx->lock); > > - for (;;) { > + for (; addr < end; addr = va_addr + va_range) { > struct pvr_gem_object *pvr_obj; > struct drm_gpuva *va; > > - va = drm_gpuva_find_first(&vm_ctx->gpuvm_mgr, > - vm_ctx->gpuvm_mgr.mm_start, > - vm_ctx->gpuvm_mgr.mm_range); > + va = drm_gpuva_find_first(&vm_ctx->gpuvm_mgr, addr, end - addr); > if (!va) > break; > > pvr_obj = gem_to_pvr_gem(va->gem.obj); > + va_addr = va->va.addr; > + va_range = va->va.range; > > - WARN_ON(pvr_vm_unmap_obj_locked(vm_ctx, pvr_obj, > - va->va.addr, va->va.range)); > + WARN_ON(pvr_vm_unmap_obj_locked(vm_ctx, pvr_obj, va_addr, > va_range)); [Severity: High] Does this code leak the GEM objects on unmap failure? When pvr_vm_unmap_obj_locked() fails, the loop continues and advances addr past va_addr + va_range, leaving the VA in the GPUVM tree without dropping its reference to the GEM object. Looking at drm_gpuvm_fini(), the core GPUVM cleanup path expects the driver to have emptied the tree and does not automatically free leftover VAs: drivers/gpu/drm/drm_gpuvm.c:drm_gpuvm_fini() { ... drm_WARN(gpuvm->drm, !RB_EMPTY_ROOT(&gpuvm->rb.tree.rb_root), "GPUVA tree is not empty, potentially leaking memory.\n"); ... } Under memory pressure, could an unprivileged user mapping/unmapping memory and destroying contexts use this to leak physical memory by causing unmap allocations to fail? Should the driver forcefully unlink and free the VA in software via drm_gpuva_unlink(), drm_gpuva_remove(), and kfree() during teardown, even if the hardware unmap operations fail? > } > > mutex_unlock(&vm_ctx->lock); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
