[PATCH 00/16] vmwgfx render-node support

[PATCH 00/16] vmwgfx render-node support

[Thomas Hellstrom]

Hi!

Some of the patches are already reviewed on dri-devel, but included for
completeness.

Patch 6 and 7 are drm patches and in particular patch 6 might be good to
have an extra pair of eyes on.

Thanks,
Thomas

[PATCH 01/16] drm: Have the crtc code only reference master from legacy nodes v2

[Thomas Hellstrom]

control- and render nodes are intended to be master-less.

v2: Replace tests for !legacy with tests for !mode_group for readability.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 16ca28e..5fb02d5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1492,9 +1492,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	mutex_unlock(&file_priv->fbs_lock);
 
 	drm_modeset_lock_all(dev);
-	mode_group = &file_priv->master->minor->mode_group;
-	if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+	if (file_priv->minor->type != DRM_MINOR_LEGACY) {
 
+		mode_group = NULL;
 		list_for_each(lh, &dev->mode_config.crtc_list)
 			crtc_count++;
 
@@ -1505,6 +1505,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 			encoder_count++;
 	} else {
 
+		mode_group = &file_priv->master->minor->mode_group;
 		crtc_count = mode_group->num_crtcs;
 		connector_count = mode_group->num_connectors;
 		encoder_count = mode_group->num_encoders;
@@ -1519,7 +1520,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_crtcs >= crtc_count) {
 		copied = 0;
 		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (!mode_group) {
 			list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 					    head) {
 				DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
@@ -1546,7 +1547,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_encoders >= encoder_count) {
 		copied = 0;
 		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (!mode_group) {
 			list_for_each_entry(encoder,
 					    &dev->mode_config.encoder_list,
 					    head) {
@@ -1577,7 +1578,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_connectors >= connector_count) {
 		copied = 0;
 		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (!mode_group) {
 			list_for_each_entry(connector,
 					    &dev->mode_config.connector_list,
 					    head) {
@@ -2846,7 +2847,8 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->bpp = fb->bits_per_pixel;
 	r->pitch = fb->pitches[0];
 	if (fb->funcs->create_handle) {
-		if (file_priv->is_master || capable(CAP_SYS_ADMIN)) {
+		if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
+		    file_priv->minor->type == DRM_MINOR_CONTROL) {
 			ret = fb->funcs->create_handle(fb, file_priv,
 						       &r->handle);
 		} else {
-- 
1.7.10.4

[PATCH 02/16] drm: Break out ioctl permission check to a separate function v2

[Thomas Hellstrom]

Helps reviewing and understanding these checks.
v2: Remove misplaced newlines.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_drv.c |  113 ++++++++++++++++++++++++++++++---------------
 1 file changed, 75 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index ec651be..05e3053 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -286,6 +286,44 @@ static int drm_version(struct drm_device *dev, void *data,
 }
 
 /**
+ * drm_ioctl_permit - Check ioctl permissions against caller
+ *
+ * @flags: ioctl permission flags.
+ * @file_priv: Pointer to struct drm_file identifying the caller.
+ *
+ * Checks whether the caller is allowed to run an ioctl with the
+ * indicated permissions. If so, returns zero. Otherwise returns an
+ * error code suitable for ioctl return.
+ */
+static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
+{
+	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
+	if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
+		return -EACCES;
+
+	/* AUTH is only for authenticated or render client */
+	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
+		     !file_priv->authenticated))
+		return -EACCES;
+
+	/* MASTER is only for master */
+	if (unlikely((flags & DRM_MASTER) && !file_priv->is_master))
+		return -EACCES;
+
+	/* Control clients must be explicitly allowed */
+	if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
+		     file_priv->minor->type == DRM_MINOR_CONTROL))
+		return -EACCES;
+
+	/* Render clients must be explicitly allowed */
+	if (unlikely(!(flags & DRM_RENDER_ALLOW) &&
+		     drm_is_render_client(file_priv)))
+		return -EACCES;
+
+	return 0;
+}
+
+/**
  * Called whenever a process performs an ioctl on /dev/drm.
  *
  * \param inode device inode.
@@ -350,52 +388,51 @@ long drm_ioctl(struct file *filp,
 	/* Do not trust userspace, use our own definition */
 	func = ioctl->func;
 
-	if (!func) {
+	if (unlikely(!func)) {
 		DRM_DEBUG("no function\n");
 		retcode = -EINVAL;
-	} else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
-		   ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) ||
-		   ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
-		   (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) ||
-		   (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
-		retcode = -EACCES;
-	} else {
-		if (cmd & (IOC_IN | IOC_OUT)) {
-			if (asize <= sizeof(stack_kdata)) {
-				kdata = stack_kdata;
-			} else {
-				kdata = kmalloc(asize, GFP_KERNEL);
-				if (!kdata) {
-					retcode = -ENOMEM;
-					goto err_i1;
-				}
-			}
-			if (asize > usize)
-				memset(kdata + usize, 0, asize - usize);
-		}
+		goto err_i1;
+	}
 
-		if (cmd & IOC_IN) {
-			if (copy_from_user(kdata, (void __user *)arg,
-					   usize) != 0) {
-				retcode = -EFAULT;
+	retcode = drm_ioctl_permit(ioctl->flags, file_priv);
+	if (unlikely(retcode))
+		goto err_i1;
+
+	if (cmd & (IOC_IN | IOC_OUT)) {
+		if (asize <= sizeof(stack_kdata)) {
+			kdata = stack_kdata;
+		} else {
+			kdata = kmalloc(asize, GFP_KERNEL);
+			if (!kdata) {
+				retcode = -ENOMEM;
 				goto err_i1;
 			}
-		} else
-			memset(kdata, 0, usize);
-
-		if (ioctl->flags & DRM_UNLOCKED)
-			retcode = func(dev, kdata, file_priv);
-		else {
-			mutex_lock(&drm_global_mutex);
-			retcode = func(dev, kdata, file_priv);
-			mutex_unlock(&drm_global_mutex);
 		}
+		if (asize > usize)
+			memset(kdata + usize, 0, asize - usize);
+	}
 
-		if (cmd & IOC_OUT) {
-			if (copy_to_user((void __user *)arg, kdata,
-					 usize) != 0)
-				retcode = -EFAULT;
+	if (cmd & IOC_IN) {
+		if (copy_from_user(kdata, (void __user *)arg,
+				   usize) != 0) {
+			retcode = -EFAULT;
+			goto err_i1;
 		}
+	} else
+		memset(kdata, 0, usize);
+
+	if (ioctl->flags & DRM_UNLOCKED)
+		retcode = func(dev, kdata, file_priv);
+	else {
+		mutex_lock(&drm_global_mutex);
+		retcode = func(dev, kdata, file_priv);
+		mutex_unlock(&drm_global_mutex);
+	}
+
+	if (cmd & IOC_OUT) {
+		if (copy_to_user((void __user *)arg, kdata,
+				 usize) != 0)
+			retcode = -EFAULT;
 	}
 
       err_i1:
-- 
1.7.10.4

[PATCH 03/16] drm: Make control nodes master-less v3

[Thomas Hellstrom]

Like for render-nodes, there is no point in maintaining the master concept
for control nodes, so set the struct drm_file::master pointer to NULL.

At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always
allowed when called through the control node. Previously the caller also
needed to be master.

v2: Adapt to refactoring of ioctl permission check.
v3: Formatting of logical expression. Use drm_is_control_client() instead of
    drm_is_control().

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 05e3053..cf2dfb7 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -306,13 +306,14 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 		     !file_priv->authenticated))
 		return -EACCES;
 
-	/* MASTER is only for master */
-	if (unlikely((flags & DRM_MASTER) && !file_priv->is_master))
+	/* MASTER is only for master or control clients */
+	if (unlikely((flags & DRM_MASTER) && !file_priv->is_master &&
+		     !drm_is_control_client(file_priv)))
 		return -EACCES;
 
 	/* Control clients must be explicitly allowed */
 	if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
-		     file_priv->minor->type == DRM_MINOR_CONTROL))
+		     drm_is_control_client(file_priv)))
 		return -EACCES;
 
 	/* Render clients must be explicitly allowed */
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 9b02f12..5432a1a 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -232,7 +232,8 @@ 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)) {
+	if (!priv->minor->master && !drm_is_render_client(priv) &&
+	    !drm_is_control_client(priv)) {
 		/* create a new master */
 		priv->minor->master = drm_master_create(priv->minor);
 		if (!priv->minor->master) {
@@ -270,7 +271,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 				goto out_close;
 			}
 		}
-	} else if (!drm_is_render_client(priv)) {
+	} else if (!drm_is_render_client(priv) &&
+		   !drm_is_control_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 daac00a..f708aa19 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1237,6 +1237,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv)
 	return file_priv->minor->type == DRM_MINOR_RENDER;
 }
 
