summaryrefslogtreecommitdiff
path: root/drivers
diff options
context:
space:
mode:
authorDaniel Vetter <daniel.vetter@ffwll.ch>2013-08-15 00:02:45 +0200
committerChanho Park <chanho61.park@samsung.com>2014-11-21 19:13:43 +0900
commit462da3c532f569d497d8f506f2d027c78e0dc638 (patch)
treeb6e6470d64c166583c99b847d9c5c3c7dcd7155a /drivers
parentdf1db9bf3ff56007e063b26f4c4a4c1b478c192a (diff)
downloadlinux-3.10-462da3c532f569d497d8f506f2d027c78e0dc638.tar.gz
linux-3.10-462da3c532f569d497d8f506f2d027c78e0dc638.tar.bz2
linux-3.10-462da3c532f569d497d8f506f2d027c78e0dc638.zip
drm/gem: completely close gem_open vs. gem_close races
The gem flink name holds a reference onto the object itself, and this self-reference would prevent an flink'ed object from every being freed. To break that loop we remove the flink name when the last userspace handle disappears, i.e. when obj->handle_count reaches 0. Now in gem_open we drop the dev->object_name_lock between the flink name lookup and actually adding the handle. This means a concurrent gem_close of the last handle could result in the flink name getting reaped right inbetween, i.e. Thread 1 Thread 2 gem_open gem_close flink -> obj lookup handle_count drops to 0 remove flink name create_handle handle_count++ If someone now flinks this object again, we'll get a new flink name. We can close this race by removing the lock dropping and making the entire lookup+handle_create sequence atomic. Unfortunately to still be able to share the handle_create logic this requires a handle_create_tail function which drops the lock - we can't hold the object_name_lock while calling into a driver's ->gem_open callback. Note that for flink fixing this race isn't really important, since racing gem_open against gem_close is clearly a userspace bug. And no matter how the race ends, we won't leak any references. But with dma-buf where the userspace dma-buf fd itself is refcounted this is a valid sequence and hence we should fix it. Therefore this patch here is just a warm-up exercise (and for consistency between flink buffer sharing and dma-buf buffer sharing with self-imports). Also note that this extension of the critical section in gem_open protected by dev->object_name_lock only works because it's now a mutex: A spinlock would conflict with the potential memory allocation in idr_preload(). This is exercises by igt/gem_flink_race/flink_name. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Dave Airlie <airlied@redhat.com>
Diffstat (limited to 'drivers')
-rw-r--r--drivers/gpu/drm/drm_gem.c42
1 files changed, 31 insertions, 11 deletions
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 129d4ecf59a..235c3763f5f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -371,23 +371,26 @@ int drm_gem_dumb_destroy(struct drm_file *file,
EXPORT_SYMBOL(drm_gem_dumb_destroy);
/**
- * Create a handle for this object. This adds a handle reference
- * to the object, which includes a regular reference count. Callers
- * will likely want to dereference the object afterwards.
+ * drm_gem_handle_create_tail - internal functions to create a handle
+ *
+ * This expects the dev->object_name_lock to be held already and will drop it
+ * before returning. Used to avoid races in establishing new handles when
+ * importing an object from either an flink name or a dma-buf.
*/
int
-drm_gem_handle_create(struct drm_file *file_priv,
- struct drm_gem_object *obj,
- u32 *handlep)
+drm_gem_handle_create_tail(struct drm_file *file_priv,
+ struct drm_gem_object *obj,
+ u32 *handlep)
{
struct drm_device *dev = obj->dev;
int ret;
+ WARN_ON(!mutex_is_locked(&dev->object_name_lock));
+
/*
* Get the user-visible handle using idr. Preload and perform
* allocation under our spinlock.
*/
- mutex_lock(&dev->object_name_lock);
idr_preload(GFP_KERNEL);
spin_lock(&file_priv->table_lock);
@@ -414,6 +417,21 @@ drm_gem_handle_create(struct drm_file *file_priv,
return 0;
}
+
+/**
+ * Create a handle for this object. This adds a handle reference
+ * to the object, which includes a regular reference count. Callers
+ * will likely want to dereference the object afterwards.
+ */
+int
+drm_gem_handle_create(struct drm_file *file_priv,
+ struct drm_gem_object *obj,
+ u32 *handlep)
+{
+ mutex_lock(&obj->dev->object_name_lock);
+
+ return drm_gem_handle_create_tail(file_priv, obj, handlep);
+}
EXPORT_SYMBOL(drm_gem_handle_create);
@@ -690,13 +708,15 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
mutex_lock(&dev->object_name_lock);
obj = idr_find(&dev->object_name_idr, (int) args->name);
- if (obj)
+ if (obj) {
drm_gem_object_reference(obj);
- mutex_unlock(&dev->object_name_lock);
- if (!obj)
+ } else {
+ mutex_unlock(&dev->object_name_lock);
return -ENOENT;
+ }
- ret = drm_gem_handle_create(file_priv, obj, &handle);
+ /* drm_gem_handle_create_tail unlocks dev->object_name_lock. */
+ ret = drm_gem_handle_create_tail(file_priv, obj, &handle);
drm_gem_object_unreference_unlocked(obj);
if (ret)
return ret;