[PATCH 00/16] vmwgfx render-node support v2

[PATCH 00/16] vmwgfx render-node support v2

[Thomas Hellstrom]

Resending only those patches that were commented on (4, 6) and one that
was affected by a change in 4 (12).

If there are no objections, I'll include this in a vmwgfx-next pull request.

Thanks,
Thomas

[PATCH 04/16] drm: Improve on minor type helpers v3

[Thomas Hellstrom]

Add a drm_is_legacy() helper, constify argument to drm_is_render_client(),
and use / change helpers where appropriate.

v2: s/drm_is_legacy/drm_is_legacy_client/ and adapt to new code context.
v3: s/legacy_client/primary_client/

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
---
 drivers/gpu/drm/drm_crtc.c |    4 ++--
 drivers/gpu/drm/drm_fops.c |    6 ++----
 include/drm/drmP.h         |    7 ++++++-
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5fb02d5..960ca98 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1492,7 +1492,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	mutex_unlock(&file_priv->fbs_lock);
 
 	drm_modeset_lock_all(dev);
-	if (file_priv->minor->type != DRM_MINOR_LEGACY) {
+	if (!drm_is_primary_client(file_priv)) {
 
 		mode_group = NULL;
 		list_for_each(lh, &dev->mode_config.crtc_list)
@@ -2848,7 +2848,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->pitch = fb->pitches[0];
 	if (fb->funcs->create_handle) {
 		if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
-		    file_priv->minor->type == DRM_MINOR_CONTROL) {
+		    drm_is_control_client(file_priv)) {
 			ret = fb->funcs->create_handle(fb, file_priv,
 						       &r->handle);
 		} else {
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 5432a1a..c7792b1 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -232,8 +232,7 @@ 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);
-	if (!priv->minor->master && !drm_is_render_client(priv) &&
-	    !drm_is_control_client(priv)) {
+	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) {
@@ -271,8 +270,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 				goto out_close;
 			}
 		}
-	} else if (!drm_is_render_client(priv) &&
-		   !drm_is_control_client(priv)) {
+	} else if (drm_is_primary_client(priv)) {
 		/* get a reference to the master */
 		priv->master = drm_master_get(priv->minor->master);
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f708aa19..7d3a7ce 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1232,7 +1232,7 @@ static inline bool drm_modeset_is_locked(struct drm_device *dev)
 	return mutex_is_locked(&dev->mode_config.mutex);
 }
 
-static inline bool drm_is_render_client(struct drm_file *file_priv)
+static inline bool drm_is_render_client(const struct drm_file *file_priv)
 {
 	return file_priv->minor->type == DRM_MINOR_RENDER;
 }
@@ -1242,6 +1242,11 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv)
 	return file_priv->minor->type == DRM_MINOR_CONTROL;
 }
 
+static inline bool drm_is_primary_client(const struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_LEGACY;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
-- 
1.7.10.4

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

[Thomas Hellstrom]

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);
 		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);
 				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 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

[PATCH 12/16] drm/vmwgfx: Tighten security around surface sharing v2

[Thomas Hellstrom]

If using legacy (non-prime) surface sharing, only allow surfaces
to be shared between clients with the same master. This will block
malicious clients from peeking at contents at surfaces from other
(possibly vt-switched) masters.

v2:
s/legacy_client/primary_client/

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index d50cd76..4ecdbf3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -36,11 +36,13 @@
  * @base:           The TTM base object handling user-space visibility.
  * @srf:            The surface metadata.
  * @size:           TTM accounting size for the surface.
+ * @master:         master of the creating client. Used for security check.
  */
 struct vmw_user_surface {
 	struct ttm_prime_object prime;
 	struct vmw_surface srf;
 	uint32_t size;
+	struct drm_master *master;
 };
 
 /**
@@ -624,6 +626,8 @@ static void vmw_user_surface_free(struct vmw_resource *res)
 	struct vmw_private *dev_priv = srf->res.dev_priv;
 	uint32_t size = user_srf->size;
 
+	if (user_srf->master)
+		drm_master_put(&user_srf->master);
 	kfree(srf->offsets);
 	kfree(srf->sizes);
 	kfree(srf->snooper.image);
@@ -819,6 +823,8 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
 
 	user_srf->prime.base.shareable = false;
 	user_srf->prime.base.tfile = NULL;
+	if (drm_is_primary_client(file_priv))
+		user_srf->master = drm_master_get(file_priv->master);
 
 	/**
 	 * From this point, the generic resource management functions
@@ -885,6 +891,7 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv,
 			     struct ttm_base_object **base_p)
 {
 	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+	struct vmw_user_surface *user_srf;
 	uint32_t handle;
 	struct ttm_base_object *base;
 	int ret;
@@ -915,6 +922,21 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv,
 	}
 
 	if (handle_type != DRM_VMW_HANDLE_PRIME) {
+		user_srf = container_of(base, struct vmw_user_surface,
+					prime.base);
+
+		/*
+		 * Make sure the surface creator has the same
+		 * authenticating master.
+		 */
+		if (drm_is_primary_client(file_priv) &&
+		    user_srf->master != file_priv->master) {
+			DRM_ERROR("Trying to reference surface outside of"
+				  " master domain.\n");
+			ret = -EACCES;
+			goto out_bad_resource;
+		}
+
 		ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL);
 		if (unlikely(ret != 0)) {
 			DRM_ERROR("Could not add a reference to a surface.\n");
@@ -1273,6 +1295,8 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,
 
 	user_srf->prime.base.shareable = false;
 	user_srf->prime.base.tfile = NULL;
+	if (drm_is_primary_client(file_priv))
+		user_srf->master = drm_master_get(file_priv->master);
 
 	/**
 	 * From this point, the generic resource management functions
-- 
1.7.10.4

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

[David Herrmann]

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
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Hellstrom]

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

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

[Thomas Hellstrom]

Hi, again!

I've looked through the code again and have some answers to your
questions. Will post an updated patch soon.

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.

No. The locking here is, as you say, unneeded. drm_minor::master is
protected by master_mutex.
I'll remove, and while doing that, I'll also remove the unneeded locking
around
priv->authenticated = 1.


>>                 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.

It's unneeded, so this should be OK. As for drm_file::master, it should
be considered immutable, except for
drm_file open() and release() where nobody is racing us.

>
> 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.

Yes.

>
>>                         if (ret) {
>>                                 /* drop both references if this fails */
>>                                 drm_master_put(&priv->minor->master);
>>                                 drm_master_put(&priv->master);
> same as above

No change needed.

>
>> -                               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..

Will fix.

>
>>         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.

Will fix.

>
>> +       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..

Protected by master_mutex.


>
>>         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.

Protected by master_mutex.

>
> Thanks
> David
>
>

Thanks,
Thomas

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

[Thomas Hellstrom]

On 03/28/2014 01:19 AM, David Herrmann wrote:
> Hi
>
>
>
> 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..

Before, struct_mutex was required outside of drm_master_put(), because
the argument to drm_master_put was
also protected by struct_mutex. Now that's no longer the case and I
chose to move the locking into the destructor, avoiding a
number of unnecessary struct_mutex locks (the master destructor is
called more seldom than master_put()).
Also in general I believe one should avoid locking around unreferencing
if possible, because it leads to ugly constructs (and even static code
analyzers complaining) if the lock has to be temporarily released in the
destructor to avoid locking order violations.
But in the end I guess that's really a matter of taste.

/Thomas