+static inline bool drm_is_control_client(const struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_CONTROL;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
-- 
1.7.10.4

Re: [PATCH 03/16] drm: Make control nodes master-less v3

[David Herrmann]

Hi

On Tue, Mar 25, 2014 at 2:18 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> Like for render-nodes, there is no point in maintaining the master concept
> for control nodes, so set the struct drm_file::master pointer to NULL.
>
> At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always
> allowed when called through the control node. Previously the caller also
> needed to be master.
>
> v2: Adapt to refactoring of ioctl permission check.
> v3: Formatting of logical expression. Use drm_is_control_client() instead of
>     drm_is_control().

Looks good now.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Reviewed-by: Brian Paul <brianp@vmware.com>
> ---
>  drivers/gpu/drm/drm_drv.c  |    7 ++++---
>  drivers/gpu/drm/drm_fops.c |    6 ++++--
>  include/drm/drmP.h         |    5 +++++
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05e3053..cf2dfb7 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -306,13 +306,14 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>                      !file_priv->authenticated))
>                 return -EACCES;
>
> -       /* MASTER is only for master */
> -       if (unlikely((flags & DRM_MASTER) && !file_priv->is_master))
> +       /* MASTER is only for master or control clients */
> +       if (unlikely((flags & DRM_MASTER) && !file_priv->is_master &&
> +                    !drm_is_control_client(file_priv)))
>                 return -EACCES;
>
>         /* Control clients must be explicitly allowed */
>         if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
> -                    file_priv->minor->type == DRM_MINOR_CONTROL))
> +                    drm_is_control_client(file_priv)))
>                 return -EACCES;
>
>         /* Render clients must be explicitly allowed */
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 9b02f12..5432a1a 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -232,7 +232,8 @@ 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)) {
> +       if (!priv->minor->master && !drm_is_render_client(priv) &&
> +           !drm_is_control_client(priv)) {
>                 /* create a new master */
>                 priv->minor->master = drm_master_create(priv->minor);
>                 if (!priv->minor->master) {
> @@ -270,7 +271,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>                                 goto out_close;
>                         }
>                 }
> -       } else if (!drm_is_render_client(priv)) {
> +       } else if (!drm_is_render_client(priv) &&
> +                  !drm_is_control_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 daac00a..f708aa19 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1237,6 +1237,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv)
>         return file_priv->minor->type == DRM_MINOR_RENDER;
>  }
>
> +static inline bool drm_is_control_client(const struct drm_file *file_priv)
> +{
> +       return file_priv->minor->type == DRM_MINOR_CONTROL;
> +}
> +
>  /******************************************************************/
>  /** \name Internal function definitions */
>  /*@{*/
> --
> 1.7.10.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

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..bf6ef77 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_legacy_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..e6cdd0f 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_legacy_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_legacy_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..5fe3d68 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_legacy_client(const struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_LEGACY;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
-- 
1.7.10.4

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

[David Herrmann]

Hi

On Tue, Mar 25, 2014 at 2:18 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> 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.

Could we avoid using "legacy" for the primary node? The node-pointer
is called ->primary and Thierry posted patches to also rename
DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY. The primary node is still the
recommended node for mode-setting, so lets not attribute "legacy" to
them,

Otherwise, patch looks good.

Thanks
David

> 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..bf6ef77 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_legacy_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..e6cdd0f 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_legacy_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_legacy_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..5fe3d68 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_legacy_client(const struct drm_file *file_priv)
> +{
> +       return file_priv->minor->type == DRM_MINOR_LEGACY;
> +}
> +
>  /******************************************************************/
>  /** \name Internal function definitions */
>  /*@{*/
> --
> 1.7.10.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/16] drm: Remove the minor master list

[Thomas Hellstrom]

It doesn't appear to be used anywhere.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_stub.c |    5 -----
 include/drm/drmP.h         |    2 --
 2 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index dc2c609..d344513 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -135,8 +135,6 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
 	INIT_LIST_HEAD(&master->magicfree);
 	master->minor = minor;
 
-	list_add_tail(&master->head, &minor->master_list);
-
 	return master;
 }
 
@@ -154,8 +152,6 @@ static void drm_master_destroy(struct kref *kref)
 	struct drm_device *dev = master->minor->dev;
 	struct drm_map_list *r_list, *list_temp;
 
-	list_del(&master->head);
-
 	if (dev->driver->master_destroy)
 		dev->driver->master_destroy(dev, master);
 
@@ -281,7 +277,6 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 
 	minor->type = type;
 	minor->dev = dev;
-	INIT_LIST_HEAD(&minor->master_list);
 
 	*drm_minor_get_slot(dev, type) = minor;
 	return 0;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 5fe3d68..1521036 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -719,7 +719,6 @@ struct drm_master {
 
 	struct kref refcount; /* refcount for this master */
 
-	struct list_head head; /**< each minor contains a list of masters */
 	struct drm_minor *minor; /**< link back to minor we are a master for */
 
 	char *unique;			/**< Unique identifier: e.g., busid */
@@ -1052,7 +1051,6 @@ struct drm_minor {
 	struct mutex debugfs_lock; /* Protects debugfs_list. */
 
 	struct drm_master *master; /* currently active master for this node */
-	struct list_head master_list;
 	struct drm_mode_group mode_group;
 };
 
-- 
1.7.10.4

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

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

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);
 		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? */
-
-	/** \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;
 	/*@} */
 
 	/** \name Usage Counters */
-- 
1.7.10.4

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

[David Herrmann]

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

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

[Thomas Hellstrom]

On 03/26/2014 08:08 PM, David Herrmann wrote:
> Hi
>
> On Tue, Mar 25, 2014 at 2:18 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> The master management was previously protected by the drm_device::struct_mutex.
>> In order to avoid locking order violations in a reworked dropped master
>> security check in the vmwgfx driver, break it out into a separate master_mutex.
> Could you elaborate on that? What exactly is "master_mutex"
> protecting? 

Its protecting drm_file::is_master and drm_minor::master, as per inline
comments. It's also a candidate for replacing
drm_global_mutex to avoid the dropmaster and setmaster ioctls racing
when the drm_global_mutex eventually goes away.

> "struct_mutex" is used to serialize all entry-points into
> the drm-device (and thus the driver) and also, often implicitly, as
> spin-lock for "struct drm_device" data protection.

No. DRM locking was added as an after-though, and is a horrendous mess.
Nobody really knows what's protecting what, and that has caused a lot of
grief in the past. Probably most so for the Intel driver that relied
(relies?) on the struct_mutex to protect everything. The
drm_global_mutex is used to serialize the non-lock-audited entry points
into the drm device. The struct_mutex is used for data protection of
most core drm structures and serializing here and there. Modern drivers
have no locks held when entering their ioctls. Also we should not
confuse mutexes and spinlocks in this context, as they have very
different semantics.


>
> Regarding master_mutex I have several questions:
>  - Can you give an example how vmwgfx dead-locks with your reworked code?

Sure. The reworked driver takes the ttm lock in non-exclusive mode
before entering an ioctl. Ioctls will then internally take the
struct_mutex. The dropmaster and setmaster ioctls would (before this
patch) take the struct_mutex and then in the driver code takes the ttm
lock in exclusive mode. We would have a lock inversion and a potential
deadlock.

>  - Why don't add a spin-lock to "drm_file" instead? Use that one to
> manage master contexts, but keep "struct_mutex" whenever calling into
> driver callbacks (set_master/drop_master)

See above. We can't have a lock in the drm_file structure since it
protects drm_minor data. Also, while it might be possible to restructure
some code to be able to use spinlocks instead of mutexes I see no reason
to. The established locking order now is master_mutex -> ttm_lock ->
struct_mutex which means master_mutex must be a mutex.

>  - why is master_mutex per device and not per-minor? I thought masters
> on minors are _entirely_ independent?

Because currently there is only one master capable minor per device, so
it would be equivalent. And even if there were more, there is no reason
to expect any contention and thus a single master_mutex would be fine.

