Re: [PATCH 1/2] drm/msm: factor out a locking put_iova_spaces() wrapper

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The VMA reference count (`vma_ref`) is decremented and checked outside of the VMA/object lock, leading to a TOCTOU race where an active VMA can be unmapped and freed while in use by another thread.
--

--- Patch [1]: [PATCH 1/2] drm/msm: factor out a locking put_iova_spaces() wrapper ---
commit 3d4e26e168daef686a19b61570238be2722c0b4d
Author: Dmitry Baryshkov <[email protected]>
Subject: drm/msm: factor out a locking put_iova_spaces() wrapper

This commit factors out a locking put_iova_spaces_locked() wrapper and
updates callers. It aims to clean up redundant lock, tear down, and unlock
sequences around the teardown path.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index de4e60e28a711..8b979d84bb958 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c

[ ... ]

> @@ -105,11 +113,7 @@ void msm_gem_vma_put(struct drm_gem_object *obj)
>  		return;
>  
>  #ifdef CONFIG_DRM_MSM_KMS
> -	struct drm_exec exec;
> -
> -	msm_gem_lock_vm_and_obj(&exec, obj, priv->kms->vm);
>  	put_iova_spaces(obj, priv->kms->vm, true, "vma_put");
> -	drm_exec_fini(&exec);     /* drop locks */
>  #endif
>  }

[ ... ]

> @@ -408,8 +412,8 @@ static struct drm_gpuva *lookup_vma(struct drm_gem_object *obj,
>   * mapping.
>   */
>  static void
> -put_iova_spaces(struct drm_gem_object *obj, struct drm_gpuvm *vm,
> -		bool close, const char *reason)
> +put_iova_spaces_locked(struct drm_gem_object *obj, struct drm_gpuvm *vm,
> +		       bool close, const char *reason)
>  {
>  	struct drm_gpuvm_bo *vm_bo, *tmp;

[Severity: Critical]
This is a pre-existing issue, but can the lockless decrement of vma_ref in
msm_gem_vma_put() cause a race condition?

If CPU1 calls msm_gem_close() -> msm_gem_vma_put() and decrements vma_ref to
0, but gets preempted before taking the lock and calling put_iova_spaces():

CPU2 could concurrently open the GEM object (e.g., via flink), calling
msm_gem_open() -> msm_gem_vma_get(), incrementing vma_ref back to 1,
mapping the object, and submitting a job.

When CPU1 resumes, it continues into put_iova_spaces() and eventually
put_iova_spaces_locked():

drivers/gpu/drm/msm/msm_gem.c:put_iova_spaces_locked() {
        ...
        drm_gpuvm_bo_for_each_va_safe (vma, vmatmp, vm_bo) {
                msm_gem_vma_unmap(vma, reason);
                if (close)
                        msm_gem_vma_close(vma);
        }
        ...
}

Since vma_ref isn't re-checked inside the lock, the IOVA space is torn down
unconditionally. Could this result in tearing down an active IOVA mapping
while the GPU is executing a job, leading to an IOMMU fault or memory
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1