[PATCH] drm: Hold the mutex when dropping the last GEM reference

[Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>]

In order to be fully threadsafe we need to check that the drm_gem_object
refcount is still 0 after acquiring the mutex in order to call the free
function. Otherwise, we may encounter scenarios like:

Thread A:                                        Thread B:
drm_gem_close
unreference_unlocked
kref_put                                         mutex_lock
...                                              i915_gem_evict
...                                              kref_get -> BUG
...                                              i915_gem_unbind
...                                              kref_put
...                                              i915_gem_object_free
...                                              mutex_unlock
mutex_lock
i915_gem_object_free -> BUG
i915_gem_object_unbind
kfree
mutex_unlock

To remmedy this, we could break the kref api and do:
  if (atomic_dec_and_test(&obj->refcount)) {
	  mutex_lock(&dev->mutex);
	  if (atomic_read(&obj->refcount) == 0)
		  dev->funcs->free(obj);
	  mutex_unlock(&dev->mutex);
  }
and similarly remove the BUG_ON(kref_get(obj->refcounts)==0).

It is simpler instead to simply rewrite the unlocked unreference variant
to take the dev->struct_mutex around the kref_put.

Note that no driver is currently using the free_unlocked vfunc and it is
scheduled for removal.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=30454
Reported-by: Magnus Kessler <Magnus.Kessler@xxxxxxx>
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxx
---
 drivers/gpu/drm/drm_gem.c |   22 ----------------------
 include/drm/drmP.h        |    9 ++++++---
 2 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index f7e61be..5663d27 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -462,28 +462,6 @@ drm_gem_object_free(struct kref *kref)
 }
 EXPORT_SYMBOL(drm_gem_object_free);
 
-/**
- * Called after the last reference to the object has been lost.
- * Must be called without holding struct_mutex
- *
- * Frees the object
- */
-void
-drm_gem_object_free_unlocked(struct kref *kref)
-{
-	struct drm_gem_object *obj = (struct drm_gem_object *) kref;
-	struct drm_device *dev = obj->dev;
-
-	if (dev->driver->gem_free_object_unlocked != NULL)
-		dev->driver->gem_free_object_unlocked(obj);
-	else if (dev->driver->gem_free_object != NULL) {
-		mutex_lock(&dev->struct_mutex);
-		dev->driver->gem_free_object(obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
-}
-EXPORT_SYMBOL(drm_gem_object_free_unlocked);
-
 static void drm_gem_object_ref_bug(struct kref *list_kref)
 {
 	BUG();
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 07e4726..76c03a0 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1456,7 +1456,6 @@ int drm_gem_init(struct drm_device *dev);
 void drm_gem_destroy(struct drm_device *dev);
 void drm_gem_object_release(struct drm_gem_object *obj);
 void drm_gem_object_free(struct kref *kref);
-void drm_gem_object_free_unlocked(struct kref *kref);
 struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev,
 					    size_t size);
 int drm_gem_object_init(struct drm_device *dev,
@@ -1484,8 +1483,12 @@ drm_gem_object_unreference(struct drm_gem_object *obj)
 static inline void
 drm_gem_object_unreference_unlocked(struct drm_gem_object *obj)
 {
-	if (obj != NULL)
-		kref_put(&obj->refcount, drm_gem_object_free_unlocked);
+	if (obj != NULL) {
+		struct drm_device *dev = obj->dev;
+		mutex_lock(&dev->struct_mutex);
+		kref_put(&obj->refcount, drm_gem_object_free);
+		mutex_unlock(&dev->struct_mutex);
+	}
 }
 
 int drm_gem_handle_create(struct drm_file *file_priv,
-- 
1.7.1

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel