Re: [PATCH v2 4/5] drm/msm: use DRM_GPUVM_RESV_PROTECTED for VM_BIND VMs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: lockless list corruption in msm_submit_retire() for VM_BIND contexts
--

commit 27940f439eb72422df49dffc6f1f360df645fb95
Author: Matthew Brost <matthew.brost@intel.com>

    drm/msm: use DRM_GPUVM_RESV_PROTECTED for VM_BIND VMs

    msm_gem_vm_create() creates every drm_gpuvm without
    DRM_GPUVM_RESV_PROTECTED. The internal spinlocks buy nothing for
    VM_BIND VMs, so set DRM_GPUVM_RESV_PROTECTED for those VMs.

>     Kernel managed VMs are left alone. The legacy submit path holds a vm_bo
>     reference per BO and drops it in msm_submit_retire() with only the
>     object's resv held, which could be the last reference once the VMA is
>     gone.

Is this assumption accurate? The legacy submit path doesn't seem strictly
limited to Kernel managed VMs.

An unprivileged userspace process can opt into MSM_PARAM_EN_VM_BIND, making
the VM userspace managed, but then still create a legacy submit queue. By
issuing a legacy MSM_GEM_SUBMIT ioctl, the legacy submit path is executed
on a userspace managed VM.

> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 1badec3caa7b8..c7cdd32a798f7 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -818,11 +818,18 @@ msm_gem_vm_create(struct drm_device *drm, struct msm_mmu *mmu, const char *name,
>  		  u64 va_start, u64 va_size, bool managed)
>  {
>  	/*
> -	 * We mostly want to use DRM_GPUVM_RESV_PROTECTED, except that
> -	 * makes drm_gpuvm_bo_evict() a no-op for extobjs (ie. we loose
> -	 * tracking that an extobj is evicted) :facepalm:
> +	 * Userspace managed (VM_BIND) VMs only ever touch the gpuvm's extobj
> +	 * and evicted lists with the VM's resv held, so use
> +	 * DRM_GPUVM_RESV_PROTECTED for those.  drm_gpuvm_bo_evict() cannot
> +	 * put an extobj on the evicted list there, but it records the
> +	 * eviction and drm_gpuvm_prepare_objects() moves it onto the list
> +	 * before drm_gpuvm_validate() runs, so nothing is lost.
> +	 *
> +	 * Kernel managed VMs keep the internal spinlocks, since the legacy
> +	 * submit path can drop the last vm_bo reference with only the
> +	 * object's resv held (see msm_submit_retire()).
>  	 */
> -	enum drm_gpuvm_flags flags = 0;
> +	enum drm_gpuvm_flags flags = managed ? 0 : DRM_GPUVM_RESV_PROTECTED;

[Severity: High]
Does this cause lockless list corruption when a legacy submit interacts with
a VM_BIND context?

If an unprivileged userspace process interleaves legacy submits with VM_BIND
unmap operations on a VM_BIND context, the legacy submit ioctl will pin
objects and take a reference on the vm_bo. When the legacy submit retires,
msm_submit_retire() is called:

drivers/gpu/drm/msm/msm_gem_submit.c:msm_submit_retire() {
	...
	msm_gem_lock(obj);
	drm_gpuvm_bo_put(vm_bo);
	msm_gem_unlock(obj);
	...
}

This drops the vm_bo reference while holding only the object's resv lock.
If userspace concurrently unmapped the VMA via an asynchronous VM_BIND
operation, dropping its reference, this could be the last reference.

When the last reference is dropped, drm_gpuvm_bo_destroy() executes:

drivers/gpu/drm/drm_gpuvm.c:drm_gpuvm_bo_destroy() {
	...
	drm_gpuvm_bo_list_del(vm_bo, extobj, lock);
	drm_gpuvm_bo_list_del(vm_bo, evict, lock);
	...
}

Because DRM_GPUVM_RESV_PROTECTED is set for the VM_BIND context,
drm_gpuvm_resv_protected() evaluates to true, skipping the internal spinlocks
(lock is false). Since msm_submit_retire() did not hold the VM's resv lock,
the list_del operations are performed locklessly, leading to list corruption.

>  	struct msm_gem_vm *vm;
>  	struct drm_gem_object *dummy_gem;
>  	int ret = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001162001.3123877-1-matthew.brost@intel.com?part=4