| Message ID | 1395753548-17441-7-git-send-email-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Tue, Mar 25, 2014 at 2:18 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. Could you elaborate on that? What exactly is "master_mutex" protecting? "struct_mutex" is used to serialize all entry-points into the drm-device (and thus the driver) and also, often implicitly, as spin-lock for "struct drm_device" data protection. Regarding master_mutex I have several questions: - Can you give an example how vmwgfx dead-locks with your reworked code? - Why don't add a spin-lock to "drm_file" instead? Use that one to manage master contexts, but keep "struct_mutex" whenever calling into driver callbacks (set_master/drop_master) - why is master_mutex per device and not per-minor? I thought masters on minors are _entirely_ independent? Few more comments inline. > 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 e6cdd0f..dad571f 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_legacy_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); What's that struct_mutex doing here? We're in ->open(), there is no-one racing against us. > 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 1521036..2b387b0 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? */ You silently dropped that field. At least mention it in the commit-message if it's unused. > - > - /** \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; Comments, comments, comments! Lets avoid adding another undocumented mutex here. Or at least mark it as private to drm_master_*() functions. Thanks David > /*@} */ > > /** \name Usage Counters */ > -- > 1.7.10.4 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 03/26/2014 08:08 PM, David Herrmann wrote: > Hi > > On Tue, Mar 25, 2014 at 2:18 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. > Could you elaborate on that? What exactly is "master_mutex" > protecting? Its protecting drm_file::is_master and drm_minor::master, as per inline comments. It's also a candidate for replacing drm_global_mutex to avoid the dropmaster and setmaster ioctls racing when the drm_global_mutex eventually goes away. > "struct_mutex" is used to serialize all entry-points into > the drm-device (and thus the driver) and also, often implicitly, as > spin-lock for "struct drm_device" data protection. No. DRM locking was added as an after-though, and is a horrendous mess. Nobody really knows what's protecting what, and that has caused a lot of grief in the past. Probably most so for the Intel driver that relied (relies?) on the struct_mutex to protect everything. The drm_global_mutex is used to serialize the non-lock-audited entry points into the drm device. The struct_mutex is used for data protection of most core drm structures and serializing here and there. Modern drivers have no locks held when entering their ioctls. Also we should not confuse mutexes and spinlocks in this context, as they have very different semantics. > > Regarding master_mutex I have several questions: > - Can you give an example how vmwgfx dead-locks with your reworked code? Sure. The reworked driver takes the ttm lock in non-exclusive mode before entering an ioctl. Ioctls will then internally take the struct_mutex. The dropmaster and setmaster ioctls would (before this patch) take the struct_mutex and then in the driver code takes the ttm lock in exclusive mode. We would have a lock inversion and a potential deadlock. > - Why don't add a spin-lock to "drm_file" instead? Use that one to > manage master contexts, but keep "struct_mutex" whenever calling into > driver callbacks (set_master/drop_master) See above. We can't have a lock in the drm_file structure since it protects drm_minor data. Also, while it might be possible to restructure some code to be able to use spinlocks instead of mutexes I see no reason to. The established locking order now is master_mutex -> ttm_lock -> struct_mutex which means master_mutex must be a mutex. > - why is master_mutex per device and not per-minor? I thought masters > on minors are _entirely_ independent? Because currently there is only one master capable minor per device, so it would be equivalent. And even if there were more, there is no reason to expect any contention and thus a single master_mutex would be fine. > > Few more comments inline. > >> 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 e6cdd0f..dad571f 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_legacy_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); > What's that struct_mutex doing here? We're in ->open(), there is > no-one racing against us. Well, it was held at that point before, and the purpose of this patch is not to generally clean up struct_mutex usage. > >> 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 1521036..2b387b0 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? */ > You silently dropped that field. At least mention it in the > commit-message if it's unused. Sure. > >> - >> - /** \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; > Comments, comments, comments! Lets avoid adding another undocumented > mutex here. Or at least mark it as private to drm_master_*() > functions. Sure. > > Thanks > David > >> /*@} */ >> >> /** \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=aQtwaWHv2a0%2B862ao5fkuBGGYVf%2B4WRqrDEgHLSsyzI%3D%0A&s=f27e1a390d6150945a05ccdde58cdd68df313e1b8b255911c3380b60424bba98 Thanks, Thomas
On Wed, Mar 26, 2014 at 09:40:18PM +0100, Thomas Hellstrom wrote: > On 03/26/2014 08:08 PM, David Herrmann wrote: > > "struct_mutex" is used to serialize all entry-points into > > the drm-device (and thus the driver) and also, often implicitly, as > > spin-lock for "struct drm_device" data protection. > > No. DRM locking was added as an after-though, and is a horrendous mess. > Nobody really knows what's protecting what, and that has caused a lot of > grief in the past. Probably most so for the Intel driver that relied > (relies?) on the struct_mutex to protect everything. The > drm_global_mutex is used to serialize the non-lock-audited entry points > into the drm device. The struct_mutex is used for data protection of > most core drm structures and serializing here and there. Modern drivers > have no locks held when entering their ioctls. Also we should not > confuse mutexes and spinlocks in this context, as they have very > different semantics. As the guy who gets to live the locking mess called dev->struct_mutex I holeheartedly welcome any efforts to split out clear subparts away from it. I actually had this very idea of adding a master-data related mutex on my todo. I'll try to review this later if I get around, but definitely Acked! -Daniel
Hi On Wed, Mar 26, 2014 at 9:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: >> - Why don't add a spin-lock to "drm_file" instead? Use that one to >> manage master contexts, but keep "struct_mutex" whenever calling into >> driver callbacks (set_master/drop_master) > > See above. We can't have a lock in the drm_file structure since it > protects drm_minor data. Also, while it might be possible to restructure > some code to be able to use spinlocks instead of mutexes I see no reason > to. The established locking order now is master_mutex -> ttm_lock -> > struct_mutex which means master_mutex must be a mutex. Thanks, that order really helps understanding what these locks do. More, actually, than the commit message ;) It also shows how awful struct_mutex is.. Using it as data-protection and execution-sync is really weird. So I'm all for doing more fine-grained locking if it's as simple as with the master-stuff. >> - why is master_mutex per device and not per-minor? I thought masters >> on minors are _entirely_ independent?How do multiple keysyms > > Because currently there is only one master capable minor per device, so > it would be equivalent. And even if there were more, there is no reason > to expect any contention and thus a single master_mutex would be fine. Fair enough. >>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c >>> index e6cdd0f..dad571f 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_legacy_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); >> What's that struct_mutex doing here? We're in ->open(), there is >> no-one racing against us. > > Well, it was held at that point before, and the purpose of this patch is > not to generally clean up struct_mutex usage. Well, it now looks like this: mutex_lock(&dev->struct_mutex); priv->authenticated = 1; mutex_unlock(&dev->struct_mutex); Which looks so _wrong_ that I thought we should fix it right away. But ok, I'm not going to force you to do so. Quick lock-review: * drm_authmagic() uses drm_global_mutex to protect setting priv->authenticated (racing against us..) * current context is ->open() so no-one has access to "priv". We haven't even linked it to dev->filelist, but we called into the driver which might have done anything.. * drm-fops read ->authenticated _unlocked_, so no reason at all to do an explicit locking around a _single_ write (which is only needed in very rare cases, anyway) * it is never set to anything else but 1; a _single_ barrier after setting it should be enough In case you don't want to incorporate that, I will send a cleanup. Would be nice to have the mutex-locking in drm-next to get some testing. v2 looks good, I haven't done a thorough locking review, though. But I guess you did, so feel free to include it in your pull. Thanks David
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index e6cdd0f..dad571f 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_legacy_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 1521036..2b387b0 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; /*@} */ /** \name Usage Counters */