Re: [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] When splitting an existing mapping, `pvr_vm_gpuva_remap` permanently leaks references to the GEM object by explicitly calling `pvr_gem_object_get` for `prev_va` and `next_va` without any corresponding put in the unmap path.
--

--- Patch [2]: [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode ---
commit 62abfc02f1a7752ea9adac50d4fa08b736d27170
Author: Gyeyoung Baek <[email protected]>

drm/imagination: Switch the GPUVM to immediate mode

This commit switches the PowerVR GPUVM implementation to use
DRM_GPUVM_IMMEDIATE_MODE instead of the deferred execution model. This change
aims to avoid deadlocks during asynchronous VM_BIND operations by removing
the use of drm_gpuvm_exec and directly protecting the gpuva list with the GEM
object's gpuva.lock.

> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 608e8c0467dce..45df76e61f7cf 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c

[ ... ]

> @@ -436,17 +441,21 @@ pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void *op_ctx)
>  
>  	if (op->remap.prev) {
>  		pvr_gem_object_get(gem_to_pvr_gem(ctx->prev_va->base.gem.obj));
> +		mutex_lock(&obj->gpuva.lock);
>  		drm_gpuva_link(&ctx->prev_va->base, vm_bo);
> +		mutex_unlock(&obj->gpuva.lock);
>  		ctx->prev_va = NULL;
>  	}
>  
>  	if (op->remap.next) {
>  		pvr_gem_object_get(gem_to_pvr_gem(ctx->next_va->base.gem.obj));
> +		mutex_lock(&obj->gpuva.lock);
>  		drm_gpuva_link(&ctx->next_va->base, vm_bo);
> +		mutex_unlock(&obj->gpuva.lock);
>  		ctx->next_va = NULL;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does pvr_vm_gpuva_remap()
permanently leak references to the GEM object here?

When splitting an existing mapping (e.g., when an unprivileged userspace
client unmaps a subset or hole of an existing mapping), this code explicitly
calls pvr_gem_object_get() for prev_va and next_va to take an additional
reference to the GEM object for the newly split mappings.

Looking at the unmap path in pvr_vm_gpuva_unmap(), it relies purely on the
vm_bo lifecycle and does not appear to drop these per-VA GEM references:

pvr_vm_gpuva_unmap() {
        ...
        drm_gpuva_unmap(&op->unmap);
        drm_gpuva_unlink_defer(op->unmap.va);
        kfree(to_pvr_vm_gpuva(op->unmap.va));
        ...
}

Because these extra references taken during the remap operation are never
dropped when the mapping is subsequently unmapped, can this lead to a
permanent memory leak of GEM objects and eventual memory exhaustion if
userspace repeatedly splits mappings?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2