drm/i915: Repeat unbinding during free if interrupted (v6)

If during the freeing of an object the unbind is interrupted by a system
call, which is quite possible if we have outstanding GPU writes that
must be flushed, the unbind is silently aborted. This still leaves the
AGP region and backing pages allocated, and perhaps more importantly,
the object remains upon the various lists exposing us to memory
corruption.

I think this is the cause behind the use-after-free, such as

  Bug 15664 - Graphics hang and kernel backtrace when starting Azureus
              with Compiz enabled
  https://bugzilla.kernel.org/show_bug.cgi?id=15664

v2: Daniel Vetter reminded me that kernel space programming is never easy.
We cannot simply spin to clear the pending signal and so must deferred
the freeing of the object until later.
v3: Run from the top level retire requests.
v4: Tested with P(return -ERESTARTSYS)=.5 from i915_gem_do_wait_request()
v5: Rebase against Eric's for-linus tree.
v6: Refactor, split and add a comment about avoiding unbounded recursion.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Eric Anholt <eric@anholt.net>

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a27780b7aef2d09157efa31b4d0ec2335dc44e80..906663b9929e7cf6b951bf3d91b135b0219b38fc 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -551,6 +551,14 @@ typedef struct drm_i915_private {
                /** LRU list of objects with fence regs on them. */
                struct list_head fence_list;
 
+               /**
+                * List of objects currently pending being freed.
+                *
+                * These objects are no longer in use, but due to a signal
+                * we were prevented from freeing them at the appointed time.
+                */
+               struct list_head deferred_free_list;
+
                /**
                 * We leave the user IRQ off as much as possible,
                 * but this means that requests will finish and never

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 138ca6ea5ef797779ceb503a919a78565df9fe1a..f45f385c84cde98d51a3aee84eee09a4ab343a50 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -53,6 +53,7 @@ static int i915_gem_evict_from_inactive_list(struct drm_device *dev);
 static int i915_gem_phys_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
                                struct drm_i915_gem_pwrite *args,
                                struct drm_file *file_priv);
+static void i915_gem_free_object_tail(struct drm_gem_object *obj);
 
 static LIST_HEAD(shrink_list);
 static DEFINE_SPINLOCK(shrink_list_lock);
@@ -1755,6 +1756,20 @@ i915_gem_retire_requests(struct drm_device *dev)
 {
        drm_i915_private_t *dev_priv = dev->dev_private;
 
+       if (!list_empty(&dev_priv->mm.deferred_free_list)) {
+           struct drm_i915_gem_object *obj_priv, *tmp;
+
+           /* We must be careful that during unbind() we do not
+            * accidentally infinitely recurse into retire requests.
+            * Currently:
+            *   retire -> free -> unbind -> wait -> retire_ring
+            */
+           list_for_each_entry_safe(obj_priv, tmp,
+                                    &dev_priv->mm.deferred_free_list,
+                                    list)
+                   i915_gem_free_object_tail(&obj_priv->base);
+       }
+
        i915_gem_retire_requests_ring(dev, &dev_priv->render_ring);
        if (HAS_BSD(dev))
                i915_gem_retire_requests_ring(dev, &dev_priv->bsd_ring);
@@ -4458,20 +4473,19 @@ int i915_gem_init_object(struct drm_gem_object *obj)
        return 0;
 }
 
-void i915_gem_free_object(struct drm_gem_object *obj)
+static void i915_gem_free_object_tail(struct drm_gem_object *obj)
 {
        struct drm_device *dev = obj->dev;
+       drm_i915_private_t *dev_priv = dev->dev_private;
        struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
+       int ret;
 
-       trace_i915_gem_object_destroy(obj);
-
-       while (obj_priv->pin_count > 0)
-               i915_gem_object_unpin(obj);
-
-       if (obj_priv->phys_obj)
-               i915_gem_detach_phys_object(dev, obj);
-
-       i915_gem_object_unbind(obj);
+       ret = i915_gem_object_unbind(obj);
+       if (ret == -ERESTARTSYS) {
+               list_move(&obj_priv->list,
+                         &dev_priv->mm.deferred_free_list);
+               return;
+       }
 
        if (obj_priv->mmap_offset)
                i915_gem_free_mmap_offset(obj);
@@ -4483,6 +4497,22 @@ void i915_gem_free_object(struct drm_gem_object *obj)
        kfree(obj_priv);
 }
 
+void i915_gem_free_object(struct drm_gem_object *obj)
+{
+       struct drm_device *dev = obj->dev;
+       struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
+
+       trace_i915_gem_object_destroy(obj);
+
+       while (obj_priv->pin_count > 0)
+               i915_gem_object_unpin(obj);
+
+       if (obj_priv->phys_obj)
+               i915_gem_detach_phys_object(dev, obj);
+
+       i915_gem_free_object_tail(obj);
+}
+
 /** Unbinds all inactive objects. */
 static int
 i915_gem_evict_from_inactive_list(struct drm_device *dev)
@@ -4756,6 +4786,7 @@ i915_gem_load(struct drm_device *dev)
        INIT_LIST_HEAD(&dev_priv->mm.gpu_write_list);
        INIT_LIST_HEAD(&dev_priv->mm.inactive_list);
        INIT_LIST_HEAD(&dev_priv->mm.fence_list);
+       INIT_LIST_HEAD(&dev_priv->mm.deferred_free_list);
        INIT_LIST_HEAD(&dev_priv->render_ring.active_list);
        INIT_LIST_HEAD(&dev_priv->render_ring.request_list);
        if (HAS_BSD(dev)) {