From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM Date: Thu, 23 Apr 2026 08:12:42 +1000 Message-ID: In-Reply-To: <20260421175748.1989002-1-zhengxingda@iscas.ac.cn> References: <20260421175748.1989002-1-zhengxingda@iscas.ac.cn> <20260421175748.1989002-1-zhengxingda@iscas.ac.cn> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Bug: Double `mutex_unlock` on the success path** After applying the patch, the success path through `pvr_vm_map()` executes: ```c mutex_lock(&vm_ctx->lock); // line 750: acquire err =3D drm_gpuvm_exec_lock(&vm_exec); // ... succeeds, no goto ... err =3D pvr_vm_bind_op_exec(&bind_op); drm_gpuvm_exec_unlock(&vm_exec); mutex_unlock(&vm_ctx->lock); // line 758: first unlock err_cleanup: pvr_vm_bind_op_fini(&bind_op); mutex_unlock(&vm_ctx->lock); // line 762: SECOND unlock =E2=80=94 B= UG ``` On success, execution falls through past the first `mutex_unlock` at line 7= 58 into `err_cleanup:`, where the mutex is unlocked a **second time**. This= is undefined behavior for a non-recursive mutex =E2=80=94 it corrupts the = mutex state, can cause subsequent lock attempts to succeed without mutual e= xclusion, or trigger a `DEBUG_LOCKS_WARN_ON` splat. This effectively trades= a NULL dereference race for a lock corruption bug. The error path (where `drm_gpuvm_exec_lock` fails and jumps to `err_cleanup= `) is correct =E2=80=94 the mutex is only unlocked once. **Fix:** Remove the `mutex_unlock` after `drm_gpuvm_exec_unlock()` and keep= only the single unlock at `err_cleanup`. This mirrors the structure used i= n `pvr_vm_unmap_obj_locked()` (lines 800=E2=80=93819) where the cleanup pat= h is the only exit: ```c mutex_lock(&vm_ctx->lock); err =3D drm_gpuvm_exec_lock(&vm_exec); if (err) goto err_cleanup; err =3D pvr_vm_bind_op_exec(&bind_op); drm_gpuvm_exec_unlock(&vm_exec); err_cleanup: pvr_vm_bind_op_fini(&bind_op); mutex_unlock(&vm_ctx->lock); return err; ``` **Minor nit:** The commit message says "all occurences" (should be "occurre= nces") and "NULL deference" (should be "dereference"). **Verdict:** The intent is correct and the fix is needed, but the implement= ation has a critical double-unlock bug. NAK in current form; a v3 with the = single-unlock-point fix described above should be straightforward. --- Generated by Claude Code Patch Reviewer