Re: [PATCH v3 7/8] drm/nouveau: use DRM_GPUVM_RESV_PROTECTED

[sashiko-bot@kernel.org]

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 <matthew.brost@intel.com>
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/20261001220632.3190896-1-matthew.brost@intel.com?part=7