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.
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); + __i915_vma_put(vma);
spin_lock(&obj->vma.lock);
Op 27-01-2022 om 12:56 schreef Thomas Hellström:
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.
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);__i915_vma_put(vma); spin_lock(&obj->vma.lock);
Reviewed-by: Maarten Lankhorst maarten.lankhorst@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_.... 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);
On 1/28/22 23:32, Tvrtko Ursulin wrote:
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_.... 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.
Yes, should've had a fixes tag here. Luckily it fixes a very recent commit, which shouldn't have had time to be released yet.
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?
Yes. Ensure would have been better here. There is some rework of the vma destruction still needed, though so I'll update or remove the comment with those patches.
Thanks,
Thomas
Regards,
Tvrtko
__i915_vma_put(vma); spin_lock(&obj->vma.lock);
dri-devel@lists.freedesktop.org
-
Maarten Lankhorst -
Thomas Hellström -
Tvrtko Ursulin