Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Calling `drm_gpuvm_bo_put()` without holding the GEM object's dma_resv lock in `pvr_vm_bind_op_fini()` causes lockdep splats and GEM gpuva list corruption on error paths. - [High] Infinite loop in `pvr_vm_unmap_all()` when `pvr_vm_unmap_obj_locked()` fails, leading to a kernel thread hang (Denial of Service). -- --- Patch [1]: [PATCH v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM --- commit 1da21cbc09436cbb38e021164bab86864e2bacca Author: Icenowy Zheng <[email protected]> drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM The drm gpuvm code doesn't protect find operation against map operation, and the driver needs to ensure a map operation shouldn't happen when a find operation is in progress. > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c > b/drivers/gpu/drm/imagination/pvr_vm.c > index 396d349fb6ce4..ceb78694cd987 100644 > --- a/drivers/gpu/drm/imagination/pvr_vm.c > +++ b/drivers/gpu/drm/imagination/pvr_vm.c > @@ -747,6 +747,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct > pvr_gem_object *pvr_obj, > > pvr_gem_object_get(pvr_obj); > > + mutex_lock(&vm_ctx->lock); > err = drm_gpuvm_exec_lock(&vm_exec); > if (err) > goto err_cleanup; > @@ -756,6 +757,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct > pvr_gem_object *pvr_obj, > drm_gpuvm_exec_unlock(&vm_exec); > > err_cleanup: > + mutex_unlock(&vm_ctx->lock); > pvr_vm_bind_op_fini(&bind_op); [Severity: High] This is a pre-existing issue, but does calling pvr_vm_bind_op_fini() in the error path here drop the drm_gpuvm_bo reference without holding the GEM's dma_resv lock? Looking at pvr_vm_bind_op_fini(), it calls drm_gpuvm_bo_put() unconditionally: drivers/gpu/drm/imagination/pvr_vm.c:pvr_vm_bind_op_fini() { drm_gpuvm_bo_put(bind_op->gpuvm_bo); ... } When drm_gpuvm_bo_put() drops the reference count to zero, it destroys the BO and removes it from the GEM's gpuva.list. This API requires the caller to hold the dma_resv lock. In error paths like when drm_gpuvm_exec_lock() fails, this cleanup path runs without the dma_resv lock held. Could this lead to list corruption and use-after-free conditions on the GEM's gpuva lists if multiple threads operate concurrently? > > return err; > } [ ... ] [Severity: High] This is a pre-existing issue, but can pvr_vm_unmap_all() enter an infinite loop if pvr_vm_unmap_obj_locked() fails? Looking at pvr_vm_unmap_all(): drivers/gpu/drm/imagination/pvr_vm.c:pvr_vm_unmap_all() { for (;;) { ... va = drm_gpuva_find_first(&vm_ctx->gpuvm_mgr, ...); if (!va) break; pvr_obj = gem_to_pvr_gem(va->gem.obj); WARN_ON(pvr_vm_unmap_obj_locked(vm_ctx, pvr_obj, va->va.addr, va->va.range)); } } The return value of pvr_vm_unmap_obj_locked() is tested inside a WARN_ON but otherwise ignored. If it fails, the mapping is not removed from the tree, and the next iteration will find the exact same mapping via drm_gpuva_find_first(). Since pvr_vm_unmap_obj_locked() uses DRM_EXEC_INTERRUPTIBLE_WAIT, if a process is being killed (e.g. SIGKILL during teardown), this lock will repeatedly fail with -ERESTARTSYS. Does this cause an unrecoverable infinite loop in the kernel under vm_ctx->lock? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
