Re: [Intel-gfx] [PATCH] drm/i915: Fix a race between vma / object destruction and unbinding

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/01/2022 11:56, Thomas Hellström wrote:
> The vma destruction code was using an unlocked advisory check for
> drm_mm_node_allocated() to avoid racing with eviction code unbinding
> the vma.
> 
> This is very fragile and prohibits the dereference of non-refcounted
> pointers of dying vmas after a call to __i915_vma_unbind(). It also
> prohibits the dereference of vma->obj of refcounted pointers of
> dying vmas after a call to __i915_vma_unbind(), since even if a
> refcount is held on the vma, that won't guarantee that its backing
> object doesn't get destroyed.
> 
> So introduce an unbind under the vm mutex at object destroy time,
> removing all weak references of the vma and its object from the
> object vma list and from the vm bound list.

Maarten suggested this fixes an oops like seen in 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22133/shard-snb6/igt@gem_softpin@softpin.html. 
If that is so, what would be the Fixes: tag to put here? Although it is 
too late now so hopefully bug was introduced in something yet unreleased.

> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_object.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 1a9e1f940a7d..e03e362d320b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -280,6 +280,12 @@ void __i915_gem_object_pages_fini(struct drm_i915_gem_object *obj)
>   			GEM_BUG_ON(vma->obj != obj);
>   			spin_unlock(&obj->vma.lock);
>   
> +			/* Verify that the vma is unbound under the vm mutex. */
> +			mutex_lock(&vma->vm->mutex);
> +			atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
> +			__i915_vma_unbind(vma);
> +			mutex_unlock(&vma->vm->mutex);

Hm I am not up to speed with the latest design, but how does the verb 
verify and absence of conditionals reconcile here? Does the comment need 
improving?

Regards,

Tvrtko

> +
>   			__i915_vma_put(vma);
>   
>   			spin_lock(&obj->vma.lock);
>