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
