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@gmx.net Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: stable@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,