diff mbox

[06/16] drm: Protect the master management with a drm_device::master_mutex v3

Message ID 1395999282-12553-1-git-send-email-thellstrom@vmware.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Hellstrom March 28, 2014, 9:34 a.m. UTC
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.
Locking order is master_mutex -> struct_mutex.

Also remove drm_master::blocked since it's not used.

v2: Add an inline comment about what drm_device::master_mutex is protecting.
v3: Remove unneeded struct_mutex locks. Fix error returns in
    drm_setmaster_ioctl().

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
---
 drivers/gpu/drm/drm_fops.c |   22 ++++++++++-----------
 drivers/gpu/drm/drm_stub.c |   43 ++++++++++++++++++++++++++---------------
 include/drm/drmP.h         |   46 ++++++++++++++++++++++++--------------------
 3 files changed, 64 insertions(+), 47 deletions(-)

Comments

David Herrmann March 28, 2014, 11:23 a.m. UTC | #1
Hi

On Fri, Mar 28, 2014 at 10:34 AM, Thomas Hellstrom
<thellstrom@vmware.com> wrote:
> 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.
> Locking order is master_mutex -> struct_mutex.
>
> Also remove drm_master::blocked since it's not used.
>
> v2: Add an inline comment about what drm_device::master_mutex is protecting.
> v3: Remove unneeded struct_mutex locks. Fix error returns in
>     drm_setmaster_ioctl().

Thanks, looks much better now. Tested on hsw, so I'm fine with pushing
this into 3.15.

Thanks
David
Thomas Hellstrom March 28, 2014, 11:52 a.m. UTC | #2
On 03/28/2014 12:23 PM, David Herrmann wrote:
> Hi
>
> On Fri, Mar 28, 2014 at 10:34 AM, Thomas Hellstrom
> <thellstrom@vmware.com> wrote:
>> 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.
>> Locking order is master_mutex -> struct_mutex.
>>
>> Also remove drm_master::blocked since it's not used.
>>
>> v2: Add an inline comment about what drm_device::master_mutex is protecting.
>> v3: Remove unneeded struct_mutex locks. Fix error returns in
>>     drm_setmaster_ioctl().
> Thanks, looks much better now. Tested on hsw, so I'm fine with pushing
> this into 3.15.
>
> Thanks
> David
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Thanks. Want a Reviewed-By: or Acked-By: added?

/Thomas
David Herrmann March 28, 2014, 12:13 p.m. UTC | #3
Hi

On Fri, Mar 28, 2014 at 12:52 PM, Thomas Hellstrom
<thellstrom@vmware.com> wrote:
> Thanks. Want a Reviewed-By: or Acked-By: added?

Oh, sure:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c7792b1..a0ce39c 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -231,12 +231,11 @@  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 */
 		priv->minor->master = drm_master_create(priv->minor);
 		if (!priv->minor->master) {
-			mutex_unlock(&dev->struct_mutex);
 			ret = -ENOMEM;
 			goto out_close;
 		}
@@ -244,29 +243,23 @@  static int drm_open_helper(struct inode *inode, struct file *filp,
 		priv->is_master = 1;
 		/* take another reference for the copy in the local file priv */
 		priv->master = drm_master_get(priv->minor->master);
-
 		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,7 +267,7 @@  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);
@@ -302,6 +295,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 +483,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 +508,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 +518,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..8f65045 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,20 @@  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;
-
-	if (!file_priv->master)
-		return -EINVAL;
+	if (file_priv->minor->master) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
 
-	if (file_priv->minor->master)
-		return -EINVAL;
+	if (!file_priv->master) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
 
-	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 +219,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 +576,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 +630,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 +652,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 */