Re: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2

[Thomas Hellstrom <thellstrom@vmware.com>]

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