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.kess...@gmx.net>
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: sta...@kernel.org
---
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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
