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

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

[Gyeyoung Baek]

pvr_vm_unmap_all() repeatedly looks up the first VA and unmaps it. The
VA is removed from the tree at the end of this path:

  pvr_vm_unmap_all()
    pvr_vm_unmap_obj_locked()
      drm_gpuvm_sm_unmap()
        pvr_vm_gpuva_unmap()
          drm_gpuva_unmap()   <- removes the VA from the tree

If this path fails before drm_gpuva_unmap(), the same VA is found
again. If the failure persists, e.g. memory allocation failure under
memory pressure, the loop never terminates and the task hangs.

Always advance to the next VA, whether or not the unmap succeeded. A
VA that fails to unmap is left in place with a warning.

Fixes: a5c4c3ba95a5 ("drm/imagination: Hold drm_gem_gpuva lock for unmap")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-167146@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
Tested with the IGT pvr_* tests.
---
 drivers/gpu/drm/imagination/pvr_vm.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index cbdd15ed74fd..8ea6e88c8bd7 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));
 	}
 
 	mutex_unlock(&vm_ctx->lock);

---
base-commit: c347fef5f8f8927985bd67835331ce4cf7ff43a4
change-id: 20261015-pvr-unmap-all-loop-99fe2f2b7e3f

Best regards,
-- 
Gyeyoung Baek <gye976@gmail.com>

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

[sashiko-bot]

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