From: Daniel Vetter Date: Wed, 14 Aug 2013 22:02:45 +0000 (+0200) Subject: drm/gem: completely close gem_open vs. gem_close races X-Git-Url: https://git.stricted.de/?a=commitdiff_plain;h=20228c447846da9399ead53fdbbc8ab69b47788a;p=GitHub%2Fmoto-9609%2Fandroid_kernel_motorola_exynos9610.git 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 Signed-off-by: Dave Airlie --- diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index adb9eda4fa1a..d47aa774d64b 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -308,23 +308,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); @@ -351,6 +354,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); @@ -627,13 +645,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 bf058470a0fd..063eac31b97b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1575,6 +1575,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);