>
> Few more comments inline.
>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> Reviewed-by: Brian Paul <brianp@vmware.com>
>> ---
>>  drivers/gpu/drm/drm_fops.c |   23 +++++++++++++---------
>>  drivers/gpu/drm/drm_stub.c |   38 ++++++++++++++++++++++--------------
>>  include/drm/drmP.h         |   46 ++++++++++++++++++++++++--------------------
>>  3 files changed, 63 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> index e6cdd0f..dad571f 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -231,12 +231,13 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>
>>         /* if there is no current master make this fd it, but do not create
>>          * any master object for render clients */
>> -       mutex_lock(&dev->struct_mutex);
>> +       mutex_lock(&dev->master_mutex);
>>         if (drm_is_legacy_client(priv) && !priv->minor->master) {
>>                 /* create a new master */
>> +               mutex_lock(&dev->struct_mutex);
>>                 priv->minor->master = drm_master_create(priv->minor);
>> +               mutex_unlock(&dev->struct_mutex);
>>                 if (!priv->minor->master) {
>> -                       mutex_unlock(&dev->struct_mutex);
>>                         ret = -ENOMEM;
>>                         goto out_close;
>>                 }
>> @@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>                 /* take another reference for the copy in the local file priv */
>>                 priv->master = drm_master_get(priv->minor->master);
>>
>> +               mutex_lock(&dev->struct_mutex);
>>                 priv->authenticated = 1;
>>
>>                 mutex_unlock(&dev->struct_mutex);
> What's that struct_mutex doing here? We're in ->open(), there is
> no-one racing against us.

Well, it was held at that point before, and the purpose of this patch is
not to generally clean up struct_mutex usage.

>
>>                 if (dev->driver->master_create) {
>>                         ret = dev->driver->master_create(dev, priv->master);
>>                         if (ret) {
>> -                               mutex_lock(&dev->struct_mutex);
>>                                 /* drop both references if this fails */
>>                                 drm_master_put(&priv->minor->master);
>>                                 drm_master_put(&priv->master);
>> -                               mutex_unlock(&dev->struct_mutex);
>>                                 goto out_close;
>>                         }
>>                 }
>> -               mutex_lock(&dev->struct_mutex);
>>                 if (dev->driver->master_set) {
>>                         ret = dev->driver->master_set(dev, priv, true);
>>                         if (ret) {
>>                                 /* drop both references if this fails */
>>                                 drm_master_put(&priv->minor->master);
>>                                 drm_master_put(&priv->master);
>> -                               mutex_unlock(&dev->struct_mutex);
>>                                 goto out_close;
>>                         }
>>                 }
>> @@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>                 /* get a reference to the master */
>>                 priv->master = drm_master_get(priv->minor->master);
>>         }
>> -       mutex_unlock(&dev->struct_mutex);
>>
>> +       mutex_unlock(&dev->master_mutex);
>>         mutex_lock(&dev->struct_mutex);
>>         list_add(&priv->lhead, &dev->filelist);
>>         mutex_unlock(&dev->struct_mutex);
>> @@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>         return 0;
>>
>>  out_close:
>> +       mutex_unlock(&dev->master_mutex);
>>         if (dev->driver->postclose)
>>                 dev->driver->postclose(dev, priv);
>>  out_prime_destroy:
>> @@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp)
>>         }
>>         mutex_unlock(&dev->ctxlist_mutex);
>>
>> -       mutex_lock(&dev->struct_mutex);
>> +       mutex_lock(&dev->master_mutex);
>>
>>         if (file_priv->is_master) {
>>                 struct drm_master *master = file_priv->master;
>>                 struct drm_file *temp;
>> +
>> +               mutex_lock(&dev->struct_mutex);
>>                 list_for_each_entry(temp, &dev->filelist, lhead) {
>>                         if ((temp->master == file_priv->master) &&
>>                             (temp != file_priv))
>> @@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp)
>>                         master->lock.file_priv = NULL;
>>                         wake_up_interruptible_all(&master->lock.lock_queue);
>>                 }
>> +               mutex_unlock(&dev->struct_mutex);
>>
>>                 if (file_priv->minor->master == file_priv->master) {
>>                         /* drop the reference held my the minor */
>> @@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp)
>>                 }
>>         }
>>
>> -       /* drop the reference held my the file priv */
>> +       /* drop the master reference held by the file priv */
>>         if (file_priv->master)
>>                 drm_master_put(&file_priv->master);
>>         file_priv->is_master = 0;
>> +       mutex_unlock(&dev->master_mutex);
>> +
>> +       mutex_lock(&dev->struct_mutex);
>>         list_del(&file_priv->lhead);
>>         mutex_unlock(&dev->struct_mutex);
>>
>> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
>> index d344513..10c8303 100644
>> --- a/drivers/gpu/drm/drm_stub.c
>> +++ b/drivers/gpu/drm/drm_stub.c
>> @@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref)
>>         struct drm_device *dev = master->minor->dev;
>>         struct drm_map_list *r_list, *list_temp;
>>
>> +       mutex_lock(&dev->struct_mutex);
>>         if (dev->driver->master_destroy)
>>                 dev->driver->master_destroy(dev, master);
>>
>> @@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)
>>
>>         drm_ht_remove(&master->magiclist);
>>
>> +       mutex_unlock(&dev->struct_mutex);
>>         kfree(master);
>>  }
>>
>> @@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>>  {
>>         int ret = 0;
>>
>> +       mutex_lock(&dev->master_mutex);
>>         if (file_priv->is_master)
>> -               return 0;
>> +               goto out_unlock;
>>
>> -       if (file_priv->minor->master && file_priv->minor->master != file_priv->master)
>> -               return -EINVAL;
>> +       ret = -EINVAL;
>> +       if (file_priv->minor->master)
>> +               goto out_unlock;
>>
>>         if (!file_priv->master)
>> -               return -EINVAL;
>> +               goto out_unlock;
>>
>> -       if (file_priv->minor->master)
>> -               return -EINVAL;
>> -
>> -       mutex_lock(&dev->struct_mutex);
>>         file_priv->minor->master = drm_master_get(file_priv->master);
>>         file_priv->is_master = 1;
>>         if (dev->driver->master_set) {
>> @@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>>                         drm_master_put(&file_priv->minor->master);
>>                 }
>>         }
>> -       mutex_unlock(&dev->struct_mutex);
>>
>> +out_unlock:
>> +       mutex_unlock(&dev->master_mutex);
>>         return ret;
>>  }
>>
>>  int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
>>                          struct drm_file *file_priv)
>>  {
>> +       int ret = -EINVAL;
>> +
>> +       mutex_lock(&dev->master_mutex);
>>         if (!file_priv->is_master)
>> -               return -EINVAL;
>> +               goto out_unlock;
>>
>>         if (!file_priv->minor->master)
>> -               return -EINVAL;
>> +               goto out_unlock;
>>
>> -       mutex_lock(&dev->struct_mutex);
>> +       ret = 0;
>>         if (dev->driver->master_drop)
>>                 dev->driver->master_drop(dev, file_priv, false);
>>         drm_master_put(&file_priv->minor->master);
>>         file_priv->is_master = 0;
>> -       mutex_unlock(&dev->struct_mutex);
>> -       return 0;
>> +
>> +out_unlock:
>> +       mutex_unlock(&dev->master_mutex);
>> +       return ret;
>>  }
>>
>>  /*
>> @@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>>         spin_lock_init(&dev->event_lock);
>>         mutex_init(&dev->struct_mutex);
>>         mutex_init(&dev->ctxlist_mutex);
>> +       mutex_init(&dev->master_mutex);
>>
>>         dev->anon_inode = drm_fs_inode_new();
>>         if (IS_ERR(dev->anon_inode)) {
>> @@ -620,6 +627,7 @@ err_minors:
>>         drm_minor_free(dev, DRM_MINOR_CONTROL);
>>         drm_fs_inode_free(dev->anon_inode);
>>  err_free:
>> +       mutex_destroy(&dev->master_mutex);
>>         kfree(dev);
>>         return NULL;
>>  }
>> @@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref)
>>         drm_minor_free(dev, DRM_MINOR_CONTROL);
>>
>>         kfree(dev->devname);
>> +
>> +       mutex_destroy(&dev->master_mutex);
>>         kfree(dev);
>>  }
>>
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index 1521036..2b387b0 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -435,7 +435,8 @@ struct drm_prime_file_private {
>>  struct drm_file {
>>         unsigned always_authenticated :1;
>>         unsigned authenticated :1;
>> -       unsigned is_master :1; /* this file private is a master for a minor */
>> +       /* Whether we're master for a minor. Protected by master_mutex */
>> +       unsigned is_master :1;
>>         /* true when the client has asked us to expose stereo 3D mode flags */
>>         unsigned stereo_allowed :1;
>>
>> @@ -714,28 +715,29 @@ struct drm_gem_object {
>>
>>  #include <drm/drm_crtc.h>
>>
>> -/* per-master structure */
>> +/**
>> + * struct drm_master - drm master structure
>> + *
>> + * @refcount: Refcount for this master object.
>> + * @minor: Link back to minor char device we are master for. Immutable.
>> + * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
>> + * @unique_len: Length of unique field. Protected by drm_global_mutex.
>> + * @unique_size: Amount allocated. Protected by drm_global_mutex.
>> + * @magiclist: Hash of used authentication tokens. Protected by struct_mutex.
>> + * @magicfree: List of used authentication tokens. Protected by struct_mutex.
>> + * @lock: DRI lock information.
>> + * @driver_priv: Pointer to driver-private information.
>> + */
>>  struct drm_master {
>> -
>> -       struct kref refcount; /* refcount for this master */
>> -
>> -       struct drm_minor *minor; /**< link back to minor we are a master for */
>> -
>> -       char *unique;                   /**< Unique identifier: e.g., busid */
>> -       int unique_len;                 /**< Length of unique field */
>> -       int unique_size;                /**< amount allocated */
>> -
>> -       int blocked;                    /**< Blocked due to VC switch? */
> You silently dropped that field. At least mention it in the
> commit-message if it's unused.

Sure.

>
>> -
>> -       /** \name Authentication */
>> -       /*@{ */
>> +       struct kref refcount;
>> +       struct drm_minor *minor;
>> +       char *unique;
>> +       int unique_len;
>> +       int unique_size;
>>         struct drm_open_hash magiclist;
>>         struct list_head magicfree;
>> -       /*@} */
>> -
>> -       struct drm_lock_data lock;      /**< Information on hardware lock */
>> -
>> -       void *driver_priv; /**< Private structure for driver to use */
>> +       struct drm_lock_data lock;
>> +       void *driver_priv;
>>  };
>>
>>  /* Size of ringbuffer for vblank timestamps. Just double-buffer
>> @@ -1050,7 +1052,8 @@ struct drm_minor {
>>         struct list_head debugfs_list;
>>         struct mutex debugfs_lock; /* Protects debugfs_list. */
>>
>> -       struct drm_master *master; /* currently active master for this node */
>> +       /* currently active master for this node. Protected by master_mutex */
>> +       struct drm_master *master;
>>         struct drm_mode_group mode_group;
>>  };
>>
>> @@ -1100,6 +1103,7 @@ struct drm_device {
>>         /*@{ */
>>         spinlock_t count_lock;          /**< For inuse, drm_device::open_count, drm_device::buf_use */
>>         struct mutex struct_mutex;      /**< For others */
>> +       struct mutex master_mutex;
> Comments, comments, comments! Lets avoid adding another undocumented
> mutex here. Or at least mark it as private to drm_master_*()
> functions.

Sure.

>
> Thanks
> David
>
>>         /*@} */
>>
>>         /** \name Usage Counters */
>> --
>> 1.7.10.4
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/dri-devel&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=aQtwaWHv2a0%2B862ao5fkuBGGYVf%2B4WRqrDEgHLSsyzI%3D%0A&s=f27e1a390d6150945a05ccdde58cdd68df313e1b8b255911c3380b60424bba98
Thanks,

Thomas

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

[Daniel Vetter]

On Wed, Mar 26, 2014 at 09:40:18PM +0100, Thomas Hellstrom wrote:
> On 03/26/2014 08:08 PM, David Herrmann wrote:
> > "struct_mutex" is used to serialize all entry-points into
> > the drm-device (and thus the driver) and also, often implicitly, as
> > spin-lock for "struct drm_device" data protection.
> 
> No. DRM locking was added as an after-though, and is a horrendous mess.
> Nobody really knows what's protecting what, and that has caused a lot of
> grief in the past. Probably most so for the Intel driver that relied
> (relies?) on the struct_mutex to protect everything. The
> drm_global_mutex is used to serialize the non-lock-audited entry points
> into the drm device. The struct_mutex is used for data protection of
> most core drm structures and serializing here and there. Modern drivers
> have no locks held when entering their ioctls. Also we should not
> confuse mutexes and spinlocks in this context, as they have very
> different semantics.

As the guy who gets to live the locking mess called dev->struct_mutex I
holeheartedly welcome any efforts to split out clear subparts away from
it. I actually had this very idea of adding a master-data related mutex on
my todo. I'll try to review this later if I get around, but definitely
Acked!
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[David Herrmann]

Hi

On Wed, Mar 26, 2014 at 9:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>>  - Why don't add a spin-lock to "drm_file" instead? Use that one to
>> manage master contexts, but keep "struct_mutex" whenever calling into
>> driver callbacks (set_master/drop_master)
>
> See above. We can't have a lock in the drm_file structure since it
> protects drm_minor data. Also, while it might be possible to restructure
> some code to be able to use spinlocks instead of mutexes I see no reason
> to. The established locking order now is master_mutex -> ttm_lock ->
> struct_mutex which means master_mutex must be a mutex.

Thanks, that order really helps understanding what these locks do.
More, actually, than the commit message ;) It also shows how awful
struct_mutex is.. Using it as data-protection and execution-sync is
really weird. So I'm all for doing more fine-grained locking if it's
as simple as with the master-stuff.

>>  - why is master_mutex per device and not per-minor? I thought masters
>> on minors are _entirely_ independent?How do multiple keysyms
>
> Because currently there is only one master capable minor per device, so
> it would be equivalent. And even if there were more, there is no reason
> to expect any contention and thus a single master_mutex would be fine.

Fair enough.

>>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>>> index e6cdd0f..dad571f 100644
>>> --- a/drivers/gpu/drm/drm_fops.c
>>> +++ b/drivers/gpu/drm/drm_fops.c
>>> @@ -231,12 +231,13 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>>
>>>         /* if there is no current master make this fd it, but do not create
>>>          * any master object for render clients */
>>> -       mutex_lock(&dev->struct_mutex);
>>> +       mutex_lock(&dev->master_mutex);
>>>         if (drm_is_legacy_client(priv) && !priv->minor->master) {
>>>                 /* create a new master */
>>> +               mutex_lock(&dev->struct_mutex);
>>>                 priv->minor->master = drm_master_create(priv->minor);
>>> +               mutex_unlock(&dev->struct_mutex);
>>>                 if (!priv->minor->master) {
>>> -                       mutex_unlock(&dev->struct_mutex);
>>>                         ret = -ENOMEM;
>>>                         goto out_close;
>>>                 }
>>> @@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>>                 /* take another reference for the copy in the local file priv */
>>>                 priv->master = drm_master_get(priv->minor->master);
>>>
>>> +               mutex_lock(&dev->struct_mutex);
>>>                 priv->authenticated = 1;
>>>
>>>                 mutex_unlock(&dev->struct_mutex);
>> What's that struct_mutex doing here? We're in ->open(), there is
>> no-one racing against us.
>
> Well, it was held at that point before, and the purpose of this patch is
> not to generally clean up struct_mutex usage.

Well, it now looks like this:

mutex_lock(&dev->struct_mutex);
priv->authenticated = 1;
mutex_unlock(&dev->struct_mutex);

Which looks so _wrong_ that I thought we should fix it right away. But
ok, I'm not going to force you to do so.

Quick lock-review:
* drm_authmagic() uses drm_global_mutex to protect setting
priv->authenticated (racing against us..)
* current context is ->open() so no-one has access to "priv". We
haven't even linked it to dev->filelist, but we called into the driver
which might have done anything..
* drm-fops read ->authenticated _unlocked_, so no reason at all to do
an explicit locking around a _single_ write (which is only needed in
very rare cases, anyway)
* it is never set to anything else but 1; a _single_ barrier after
setting it should be enough

In case you don't want to incorporate that, I will send a cleanup.

Would be nice to have the mutex-locking in drm-next to get some
testing. v2 looks good, I haven't done a thorough locking review,
though. But I guess you did, so feel free to include it in your pull.
Thanks
David

[PATCH 07/16] drm: Add a function to get the ioctl flags

[Thomas Hellstrom]

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
---
 drivers/gpu/drm/drm_drv.c |   18 ++++++++++++++++++
 include/drm/drmP.h        |    1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index cf2dfb7..03711d0 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -450,3 +450,21 @@ long drm_ioctl(struct file *filp,
 	return retcode;
 }
 EXPORT_SYMBOL(drm_ioctl);
+
+/**
+ * drm_ioctl_flags - Check for core ioctl and return ioctl permission flags
+ *
+ * @nr: Ioctl number.
+ * @flags: Where to return the ioctl permission flags
+ */
+bool drm_ioctl_flags(unsigned int nr, unsigned int *flags)
+{
+	if ((nr >= DRM_COMMAND_END && nr < DRM_CORE_IOCTL_COUNT) ||
+	    (nr < DRM_COMMAND_BASE)) {
+		*flags = drm_ioctls[nr].flags;
+		return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(drm_ioctl_flags);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2b387b0..df57ca5 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1259,6 +1259,7 @@ extern long drm_ioctl(struct file *filp,
 extern long drm_compat_ioctl(struct file *filp,
 			     unsigned int cmd, unsigned long arg);
 extern int drm_lastclose(struct drm_device *dev);
+extern bool drm_ioctl_flags(unsigned int nr, unsigned int *flags);
 
 				/* Device support (drm_fops.h) */
 extern struct mutex drm_global_mutex;
-- 
1.7.10.4

[PATCH 08/16] drm/vmwgfx: Use a per-device semaphore for reservation protection

[Thomas Hellstrom]

Don't use a per-master semaphore (ttm lock) for reservation protection, but
rather a per-device semaphore. This is needed since clients connecting using
render nodes aren't master aware.

The ttm lock used should probably be replaced with a reader-write semaphore
once the function down_xx_interruptible() is available.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_context.c  |    5 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c   |   15 ++++++---------
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c      |    6 +++---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h      |    5 +++++
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c  |    5 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c       |    9 ++++-----
 drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c    |   10 ++++------
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c      |   15 ++++++---------
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c |   15 ++++++---------
 drivers/gpu/drm/vmwgfx/vmwgfx_shader.c   |    5 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c  |   14 ++++++--------
 11 files changed, 46 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
index 1e80152..701d520 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
@@ -462,7 +462,6 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data,
 	struct vmw_resource *tmp;
 	struct drm_vmw_context_arg *arg = (struct drm_vmw_context_arg *)data;
 	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	int ret;
 
 
@@ -474,7 +473,7 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data,
 	if (unlikely(vmw_user_context_size == 0))
 		vmw_user_context_size = ttm_round_pot(sizeof(*ctx)) + 128;
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -521,7 +520,7 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data,
 out_err:
 	vmw_resource_unreference(&res);
 out_unlock:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return ret;
 
 }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c
index a758402..70ddce835 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c
@@ -52,11 +52,10 @@ int vmw_dmabuf_to_placement(struct vmw_private *dev_priv,
 			    struct ttm_placement *placement,
 			    bool interruptible)
 {
-	struct vmw_master *vmaster = dev_priv->active_master;
 	struct ttm_buffer_object *bo = &buf->base;
 	int ret;
 
-	ret = ttm_write_lock(&vmaster->lock, interruptible);
+	ret = ttm_write_lock(&dev_priv->reservation_sem, interruptible);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -71,7 +70,7 @@ int vmw_dmabuf_to_placement(struct vmw_private *dev_priv,
 	ttm_bo_unreserve(bo);
 
 err:
-	ttm_write_unlock(&vmaster->lock);
+	ttm_write_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
 
@@ -95,12 +94,11 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv,
 			      struct vmw_dma_buffer *buf,
 			      bool pin, bool interruptible)
 {
-	struct vmw_master *vmaster = dev_priv->active_master;
 	struct ttm_buffer_object *bo = &buf->base;
 	struct ttm_placement *placement;
 	int ret;
 
-	ret = ttm_write_lock(&vmaster->lock, interruptible);
+	ret = ttm_write_lock(&dev_priv->reservation_sem, interruptible);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -143,7 +141,7 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv,
 err_unreserve:
 	ttm_bo_unreserve(bo);
 err:
-	ttm_write_unlock(&vmaster->lock);
+	ttm_write_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
 
@@ -198,7 +196,6 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv,
 				struct vmw_dma_buffer *buf,
 				bool pin, bool interruptible)
 {
-	struct vmw_master *vmaster = dev_priv->active_master;
 	struct ttm_buffer_object *bo = &buf->base;
 	struct ttm_placement placement;
 	int ret = 0;
@@ -209,7 +206,7 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv,
 		placement = vmw_vram_placement;
 	placement.lpfn = bo->num_pages;
 
-	ret = ttm_write_lock(&vmaster->lock, interruptible);
+	ret = ttm_write_lock(&dev_priv->reservation_sem, interruptible);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -232,7 +229,7 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv,
 
 	ttm_bo_unreserve(bo);
 err_unlock:
-	ttm_write_unlock(&vmaster->lock);
+	ttm_write_unlock(&dev_priv->reservation_sem);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index c35715f..54cfeb6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -606,6 +606,7 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 	mutex_init(&dev_priv->release_mutex);
 	mutex_init(&dev_priv->binding_mutex);
 	rwlock_init(&dev_priv->resource_lock);
+	ttm_lock_init(&dev_priv->reservation_sem);
 
 	for (i = vmw_res_context; i < vmw_res_max; ++i) {
 		idr_init(&dev_priv->res_idr[i]);
@@ -1175,12 +1176,11 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val,
 {
 	struct vmw_private *dev_priv =
 		container_of(nb, struct vmw_private, pm_nb);
-	struct vmw_master *vmaster = dev_priv->active_master;
 
 	switch (val) {
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
-		ttm_suspend_lock(&vmaster->lock);
+		ttm_suspend_lock(&dev_priv->reservation_sem);
 
 		/**
 		 * This empties VRAM and unbinds all GMR bindings.
@@ -1194,7 +1194,7 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val,
 	case PM_POST_HIBERNATION:
 	case PM_POST_SUSPEND:
 	case PM_POST_RESTORE:
-		ttm_suspend_unlock(&vmaster->lock);
+		ttm_suspend_unlock(&dev_priv->reservation_sem);
 
 		break;
 	case PM_RESTORE_PREPARE:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 0783155..8c6b71f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -487,6 +487,11 @@ struct vmw_private {
 	uint32_t num_3d_resources;
 
 	/*
+	 * Replace this with an rwsem as soon as we have down_xx_interruptible()
+	 */
+	struct ttm_lock reservation_sem;
+
+	/*
 	 * Query processing. These members
 	 * are protected by the cmdbuf mutex.
 	 */
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index efb575a..931490b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -2712,7 +2712,6 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
 	struct drm_vmw_execbuf_arg *arg = (struct drm_vmw_execbuf_arg *)data;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	int ret;
 
 	/*
@@ -2729,7 +2728,7 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -2745,6 +2744,6 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
 	vmw_kms_cursor_post_execbuf(dev_priv);
 
 out_unlock:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index ed5ce2a..9699bd1 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -377,14 +377,13 @@ static int vmw_fb_create_bo(struct vmw_private *vmw_priv,
 
 	ne_placement.lpfn = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 
-	/* interuptable? */
-	ret = ttm_write_lock(&vmw_priv->fbdev_master.lock, false);
-	if (unlikely(ret != 0))
-		return ret;
+	(void) ttm_write_lock(&vmw_priv->reservation_sem, false);
 
 	vmw_bo = kmalloc(sizeof(*vmw_bo), GFP_KERNEL);
-	if (!vmw_bo)
+	if (!vmw_bo) {
+		ret = -ENOMEM;
 		goto err_unlock;
+	}
 
 	ret = vmw_dmabuf_init(vmw_priv, vmw_bo, size,
 			      &ne_placement,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
index 47b7094..37881ec 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
@@ -226,7 +226,6 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
 	struct drm_vmw_present_arg *arg =
 		(struct drm_vmw_present_arg *)data;
 	struct vmw_surface *surface;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	struct drm_vmw_rect __user *clips_ptr;
 	struct drm_vmw_rect *clips = NULL;
 	struct drm_framebuffer *fb;
@@ -271,7 +270,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
 	}
 	vfb = vmw_framebuffer_to_vfb(fb);
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		goto out_no_ttm_lock;
 
@@ -291,7 +290,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
 	vmw_surface_unreference(&surface);
 
 out_no_surface:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 out_no_ttm_lock:
 	drm_framebuffer_unreference(fb);
 out_no_fb:
@@ -311,7 +310,6 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
 	struct drm_vmw_fence_rep __user *user_fence_rep =
 		(struct drm_vmw_fence_rep __user *)
 		(unsigned long)arg->fence_rep;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	struct drm_vmw_rect __user *clips_ptr;
 	struct drm_vmw_rect *clips = NULL;
 	struct drm_framebuffer *fb;
@@ -361,7 +359,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
 		goto out_no_ttm_lock;
 	}
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		goto out_no_ttm_lock;
 
@@ -369,7 +367,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,
 			       vfb, user_fence_rep,
 			       clips, num_clips);
 
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 out_no_ttm_lock:
 	drm_framebuffer_unreference(fb);
 out_no_fb:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 8a65041..159af7e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -596,7 +596,6 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
 				  unsigned num_clips)
 {
 	struct vmw_private *dev_priv = vmw_priv(framebuffer->dev);
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	struct vmw_framebuffer_surface *vfbs =
 		vmw_framebuffer_to_vfbs(framebuffer);
 	struct drm_clip_rect norect;
@@ -611,7 +610,7 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
 
 	drm_modeset_lock_all(dev_priv->dev);
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0)) {
 		drm_modeset_unlock_all(dev_priv->dev);
 		return ret;
@@ -632,7 +631,7 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
 				   flags, color,
 				   clips, num_clips, inc, NULL);
 
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 
 	drm_modeset_unlock_all(dev_priv->dev);
 
@@ -954,7 +953,6 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,
 				 unsigned num_clips)
 {
 	struct vmw_private *dev_priv = vmw_priv(framebuffer->dev);
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	struct vmw_framebuffer_dmabuf *vfbd =
 		vmw_framebuffer_to_vfbd(framebuffer);
 	struct drm_clip_rect norect;
@@ -962,7 +960,7 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,
 
 	drm_modeset_lock_all(dev_priv->dev);
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0)) {
 		drm_modeset_unlock_all(dev_priv->dev);
 		return ret;
@@ -989,7 +987,7 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,
 					  clips, num_clips, increment, NULL);
 	}
 
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 
 	drm_modeset_unlock_all(dev_priv->dev);
 
@@ -2022,7 +2020,6 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 	struct vmw_private *dev_priv = vmw_priv(dev);
 	struct drm_vmw_update_layout_arg *arg =
 		(struct drm_vmw_update_layout_arg *)data;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	void __user *user_rects;
 	struct drm_vmw_rect *rects;
 	unsigned rects_size;
@@ -2030,7 +2027,7 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 	int i;
 	struct drm_mode_config *mode_config = &dev->mode_config;
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -2072,6 +2069,6 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 out_free:
 	kfree(rects);
 out_unlock:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index 9757b57..30439cb 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -676,10 +676,9 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,
 	struct drm_vmw_dmabuf_rep *rep = &arg->rep;
 	struct vmw_dma_buffer *dma_buf;
 	uint32_t handle;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	int ret;
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -696,7 +695,7 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,
 	vmw_dmabuf_unreference(&dma_buf);
 
 out_no_dmabuf:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 
 	return ret;
 }
