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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

On Tue, Mar 25, 2014 at 2:18 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.

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux