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

[Thomas Hellstrom <thellstrom@vmware.com>]

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