Hi On Thu, Mar 27, 2014 at 9:23 PM, Thomas Hellstrom <thellstrom at 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. > > 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);
drm_master_create() doesn't need any locking (considering your removal of master_list), so this locking is exclusively to set ->master? See below. > 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); drm_master_put() resets the pointers to NULL. So _if_ the locking above is needed, then this one _must_ stay, too. I also don't quite understand why you move the struct_mutex locking into drm_master_destroy() instead of requiring callers to lock it as before? I mean, the whole function is protected by the lock.. > goto out_close; > } > } > - mutex_lock(&dev->struct_mutex); > if (dev->driver->master_set) { > ret = dev->driver->master_set(dev, priv, true); vmwgfx is the only driver using that, so I guess you reviewed this lock-change. > if (ret) { > /* drop both references if this fails */ > drm_master_put(&priv->minor->master); > drm_master_put(&priv->master); same as above > - 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); Weird white-space change.. > 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, > {for setting > int ret = 0; > > + mutex_lock(&dev->master_mutex); Yey! One more step towards DRM_UNLOCKED. > if (file_priv->is_master) > - return 0; > + goto out_unlock; > > - if (file_priv->minor->master && file_priv->minor->master != > file_priv->master) > - return -EINVAL; That one was redundant.. yey.. > + ret = -EINVAL; This breaks all drivers with set_master == NULL. We never return 0 then.. I also dislike setting error-codes before doing the comparison just to drop the curly brackets.. that used to be common kernel-coding-style, but I thought we all stopped doing that. Same for dropmaster below, but just my personal opinion. > + 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); You set minor->master unlocked here again. See my reasoning above.. > 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); Again setting minor->master unlocked. Thanks David > 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 > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel