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>
(cherry picked from commit c996fd0b956450563454e7ccc97a82ca31f9d043)
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Conflicts:
	drivers/gpu/drm/drm_stub.c

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 7b5d196..405fc69 100644 (file)
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -240,12 +240,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;
                }
@@ -253,29 +252,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;
                        }
                }
@@ -283,7 +276,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);
@@ -311,6 +304,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:
@@ -498,11 +492,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))
@@ -521,6 +517,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 */
@@ -530,10 +527,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 fb514d2..5046180 100644 (file)
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -169,6 +169,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);
 
@@ -196,6 +197,7 @@ static void drm_master_destroy(struct kref *kref)
 
        drm_ht_remove(&master->magiclist);
 
+       mutex_unlock(&dev->struct_mutex);
        kfree(master);
 }
 
@@ -211,19 +213,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) {
@@ -233,27 +236,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;
 }
 
 static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
@@ -504,6 +513,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);
 
        if (drm_core_check_feature(dev, DRIVER_MODESET)) {
                ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL);
@@ -556,6 +566,7 @@ err_minors:
        drm_put_minor(dev->control);
        drm_put_minor(dev->render);
        drm_put_minor(dev->primary);
+       mutex_destroy(&dev->master_mutex);
        kfree(dev);
        return NULL;
 }
@@ -585,6 +596,8 @@ void drm_dev_free(struct drm_device *dev)
        drm_fs_inode_free(dev->anon_inode);
 
        kfree(dev->devname);
+
+       mutex_destroy(&dev->master_mutex);
        kfree(dev);
 }
 EXPORT_SYMBOL(drm_dev_free);

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2543643..bcc9a7d 100644 (file)
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -434,7 +434,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;
 
@@ -713,28 +714,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
@@ -1048,7 +1050,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 */