@@ -873,7 +872,6 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data,
 	struct vmw_resource *tmp;
 	struct drm_vmw_stream_arg *arg = (struct drm_vmw_stream_arg *)data;
 	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	int ret;
 
 	/*
@@ -884,7 +882,7 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data,
 	if (unlikely(vmw_user_stream_size == 0))
 		vmw_user_stream_size = ttm_round_pot(sizeof(*stream)) + 128;
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -932,7 +930,7 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data,
 out_err:
 	vmw_resource_unreference(&res);
 out_unlock:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
 
@@ -985,14 +983,13 @@ int vmw_dumb_create(struct drm_file *file_priv,
 		    struct drm_mode_create_dumb *args)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	struct vmw_dma_buffer *dma_buf;
 	int ret;
 
 	args->pitch = args->width * ((args->bpp + 7) / 8);
 	args->size = args->pitch * args->height;
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -1004,7 +1001,7 @@ int vmw_dumb_create(struct drm_file *file_priv,
 
 	vmw_dmabuf_unreference(&dma_buf);
 out_no_dmabuf:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
index ee38565..c1559eea 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
@@ -449,7 +449,6 @@ int vmw_shader_define_ioctl(struct drm_device *dev, void *data,
 	struct drm_vmw_shader_create_arg *arg =
 		(struct drm_vmw_shader_create_arg *)data;
 	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	struct vmw_dma_buffer *buffer = NULL;
 	SVGA3dShaderType shader_type;
 	int ret;
@@ -487,14 +486,14 @@ int vmw_shader_define_ioctl(struct drm_device *dev, void *data,
 		goto out_bad_arg;
 	}
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		goto out_bad_arg;
 
 	ret = vmw_shader_alloc(dev_priv, buffer, arg->size, arg->offset,
 			       shader_type, tfile, &arg->shader_handle);
 
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 out_bad_arg:
 	vmw_dmabuf_unreference(&buffer);
 	return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index e7af580..aac243b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -697,7 +697,6 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
 	struct vmw_surface_offset *cur_offset;
 	uint32_t num_sizes;
 	uint32_t size;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	const struct svga3d_surface_desc *desc;
 
 	if (unlikely(vmw_user_surface_size == 0))
@@ -723,7 +722,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -862,7 +861,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
 	rep->sid = user_srf->prime.base.hash.key;
 	vmw_resource_unreference(&res);
 
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return 0;
 out_no_copy:
 	kfree(srf->offsets);
@@ -873,7 +872,7 @@ out_no_sizes:
 out_no_user_srf:
 	ttm_mem_global_free(vmw_mem_glob(dev_priv), size);
 out_unlock:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
 
@@ -1173,7 +1172,6 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,
 	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
 	int ret;
 	uint32_t size;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
 	const struct svga3d_surface_desc *desc;
 	uint32_t backup_handle;
 
@@ -1189,7 +1187,7 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	ret = ttm_read_lock(&vmaster->lock, true);
+	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -1283,12 +1281,12 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,
 
 	vmw_resource_unreference(&res);
 
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return 0;
 out_no_user_srf:
 	ttm_mem_global_free(vmw_mem_glob(dev_priv), size);
 out_unlock:
-	ttm_read_unlock(&vmaster->lock);
+	ttm_read_unlock(&dev_priv->reservation_sem);
 	return ret;
 }
 
-- 
1.7.10.4

[PATCH 09/16] drm/vmwgfx: Reinstate and tighten security around legacy master model

[Thomas Hellstrom]

The following restrictions affect clients connecting using legacy nodes:

*) Masters that have dropped master privilieges are not considered
   authenticated until they regain master privileges.
*) Clients whose master have dropped master privileges block interruptibly on
   ioctls  requiring authentication until their master regains master
   privileges. If their master exits, they are killed.

This is primarily designed to prevent clients authenticated with one master to
access data from clients authenticated with another master.
(Think fast user-switching or data sniffers enabled while X is vt-switched).

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

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 54cfeb6..8fdbe26 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -982,12 +982,70 @@ out_no_tfile:
 	return ret;
 }
 
-static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd,
-			       unsigned long arg)
+static struct vmw_master *vmw_master_check(struct drm_device *dev,
+					   struct drm_file *file_priv,
+					   unsigned int flags)
+{
+	int ret;
+	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
+	struct vmw_master *vmaster;
+
+	if (file_priv->minor->type != DRM_MINOR_LEGACY ||
+	    !(flags & DRM_AUTH))
+		return NULL;
+
+	ret = mutex_lock_interruptible(&dev->master_mutex);
+	if (unlikely(ret != 0))
+		return ERR_PTR(-ERESTARTSYS);
+
+	if (file_priv->is_master) {
+		mutex_unlock(&dev->master_mutex);
+		return NULL;
+	}
+
+	/*
+	 * Check if we were previously master, but now dropped.
+	 */
+	if (vmw_fp->locked_master) {
+		mutex_unlock(&dev->master_mutex);
+		DRM_ERROR("Dropped master trying to access ioctl that "
+			  "requires authentication.\n");
+		return ERR_PTR(-EACCES);
+	}
+	mutex_unlock(&dev->master_mutex);
+
+	/*
+	 * Taking the drm_global_mutex after the TTM lock might deadlock
+	 */
+	if (!(flags & DRM_UNLOCKED)) {
+		DRM_ERROR("Refusing locked ioctl access.\n");
+		return ERR_PTR(-EDEADLK);
+	}
+
+	/*
+	 * Take the TTM lock. Possibly sleep waiting for the authenticating
+	 * master to become master again, or for a SIGTERM if the
+	 * authenticating master exits.
+	 */
+	vmaster = vmw_master(file_priv->master);
+	ret = ttm_read_lock(&vmaster->lock, true);
+	if (unlikely(ret != 0))
+		vmaster = ERR_PTR(ret);
+
+	return vmaster;
+}
+
+static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
+			      unsigned long arg,
+			      long (*ioctl_func)(struct file *, unsigned int,
+						 unsigned long))
 {
 	struct drm_file *file_priv = filp->private_data;
 	struct drm_device *dev = file_priv->minor->dev;
 	unsigned int nr = DRM_IOCTL_NR(cmd);
+	struct vmw_master *vmaster;
+	unsigned int flags;
+	long ret;
 
 	/*
 	 * Do extra checking on driver private ioctls.
@@ -996,18 +1054,44 @@ static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd,
 	if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END)
 	    && (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls)) {
 		const struct drm_ioctl_desc *ioctl =
-		    &vmw_ioctls[nr - DRM_COMMAND_BASE];
+			&vmw_ioctls[nr - DRM_COMMAND_BASE];
 
 		if (unlikely(ioctl->cmd_drv != cmd)) {
 			DRM_ERROR("Invalid command format, ioctl %d\n",
 				  nr - DRM_COMMAND_BASE);
 			return -EINVAL;
 		}
+		flags = ioctl->flags;
+	} else if (!drm_ioctl_flags(nr, &flags))
+		return -EINVAL;
+
+	vmaster = vmw_master_check(dev, file_priv, flags);
+	if (unlikely(IS_ERR(vmaster))) {
+		DRM_INFO("IOCTL ERROR %d\n", nr);
+		return PTR_ERR(vmaster);
 	}
 
-	return drm_ioctl(filp, cmd, arg);
+	ret = ioctl_func(filp, cmd, arg);
+	if (vmaster)
+		ttm_read_unlock(&vmaster->lock);
+
+	return ret;
+}
+
+static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd,
+			       unsigned long arg)
+{
+	return vmw_generic_ioctl(filp, cmd, arg, &drm_ioctl);
 }
 
+#ifdef CONFIG_COMPAT
+static long vmw_compat_ioctl(struct file *filp, unsigned int cmd,
+			     unsigned long arg)
+{
+	return vmw_generic_ioctl(filp, cmd, arg, &drm_compat_ioctl);
+}
+#endif
+
 static void vmw_lastclose(struct drm_device *dev)
 {
 	struct drm_crtc *crtc;
@@ -1315,7 +1399,7 @@ static const struct file_operations vmwgfx_driver_fops = {
 	.poll = vmw_fops_poll,
 	.read = vmw_fops_read,
 #if defined(CONFIG_COMPAT)
-	.compat_ioctl = drm_compat_ioctl,
+	.compat_ioctl = vmw_compat_ioctl,
 #endif
 	.llseek = noop_llseek,
 };
-- 
1.7.10.4

[PATCH 10/16] drm/vmwgfx: Drop authentication requirement on UNREF ioctls

[Thomas Hellstrom]

These ioctls will anyway only succeed if the client previously opened
referenced the object. Furthermore, closing the client would implicitly
execute the same action. This prevents clients from blocking on UNREF if
their master dropped, and will allow masters to UNREF after dropping
master privileges.

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

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 8fdbe26..de8a9dc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -146,7 +146,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
 	VMW_IOCTL_DEF(VMW_ALLOC_DMABUF, vmw_dmabuf_alloc_ioctl,
 		      DRM_AUTH | DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_UNREF_DMABUF, vmw_dmabuf_unref_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_CURSOR_BYPASS,
 		      vmw_kms_cursor_bypass_ioctl,
 		      DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED),
@@ -161,11 +161,11 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
 	VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl,
 		      DRM_AUTH | DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_UNREF_CONTEXT, vmw_context_destroy_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_CREATE_SURFACE, vmw_surface_define_ioctl,
 		      DRM_AUTH | DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_UNREF_SURFACE, vmw_surface_destroy_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
 		      DRM_AUTH | DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl,
@@ -176,7 +176,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
 		      vmw_fence_obj_signaled_ioctl,
 		      DRM_AUTH | DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_FENCE_EVENT,
 		      vmw_fence_event_ioctl,
 		      DRM_AUTH | DRM_UNLOCKED),
@@ -197,7 +197,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
 		      DRM_AUTH | DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_UNREF_SHADER,
 		      vmw_shader_destroy_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE,
 		      vmw_gb_surface_define_ioctl,
 		      DRM_AUTH | DRM_UNLOCKED),
-- 
1.7.10.4

[PATCH 11/16] drm/vmwgfx: Allow prime fds in the surface reference ioctls

[Thomas Hellstrom]

Allow prime fds and at the same time block legacy handles for render-nodes
in the surface reference ioctls. This means these ioctls can be used
directly from prime-aware clients, and that they can be called from
render-nodes.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |  105 ++++++++++++++++++++-----------
 include/uapi/drm/vmwgfx_drm.h           |   12 +++-
 2 files changed, 81 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index aac243b..d50cd76 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -876,6 +876,64 @@ out_unlock:
 	return ret;
 }
 
+
+static int
+vmw_surface_handle_reference(struct vmw_private *dev_priv,
+			     struct drm_file *file_priv,
+			     uint32_t u_handle,
+			     enum drm_vmw_handle_type handle_type,
+			     struct ttm_base_object **base_p)
+{
+	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+	uint32_t handle;
+	struct ttm_base_object *base;
+	int ret;
+
+	if (handle_type == DRM_VMW_HANDLE_PRIME) {
+		ret = ttm_prime_fd_to_handle(tfile, u_handle, &handle);
+		if (unlikely(ret != 0))
+			return ret;
+	} else {
+		if (unlikely(drm_is_render_client(file_priv))) {
+			DRM_ERROR("Render client refused legacy "
+				  "surface reference.\n");
+			return -EACCES;
+		}
+		handle = u_handle;
+	}
+
+	ret = -EINVAL;
+	base = ttm_base_object_lookup_for_ref(dev_priv->tdev, handle);
+	if (unlikely(base == NULL)) {
+		DRM_ERROR("Could not find surface to reference.\n");
+		goto out_no_lookup;
+	}
+
+	if (unlikely(ttm_base_object_type(base) != VMW_RES_SURFACE)) {
+		DRM_ERROR("Referenced object is not a surface.\n");
+		goto out_bad_resource;
+	}
+
+	if (handle_type != DRM_VMW_HANDLE_PRIME) {
+		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");
+			goto out_bad_resource;
+		}
+	}
+
+	*base_p = base;
+	return 0;
+
+out_bad_resource:
+	ttm_base_object_unref(&base);
+out_no_lookup:
+	if (handle_type == DRM_VMW_HANDLE_PRIME)
+		(void) ttm_ref_object_base_unref(tfile, handle, TTM_REF_USAGE);
+
+	return ret;
+}
+
 /**
  * vmw_user_surface_define_ioctl - Ioctl function implementing
  *                                  the user surface reference functionality.
@@ -897,27 +955,16 @@ int vmw_surface_reference_ioctl(struct drm_device *dev, void *data,
 	struct vmw_user_surface *user_srf;
 	struct drm_vmw_size __user *user_sizes;
 	struct ttm_base_object *base;
-	int ret = -EINVAL;
-
-	base = ttm_base_object_lookup_for_ref(dev_priv->tdev, req->sid);
-	if (unlikely(base == NULL)) {
-		DRM_ERROR("Could not find surface to reference.\n");
-		return -EINVAL;
-	}
+	int ret;
 
-	if (unlikely(ttm_base_object_type(base) != VMW_RES_SURFACE))
-		goto out_bad_resource;
+	ret = vmw_surface_handle_reference(dev_priv, file_priv, req->sid,
+					   req->handle_type, &base);
+	if (unlikely(ret != 0))
+		return ret;
 
 	user_srf = container_of(base, struct vmw_user_surface, prime.base);
 	srf = &user_srf->srf;
 
-	ret = ttm_ref_object_add(tfile, &user_srf->prime.base,
-				 TTM_REF_USAGE, NULL);
-	if (unlikely(ret != 0)) {
-		DRM_ERROR("Could not add a reference to a surface.\n");
-		goto out_no_reference;
-	}
-
 	rep->flags = srf->flags;
 	rep->format = srf->format;
 	memcpy(rep->mip_levels, srf->mip_levels, sizeof(srf->mip_levels));
@@ -930,10 +977,10 @@ int vmw_surface_reference_ioctl(struct drm_device *dev, void *data,
 	if (unlikely(ret != 0)) {
 		DRM_ERROR("copy_to_user failed %p %u\n",
 			  user_sizes, srf->num_sizes);
+		ttm_ref_object_base_unref(tfile, base->hash.key, TTM_REF_USAGE);
 		ret = -EFAULT;
 	}
-out_bad_resource:
-out_no_reference:
+
 	ttm_base_object_unref(&base);
 
 	return ret;
@@ -1313,14 +1360,10 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data,
 	uint32_t backup_handle;
 	int ret = -EINVAL;
 
-	base = ttm_base_object_lookup_for_ref(dev_priv->tdev, req->sid);
-	if (unlikely(base == NULL)) {
-		DRM_ERROR("Could not find surface to reference.\n");
-		return -EINVAL;
-	}
-
-	if (unlikely(ttm_base_object_type(base) != VMW_RES_SURFACE))
-		goto out_bad_resource;
+	ret = vmw_surface_handle_reference(dev_priv, file_priv, req->sid,
+					   req->handle_type, &base);
+	if (unlikely(ret != 0))
+		return ret;
 
 	user_srf = container_of(base, struct vmw_user_surface, prime.base);
 	srf = &user_srf->srf;
@@ -1329,13 +1372,6 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data,
 		goto out_bad_resource;
 	}
 
-	ret = ttm_ref_object_add(tfile, &user_srf->prime.base,
-				 TTM_REF_USAGE, NULL);
-	if (unlikely(ret != 0)) {
-		DRM_ERROR("Could not add a reference to a GB surface.\n");
-		goto out_bad_resource;
-	}
-
 	mutex_lock(&dev_priv->cmdbuf_mutex); /* Protect res->backup */
 	ret = vmw_user_dmabuf_reference(tfile, srf->res.backup,
 					&backup_handle);
@@ -1344,8 +1380,7 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data,
 	if (unlikely(ret != 0)) {
 		DRM_ERROR("Could not add a reference to a GB surface "
 			  "backup buffer.\n");
-		(void) ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,
-						 req->sid,
+		(void) ttm_ref_object_base_unref(tfile, base->hash.key,
 						 TTM_REF_USAGE);
 		goto out_bad_resource;
 	}
diff --git a/include/uapi/drm/vmwgfx_drm.h b/include/uapi/drm/vmwgfx_drm.h
index 87792a5..4fc66f6 100644
--- a/include/uapi/drm/vmwgfx_drm.h
+++ b/include/uapi/drm/vmwgfx_drm.h
@@ -90,6 +90,15 @@
 #define DRM_VMW_PARAM_MAX_MOB_SIZE     10
 
 /**
+ * enum drm_vmw_handle_type - handle type for ref ioctls
+ *
+ */
+enum drm_vmw_handle_type {
+	DRM_VMW_HANDLE_LEGACY = 0,
+	DRM_VMW_HANDLE_PRIME = 1
+};
+
+/**
  * struct drm_vmw_getparam_arg
  *
  * @value: Returned value. //Out
@@ -177,6 +186,7 @@ struct drm_vmw_surface_create_req {
  * struct drm_wmv_surface_arg
  *
  * @sid: Surface id of created surface or surface to destroy or reference.
+ * @handle_type: Handle type for DRM_VMW_REF_SURFACE Ioctl.
  *
  * Output data from the DRM_VMW_CREATE_SURFACE Ioctl.
  * Input argument to the DRM_VMW_UNREF_SURFACE Ioctl.
@@ -185,7 +195,7 @@ struct drm_vmw_surface_create_req {
 
 struct drm_vmw_surface_arg {
 	int32_t sid;
-	uint32_t pad64;
+	enum drm_vmw_handle_type handle_type;
 };
 
 /**
-- 
1.7.10.4

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

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

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..8688e52 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_legacy_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_legacy_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_legacy_client(file_priv))
+		user_srf->master = drm_master_get(file_priv->master);
 
 	/**
 	 * From this point, the generic resource management functions
-- 
1.7.10.4

[PATCH 13/16] drm/ttm: Add a ttm_ref_object_exists function

[Thomas Hellstrom]

A function to be used to check whether a caller has put a ref object
(opened) a struct ttm_base_object

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
---
 drivers/gpu/drm/ttm/ttm_object.c |   46 ++++++++++++++++++++++++++++++++++++++
 include/drm/ttm/ttm_object.h     |    4 ++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_object.c b/drivers/gpu/drm/ttm/ttm_object.c
index 53b51c4..d2a0533 100644
--- a/drivers/gpu/drm/ttm/ttm_object.c
+++ b/drivers/gpu/drm/ttm/ttm_object.c
@@ -270,6 +270,52 @@ ttm_base_object_lookup_for_ref(struct ttm_object_device *tdev, uint32_t key)
 }
 EXPORT_SYMBOL(ttm_base_object_lookup_for_ref);
 
+/**
+ * ttm_ref_object_exists - Check whether a caller has a valid ref object
+ * (has opened) a base object.
+ *
+ * @tfile: Pointer to a struct ttm_object_file identifying the caller.
+ * @base: Pointer to a struct base object.
+ *
+ * Checks wether the caller identified by @tfile has put a valid USAGE
+ * reference object on the base object identified by @base.
+ */
+bool ttm_ref_object_exists(struct ttm_object_file *tfile,
+			   struct ttm_base_object *base)
+{
+	struct drm_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE];
+	struct drm_hash_item *hash;
+	struct ttm_ref_object *ref;
+
+	rcu_read_lock();
+	if (unlikely(drm_ht_find_item_rcu(ht, base->hash.key, &hash) != 0))
+		goto out_false;
+
+	/*
+	 * Verify that the ref object is really pointing to our base object.
+	 * Our base object could actually be dead, and the ref object pointing
+	 * to another base object with the same handle.
+	 */
+	ref = drm_hash_entry(hash, struct ttm_ref_object, hash);
+	if (unlikely(base != ref->obj))
+		goto out_false;
+
+	/*
+	 * Verify that the ref->obj pointer was actually valid!
+	 */
+	rmb();
+	if (unlikely(atomic_read(&ref->kref.refcount) == 0))
+		goto out_false;
+
+	rcu_read_unlock();
+	return true;
+
+ out_false:
+	rcu_read_unlock();
+	return false;
+}
+EXPORT_SYMBOL(ttm_ref_object_exists);
+
 int ttm_ref_object_add(struct ttm_object_file *tfile,
 		       struct ttm_base_object *base,
 		       enum ttm_ref_type ref_type, bool *existed)
diff --git a/include/drm/ttm/ttm_object.h b/include/drm/ttm/ttm_object.h
index 0097cc0..ed953f9 100644
--- a/include/drm/ttm/ttm_object.h
+++ b/include/drm/ttm/ttm_object.h
@@ -244,6 +244,10 @@ extern void ttm_base_object_unref(struct ttm_base_object **p_base);
 extern int ttm_ref_object_add(struct ttm_object_file *tfile,
 			      struct ttm_base_object *base,
 			      enum ttm_ref_type ref_type, bool *existed);
+
+extern bool ttm_ref_object_exists(struct ttm_object_file *tfile,
+				  struct ttm_base_object *base);
+
 /**
  * ttm_ref_object_base_unref
  *
-- 
1.7.10.4

[PATCH 14/16] drm/vmwgfx: Tighten the security around buffer maps

[Thomas Hellstrom]

Make sure only buffer objects that are referenced by the client can be mapped.

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

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index 30439cb..01d68f0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -538,8 +538,13 @@ int vmw_user_dmabuf_verify_access(struct ttm_buffer_object *bo,
 		return -EPERM;
 
 	vmw_user_bo = vmw_user_dma_buffer(bo);
-	return (vmw_user_bo->prime.base.tfile == tfile ||
-		vmw_user_bo->prime.base.shareable) ? 0 : -EPERM;
+
+	/* Check that the caller has opened the object. */
+	if (likely(ttm_ref_object_exists(tfile, &vmw_user_bo->prime.base)))
+		return 0;
+
+	DRM_ERROR("Could not grant buffer access.\n");
+	return -EPERM;
 }
 
 /**
-- 
1.7.10.4

[PATCH 15/16] drm/vmwgfx: Enable render nodes

[Thomas Hellstrom]

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

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index de8a9dc..c700958 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -142,11 +142,11 @@
 
 static const struct drm_ioctl_desc vmw_ioctls[] = {
 	VMW_IOCTL_DEF(VMW_GET_PARAM, vmw_getparam_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_ALLOC_DMABUF, vmw_dmabuf_alloc_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_UNREF_DMABUF, vmw_dmabuf_unref_ioctl,
-		      DRM_UNLOCKED),
+		      DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_CURSOR_BYPASS,
 		      vmw_kms_cursor_bypass_ioctl,
 		      DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED),
@@ -159,29 +159,28 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
 		      DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED),
 
 	VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_UNREF_CONTEXT, vmw_context_destroy_ioctl,
-		      DRM_UNLOCKED),
+		      DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_CREATE_SURFACE, vmw_surface_define_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_UNREF_SURFACE, vmw_surface_destroy_ioctl,
-		      DRM_UNLOCKED),
+		      DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_FENCE_SIGNALED,
 		      vmw_fence_obj_signaled_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl,
-		      DRM_UNLOCKED),
-	VMW_IOCTL_DEF(VMW_FENCE_EVENT,
-		      vmw_fence_event_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_UNLOCKED | DRM_RENDER_ALLOW),
+	VMW_IOCTL_DEF(VMW_FENCE_EVENT, vmw_fence_event_ioctl,
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_GET_3D_CAP, vmw_get_cap_3d_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 
 	/* these allow direct access to the framebuffers mark as master only */
 	VMW_IOCTL_DEF(VMW_PRESENT, vmw_present_ioctl,
@@ -194,19 +193,19 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
 		      DRM_MASTER | DRM_UNLOCKED),
 	VMW_IOCTL_DEF(VMW_CREATE_SHADER,
 		      vmw_shader_define_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_UNREF_SHADER,
 		      vmw_shader_destroy_ioctl,
-		      DRM_UNLOCKED),
+		      DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE,
 		      vmw_gb_surface_define_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_GB_SURFACE_REF,
 		      vmw_gb_surface_reference_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_SYNCCPU,
 		      vmw_user_dmabuf_synccpu_ioctl,
-		      DRM_AUTH | DRM_UNLOCKED),
+		      DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW),
 };
 
 static struct pci_device_id vmw_pci_id_list[] = {
@@ -1406,7 +1405,7 @@ static const struct file_operations vmwgfx_driver_fops = {
 
 static struct drm_driver driver = {
 	.driver_features = DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED |
-	DRIVER_MODESET | DRIVER_PRIME,
+	DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
 	.load = vmw_driver_load,
 	.unload = vmw_driver_unload,
 	.lastclose = vmw_lastclose,
-- 
1.7.10.4

[PATCH 16/16] drm/vmwgfx: Bump driver minor and date

[Thomas Hellstrom]

Signal availability of prime fd reference ioctls and render nodes.

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

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 8c6b71f..6b252a8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -40,9 +40,9 @@
 #include <drm/ttm/ttm_module.h>
 #include "vmwgfx_fence.h"
 
-#define VMWGFX_DRIVER_DATE "20140228"
+#define VMWGFX_DRIVER_DATE "20140325"
 #define VMWGFX_DRIVER_MAJOR 2
-#define VMWGFX_DRIVER_MINOR 5
+#define VMWGFX_DRIVER_MINOR 6
 #define VMWGFX_DRIVER_PATCHLEVEL 0
 #define VMWGFX_FILE_PAGE_OFFSET 0x00100000
 #define VMWGFX_FIFO_STATIC_SIZE (1024*1024)
-- 
1.7.10.4