[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2
Thomas Hellstrom
thellstrom at vmware.com
Thu Mar 27 13:23:18 PDT 2014
The master management was previously protected by the drm_device::struct_mutex.
In order to avoid locking order violations in a reworked dropped master
security check in the vmwgfx driver, break it out into a separate master_mutex.
Also remove drm_master::blocked since it's not used.
v2: Add an inline comment about what drm_device::master_mutex is protecting.
Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
Reviewed-by: Brian Paul <brianp at vmware.com>
---
drivers/gpu/drm/drm_fops.c | 23 +++++++++++++---------
drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++--------------
include/drm/drmP.h | 46 ++++++++++++++++++++++++--------------------
3 files changed, 63 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c7792b1..af55e2b 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -231,12 +231,13 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
/* if there is no current master make this fd it, but do not create
* any master object for render clients */
- mutex_lock(&dev->struct_mutex);
+ mutex_lock(&dev->master_mutex);
if (drm_is_primary_client(priv) && !priv->minor->master) {
/* create a new master */
+ mutex_lock(&dev->struct_mutex);
priv->minor->master = drm_master_create(priv->minor);
+ mutex_unlock(&dev->struct_mutex);
if (!priv->minor->master) {
- mutex_unlock(&dev->struct_mutex);
ret = -ENOMEM;
goto out_close;
}
@@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
/* take another reference for the copy in the local file priv */
priv->master = drm_master_get(priv->minor->master);
+ mutex_lock(&dev->struct_mutex);
priv->authenticated = 1;
mutex_unlock(&dev->struct_mutex);
if (dev->driver->master_create) {
ret = dev->driver->master_create(dev, priv->master);
if (ret) {
- mutex_lock(&dev->struct_mutex);
/* drop both references if this fails */
drm_master_put(&priv->minor->master);
drm_master_put(&priv->master);
- mutex_unlock(&dev->struct_mutex);
goto out_close;
}
}
- mutex_lock(&dev->struct_mutex);
if (dev->driver->master_set) {
ret = dev->driver->master_set(dev, priv, true);
if (ret) {
/* drop both references if this fails */
drm_master_put(&priv->minor->master);
drm_master_put(&priv->master);
- mutex_unlock(&dev->struct_mutex);
goto out_close;
}
}
@@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
/* get a reference to the master */
priv->master = drm_master_get(priv->minor->master);
}
- mutex_unlock(&dev->struct_mutex);
+ mutex_unlock(&dev->master_mutex);
mutex_lock(&dev->struct_mutex);
list_add(&priv->lhead, &dev->filelist);
mutex_unlock(&dev->struct_mutex);
@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
return 0;
out_close:
+ mutex_unlock(&dev->master_mutex);
if (dev->driver->postclose)
dev->driver->postclose(dev, priv);
out_prime_destroy:
@@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp)
}
mutex_unlock(&dev->ctxlist_mutex);
- mutex_lock(&dev->struct_mutex);
+ mutex_lock(&dev->master_mutex);
if (file_priv->is_master) {
struct drm_master *master = file_priv->master;
struct drm_file *temp;
+
+ mutex_lock(&dev->struct_mutex);
list_for_each_entry(temp, &dev->filelist, lhead) {
if ((temp->master == file_priv->master) &&
(temp != file_priv))
@@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp)
master->lock.file_priv = NULL;
wake_up_interruptible_all(&master->lock.lock_queue);
}
+ mutex_unlock(&dev->struct_mutex);
if (file_priv->minor->master == file_priv->master) {
/* drop the reference held my the minor */
@@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp)
}
}
- /* drop the reference held my the file priv */
+ /* drop the master reference held by the file priv */
if (file_priv->master)
drm_master_put(&file_priv->master);
file_priv->is_master = 0;
+ mutex_unlock(&dev->master_mutex);
+
+ mutex_lock(&dev->struct_mutex);
list_del(&file_priv->lhead);
mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index d344513..10c8303 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref)
struct drm_device *dev = master->minor->dev;
struct drm_map_list *r_list, *list_temp;
+ mutex_lock(&dev->struct_mutex);
if (dev->driver->master_destroy)
dev->driver->master_destroy(dev, master);
@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)
drm_ht_remove(&master->magiclist);
+ mutex_unlock(&dev->struct_mutex);
kfree(master);
}
@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
{
int ret = 0;
+ mutex_lock(&dev->master_mutex);
if (file_priv->is_master)
- return 0;
+ goto out_unlock;
- if (file_priv->minor->master && file_priv->minor->master != file_priv->master)
- return -EINVAL;
+ ret = -EINVAL;
+ if (file_priv->minor->master)
+ goto out_unlock;
if (!file_priv->master)
- return -EINVAL;
+ goto out_unlock;
- if (file_priv->minor->master)
- return -EINVAL;
-
- mutex_lock(&dev->struct_mutex);
file_priv->minor->master = drm_master_get(file_priv->master);
file_priv->is_master = 1;
if (dev->driver->master_set) {
@@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
drm_master_put(&file_priv->minor->master);
}
}
- mutex_unlock(&dev->struct_mutex);
+out_unlock:
+ mutex_unlock(&dev->master_mutex);
return ret;
}
int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv)
{
+ int ret = -EINVAL;
+
+ mutex_lock(&dev->master_mutex);
if (!file_priv->is_master)
- return -EINVAL;
+ goto out_unlock;
if (!file_priv->minor->master)
- return -EINVAL;
+ goto out_unlock;
- mutex_lock(&dev->struct_mutex);
+ ret = 0;
if (dev->driver->master_drop)
dev->driver->master_drop(dev, file_priv, false);
drm_master_put(&file_priv->minor->master);
file_priv->is_master = 0;
- mutex_unlock(&dev->struct_mutex);
- return 0;
+
+out_unlock:
+ mutex_unlock(&dev->master_mutex);
+ return ret;
}
/*
@@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
spin_lock_init(&dev->event_lock);
mutex_init(&dev->struct_mutex);
mutex_init(&dev->ctxlist_mutex);
+ mutex_init(&dev->master_mutex);
dev->anon_inode = drm_fs_inode_new();
if (IS_ERR(dev->anon_inode)) {
@@ -620,6 +627,7 @@ err_minors:
drm_minor_free(dev, DRM_MINOR_CONTROL);
drm_fs_inode_free(dev->anon_inode);
err_free:
+ mutex_destroy(&dev->master_mutex);
kfree(dev);
return NULL;
}
@@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref)
drm_minor_free(dev, DRM_MINOR_CONTROL);
kfree(dev->devname);
+
+ mutex_destroy(&dev->master_mutex);
kfree(dev);
}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 30670d3..9a9a657 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -435,7 +435,8 @@ struct drm_prime_file_private {
struct drm_file {
unsigned always_authenticated :1;
unsigned authenticated :1;
- unsigned is_master :1; /* this file private is a master for a minor */
+ /* Whether we're master for a minor. Protected by master_mutex */
+ unsigned is_master :1;
/* true when the client has asked us to expose stereo 3D mode flags */
unsigned stereo_allowed :1;
@@ -714,28 +715,29 @@ struct drm_gem_object {
#include <drm/drm_crtc.h>
-/* per-master structure */
+/**
+ * struct drm_master - drm master structure
+ *
+ * @refcount: Refcount for this master object.
+ * @minor: Link back to minor char device we are master for. Immutable.
+ * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
+ * @unique_len: Length of unique field. Protected by drm_global_mutex.
+ * @unique_size: Amount allocated. Protected by drm_global_mutex.
+ * @magiclist: Hash of used authentication tokens. Protected by struct_mutex.
+ * @magicfree: List of used authentication tokens. Protected by struct_mutex.
+ * @lock: DRI lock information.
+ * @driver_priv: Pointer to driver-private information.
+ */
struct drm_master {
-
- struct kref refcount; /* refcount for this master */
-
- struct drm_minor *minor; /**< link back to minor we are a master for */
-
- char *unique; /**< Unique identifier: e.g., busid */
- int unique_len; /**< Length of unique field */
- int unique_size; /**< amount allocated */
-
- int blocked; /**< Blocked due to VC switch? */
-
- /** \name Authentication */
- /*@{ */
+ struct kref refcount;
+ struct drm_minor *minor;
+ char *unique;
+ int unique_len;
+ int unique_size;
struct drm_open_hash magiclist;
struct list_head magicfree;
- /*@} */
-
- struct drm_lock_data lock; /**< Information on hardware lock */
-
- void *driver_priv; /**< Private structure for driver to use */
+ struct drm_lock_data lock;
+ void *driver_priv;
};
/* Size of ringbuffer for vblank timestamps. Just double-buffer
@@ -1050,7 +1052,8 @@ struct drm_minor {
struct list_head debugfs_list;
struct mutex debugfs_lock; /* Protects debugfs_list. */
- struct drm_master *master; /* currently active master for this node */
+ /* currently active master for this node. Protected by master_mutex */
+ struct drm_master *master;
struct drm_mode_group mode_group;
};
@@ -1100,6 +1103,7 @@ struct drm_device {
/*@{ */
spinlock_t count_lock; /**< For inuse, drm_device::open_count, drm_device::buf_use */
struct mutex struct_mutex; /**< For others */
+ struct mutex master_mutex; /**< For drm_minor::master and drm_file::is_master */
/*@} */
/** \name Usage Counters */
--
1.7.10.4
More information about the dri-devel
mailing list