Re: [PATCH] drm/imagination: Don't retry a failed unmap in pvr_vm_unmap_all()

[sashiko-bot@kernel.org]

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 <gye976@gmail.com>

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/sashiko-outbox-167146@kernel.org/

> 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/20261015-pvr-unmap-all-loop-v1-1-54c676d41269@gmail.com?part=1