drm: Protect the master management with a drm_device::master_mutex v3
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>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Daniel Vetter <daniel@ffwll.ch>
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 @@
/* 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 @@
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 @@
/* 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 @@
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 @@
}
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 @@
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 @@
}
}
- /* 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 a378af4..fac6f98 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -144,6 +144,7 @@
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);
@@ -171,6 +172,7 @@
drm_ht_remove(&master->magiclist);
+ mutex_unlock(&dev->struct_mutex);
kfree(master);
}
@@ -186,19 +188,20 @@
{
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->minor->master) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
- if (!file_priv->master)
- return -EINVAL;
+ if (!file_priv->master) {
+ ret = -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) {
@@ -208,27 +211,33 @@
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;
}
/*
@@ -559,6 +568,7 @@
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)) {
@@ -612,6 +622,7 @@
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;
}
@@ -633,6 +644,8 @@
drm_minor_free(dev, DRM_MINOR_CONTROL);
kfree(dev->devname);
+
+ mutex_destroy(&dev->master_mutex);
kfree(dev);
}