| Message ID | 1395951799-3666-3-git-send-email-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Thu, Mar 27, 2014 at 9:23 PM, 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. > > 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@vmware.com> > Reviewed-by: Brian Paul <brianp@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, David. Thanks for reviewing. I'll try to address all your concerns and resend. /Thomas On 03/28/2014 01:19 AM, David Herrmann wrote: > Hi > > On Thu, Mar 27, 2014 at 9:23 PM, 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. >> >> 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@vmware.com> >> Reviewed-by: Brian Paul <brianp@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@lists.freedesktop.org >> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/dri-devel&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=%2B64SF93E3xQn2TiEi9M77TN%2BoMizm3hGFl62Q%2Fc6Dps%3D%0A&s=136e04f9f91ff52cf2d7dd89e04aa547a3b45a0ebd33442cdb8a76291445dcdf
Hi, again! I've looked through the code again and have some answers to your questions. Will post an updated patch soon. On 03/28/2014 01:19 AM, David Herrmann wrote: > Hi > > On Thu, Mar 27, 2014 at 9:23 PM, 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. >> >> 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@vmware.com> >> Reviewed-by: Brian Paul <brianp@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. No. The locking here is, as you say, unneeded. drm_minor::master is protected by master_mutex. I'll remove, and while doing that, I'll also remove the unneeded locking around priv->authenticated = 1. >> 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. It's unneeded, so this should be OK. As for drm_file::master, it should be considered immutable, except for drm_file open() and release() where nobody is racing us. > > 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. Yes. > >> if (ret) { >> /* drop both references if this fails */ >> drm_master_put(&priv->minor->master); >> drm_master_put(&priv->master); > same as above No change needed. > >> - 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.. Will fix. > >> 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. Will fix. > >> + 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.. Protected by master_mutex. > >> 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. Protected by master_mutex. > > Thanks > David > > Thanks, Thomas
On 03/28/2014 01:19 AM, David Herrmann wrote: > Hi > > > > 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.. Before, struct_mutex was required outside of drm_master_put(), because the argument to drm_master_put was also protected by struct_mutex. Now that's no longer the case and I chose to move the locking into the destructor, avoiding a number of unnecessary struct_mutex locks (the master destructor is called more seldom than master_put()). Also in general I believe one should avoid locking around unreferencing if possible, because it leads to ugly constructs (and even static code analyzers complaining) if the lock has to be temporarily released in the destructor to avoid locking order violations. But in the end I guess that's really a matter of taste. /Thomas
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 */