From patchwork Fri Mar 28 09:34:42 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thomas Hellstrom X-Patchwork-Id: 3901621 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id A2304BF540 for ; Fri, 28 Mar 2014 09:34:59 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 81ECC20295 for ; Fri, 28 Mar 2014 09:34:58 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 33AC72026F for ; Fri, 28 Mar 2014 09:34:57 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C96B56EC23; Fri, 28 Mar 2014 02:34:55 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from smtp-outbound-1.vmware.com (smtp-outbound-1.vmware.com [208.91.2.12]) by gabe.freedesktop.org (Postfix) with ESMTP id B69E56EC23 for ; Fri, 28 Mar 2014 02:34:54 -0700 (PDT) Received: from sc9-mailhost2.vmware.com (sc9-mailhost2.vmware.com [10.113.161.72]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 136AB2846C; Fri, 28 Mar 2014 02:34:54 -0700 (PDT) Received: from zcs-prod-mta-2.vmware.com (zcs-prod-mta-2.vmware.com [10.113.163.64]) by sc9-mailhost2.vmware.com (Postfix) with ESMTP id 0A290B092D; Fri, 28 Mar 2014 02:34:54 -0700 (PDT) Received: from zcs-prod-mta-2.vmware.com (localhost.localdomain [127.0.0.1]) by zcs-prod-mta-2.vmware.com (Postfix) with ESMTP id 007C8E004B; Fri, 28 Mar 2014 02:34:53 -0700 (PDT) Received: from ubuntu.localdomain (unknown [10.113.160.14]) by zcs-prod-mta-2.vmware.com (Postfix) with ESMTPSA; Fri, 28 Mar 2014 02:34:52 -0700 (PDT) From: Thomas Hellstrom To: dri-devel@lists.freedesktop.org Subject: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v3 Date: Fri, 28 Mar 2014 10:34:42 +0100 Message-Id: <1395999282-12553-1-git-send-email-thellstrom@vmware.com> X-Mailer: git-send-email 1.7.10.4 Cc: Thomas Hellstrom X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. Locking order is master_mutex -> struct_mutex. Also remove drm_master::blocked since it's not used. v2: Add an inline comment about what drm_device::master_mutex is protecting. v3: Remove unneeded struct_mutex locks. Fix error returns in drm_setmaster_ioctl(). Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul Reviewed-by: David Herrmann --- drivers/gpu/drm/drm_fops.c | 22 ++++++++++----------- drivers/gpu/drm/drm_stub.c | 43 ++++++++++++++++++++++++++--------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 64 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1..a0ce39c 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -231,12 +231,11 @@ 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 */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { - mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; goto out_close; } @@ -244,29 +243,23 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->is_master = 1; /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master); - 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); 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,7 +267,7 @@ 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); @@ -302,6 +295,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 +483,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 +508,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 +518,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..8f65045 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,20 @@ 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; - - if (!file_priv->master) - return -EINVAL; + if (file_priv->minor->master) { + ret = -EINVAL; + goto out_unlock; + } - if (file_priv->minor->master) - return -EINVAL; + if (!file_priv->master) { + ret = -EINVAL; + goto out_unlock; + } - 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 +219,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 +576,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 +630,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 +652,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 -/* 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 */