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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Reviewed-by: Brian Paul <brianp@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
