summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDaniel Vetter <daniel.vetter@ffwll.ch>2013-08-14 22:02:45 (GMT)
committerInki Dae <inki.dae@samsung.com>2014-10-15 11:05:17 (GMT)
commitbd41b5c686b8ffc867a65e7460d09a71362160be (patch)
tree90e6a1bbf9937ba827cd66d769b0875b5687f34b
parent45445d7b1116797508323d882b1f62ed4c07faf1 (diff)
downloadlinux-3.10-bd41b5c686b8ffc867a65e7460d09a71362160be.zip
linux-3.10-bd41b5c686b8ffc867a65e7460d09a71362160be.tar.gz
linux-3.10-bd41b5c686b8ffc867a65e7460d09a71362160be.tar.bz2
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>
-rw-r--r--drivers/gpu/drm/drm_gem.c42
-rw-r--r--include/drm/drmP.h3
2 files changed, 34 insertions, 11 deletions
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 129d4ec..235c376 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;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 747cb7c..eaf6624 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1714,6 +1714,9 @@ drm_gem_object_unreference_unlocked(struct drm_gem_object *obj)
}
}
+int drm_gem_handle_create_tail(struct drm_file *file_priv,
+ struct drm_gem_object *obj,
+ u32 *handlep);
int drm_gem_handle_create(struct drm_file *file_priv,
struct drm_gem_object *obj,
u32 *handlep);