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

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