Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/nouveau: silently ignored lock failure causes unprotected list access --
commit 8fab8734d6eb64d1f1dd2d598d76c6f17c6efddd Author: Matthew Brost <[email protected]> Subject: drm/nouveau: use DRM_GPUVM_RESV_PROTECTED This commit sets DRM_GPUVM_RESV_PROTECTED for the nouveau drm_gpuvm. It introduces nouveau_uvmm_lock_vm_and_obj() to lock the VM's dma-resv together with the object's dma-resv when dropping a reference to a drm_gpuvm_bo. > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > index fc125fd44a9b7..13e11905b5f97 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c [ ... ] > @@ -1184,6 +1184,27 @@ bind_validate_region(struct nouveau_job *job) > return 0; > } > > +/* > + * Lock the VM's common dma-resv together with the one of @obj, as needed to > + * drop what may be the last reference of a &drm_gpuvm_bo. > + */ > +static void > +nouveau_uvmm_lock_vm_and_obj(struct nouveau_uvmm *uvmm, struct drm_exec > *exec, > + struct drm_gem_object *obj) > +{ > + int ret; > + > + drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, 2); > + drm_exec_until_all_locked(exec) { > + ret = drm_exec_lock_obj(exec, drm_gpuvm_resv_obj(&uvmm->base)); > + if (!ret) > + ret = drm_exec_lock_obj(exec, obj); > + drm_exec_retry_on_contention(exec); > + if (drm_WARN_ON(uvmm->base.drm, ret)) > + break; [Severity: High] If drm_exec_lock_obj() fails with an error like -ENOMEM (which can occur during internal memory allocations), the loop hits this break and returns. Does this mean the function returns without fully acquiring the necessary locks, while hiding the failure from the caller because of the void return type? > + } > +} [ ... ] > @@ -1594,9 +1625,11 @@ nouveau_uvmm_bind_job_cleanup(struct nouveau_job *job) > drm_gpuva_ops_free(&uvmm->base, op->ops); > > if (!IS_ERR_OR_NULL(op->vm_bo)) { > - dma_resv_lock(obj->resv, NULL); > + struct drm_exec exec; > + > + nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj); > drm_gpuvm_bo_put(op->vm_bo); [Severity: High] If nouveau_uvmm_lock_vm_and_obj() fails and returns without holding the dma_resv locks, will calling drm_gpuvm_bo_put() here result in unprotected concurrent list operations? The caller assumes the locks are securely held. Functions like drm_gpuvm_bo_put() can modify obj->gpuva.list and gpuvm->extobj.list. Under memory pressure on a shared GEM object, could this lead to list corruption or a use-after-free? > - dma_resv_unlock(obj->resv); > + drm_exec_fini(&exec); > } > > if (obj) [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
