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 <[email protected]> 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/[email protected]?part=4
