(unknown),

(unknown),

[Thomas Hellstrom]

After a previous patch series and a discussion with Daniel Vetter and
David Herrmann, I've reworked the patches a bit. Please review.

Patch 5 is already reviewed.

/Thomas

>From Thomas Hellstrom <thellstrom@vmware.com> # This line is ignored.
From: Thomas Hellstrom <thellstrom@vmware.com>
Subject: 
In-Reply-To: 

[PATCH 1/5] drm: Have the crtc code only reference master from legacy nodes

[Thomas Hellstrom]

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

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/drm_crtc.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3b7d32d..c9d895a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1398,7 +1398,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	uint32_t __user *crtc_id;
 	uint32_t __user *connector_id;
 	uint32_t __user *encoder_id;
-	struct drm_mode_group *mode_group;
+	struct drm_mode_group *mode_group = NULL;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -1429,8 +1429,7 @@ 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) {
 
 		list_for_each(lh, &dev->mode_config.crtc_list)
 			crtc_count++;
@@ -1442,6 +1441,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;
@@ -1456,7 +1456,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 (file_priv->minor->type != DRM_MINOR_LEGACY) {
 			list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 					    head) {
 				DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
@@ -1483,7 +1483,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 (file_priv->minor->type != DRM_MINOR_LEGACY) {
 			list_for_each_entry(encoder,
 					    &dev->mode_config.encoder_list,
 					    head) {
@@ -1514,7 +1514,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 (file_priv->minor->type != DRM_MINOR_LEGACY) {
 			list_for_each_entry(connector,
 					    &dev->mode_config.connector_list,
 					    head) {
@@ -2715,7 +2715,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

Re: [PATCH 1/5] drm: Have the crtc code only reference master from legacy nodes

[David Herrmann]

Hi

On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom
<thellstrom@vmware.com> wrote:
> control- and render nodes are intended to be master-less.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 3b7d32d..c9d895a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1398,7 +1398,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>         uint32_t __user *crtc_id;
>         uint32_t __user *connector_id;
>         uint32_t __user *encoder_id;
> -       struct drm_mode_group *mode_group;
> +       struct drm_mode_group *mode_group = NULL;

Why not move this mode_group=NULL; into the if() condition below?
Avoids global initialization and makes the "if() - else" parts easier
to read as you directly see that mode_group is set to NULL for
non-legacy nodes.

>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
> @@ -1429,8 +1429,7 @@ 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) {
>
>                 list_for_each(lh, &dev->mode_config.crtc_list)
>                         crtc_count++;
> @@ -1442,6 +1441,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;
> @@ -1456,7 +1456,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 (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

You set mode_group=NULL with your patch, so I think it's much easier
to read if you replace all these minor-id-tests with "if
(!mode_group)". This moves the group-selection to the head of the
function and makes the remaining parts just work on the selected group
(or global if NULL).

Same for the two conditions below..

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

Thanks
David

>                         list_for_each_entry(crtc, &dev->mode_config.crtc_list,
>                                             head) {
>                                 DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
> @@ -1483,7 +1483,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 (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

>                         list_for_each_entry(encoder,
>                                             &dev->mode_config.encoder_list,
>                                             head) {
> @@ -1514,7 +1514,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 (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

>                         list_for_each_entry(connector,
>                                             &dev->mode_config.connector_list,
>                                             head) {
> @@ -2715,7 +2715,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
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm: Break out ioctl permission check to a separate function

[Thomas Hellstrom]

Helps reviewing and understanding these checks.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/drm_drv.c |  116 ++++++++++++++++++++++++++++++---------------
 1 file changed, 78 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 345be03..0afc6e4 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -285,6 +285,47 @@ static int drm_version(struct drm_device *dev, void *data,
 	return err;
 }
 
+
+/**
+ * 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.
  *
@@ -350,52 +391,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

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

[David Herrmann]

Hi

On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom
<thellstrom@vmware.com> wrote:
> Helps reviewing and understanding these checks.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  drivers/gpu/drm/drm_drv.c |  116 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 78 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 345be03..0afc6e4 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -285,6 +285,47 @@ static int drm_version(struct drm_device *dev, void *data,
>         return err;
>  }
>
> +

Why the double blank line?

> +/**
> + * 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)
> +{
> +

We don't do blank lines after function-headers.

> +       /* 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;
> +}
> +
> +

Again, double blank-line.

>  /**
>   * Called whenever a process performs an ioctl on /dev/drm.
>   *
> @@ -350,52 +391,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))

That "unlikely" seems redundant given that all error paths in
drm_ioctl_permit() already are "unlikely".

Otherwise, patch looks good:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

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

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

[Thomas Hellstrom]

Hi.

Thanks for reviewing. I'll incorporate your suggestions, except this
one, and resend.


On 03/13/2014 12:19 PM, David Herrmann wrote:
> Hi
>
> On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom
> <thellstrom@vmware.com> wrote:
...

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

> That "unlikely" seems redundant given that all error paths in
> drm_ioctl_permit() already are "unlikely".

Yes, we know that's true, but I don't think compilers in general can
combine branch prediction hints in that way,
or even have the information necessary to do it.
I mean even if each individual test resulting in an error is unlikely,
how could the compiler know that
all tests combined would result in an error being unlikely?

/Thomas

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

[David Herrmann]

Hi

On Thu, Mar 13, 2014 at 1:11 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> Hi.
>
> Thanks for reviewing. I'll incorporate your suggestions, except this
> one, and resend.
>
>
> On 03/13/2014 12:19 PM, David Herrmann wrote:
>> Hi
>>
>> On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom
>> <thellstrom@vmware.com> wrote:
> ...
>
> -               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))
>
>> That "unlikely" seems redundant given that all error paths in
>> drm_ioctl_permit() already are "unlikely".
>
> Yes, we know that's true, but I don't think compilers in general can
> combine branch prediction hints in that way,
> or even have the information necessary to do it.
> I mean even if each individual test resulting in an error is unlikely,
> how could the compiler know that
> all tests combined would result in an error being unlikely?

The function is static, so the compiler can see that it returns "!=0"
only if one of the "unlikely" branches was hit. So I think it's safe
to assume the whole thing returns "!=0" only in unlikely conditions.
But it's probably inlined, anyway..

I'm no big fan of excessive likely/unlikely annotations, but I'm fine
if you want to keep it.

Thanks
David

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

[Thomas Hellstrom]

On 03/13/2014 01:15 PM, David Herrmann wrote:
> Hi
>
> On Thu, Mar 13, 2014 at 1:11 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> Hi.
>>
>> Thanks for reviewing. I'll incorporate your suggestions, except this
>> one, and resend.
>>
>>
>> On 03/13/2014 12:19 PM, David Herrmann wrote:
>>> Hi
>>>
>>> On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom
>>> <thellstrom@vmware.com> wrote:
>> ...
>>
>> -               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))
>>
>>> That "unlikely" seems redundant given that all error paths in
>>> drm_ioctl_permit() already are "unlikely".
>> Yes, we know that's true, but I don't think compilers in general can
>> combine branch prediction hints in that way,
>> or even have the information necessary to do it.
>> I mean even if each individual test resulting in an error is unlikely,
>> how could the compiler know that
>> all tests combined would result in an error being unlikely?
> The function is static, so the compiler can see that it returns "!=0"
> only if one of the "unlikely" branches was hit. So I think it's safe
> to assume the whole thing returns "!=0" only in unlikely conditions.
>

But a compiler can't (or shouldn't) make that assumption. Just as an
(adapted) example, imagine that
each test had a 20% probability of returning an error. The probability
of the function returning an error would
then be 68%..

> I'm no big fan of excessive likely/unlikely annotations, but I'm fine
> if you want to keep it.

Fair enough.

Thanks,
Thomas


>
> Thanks
> David
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: Break out ioctl permission check to a separate function

[Daniel Vetter]

On Thu, Mar 13, 2014 at 1:28 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> But a compiler can't (or shouldn't) make that assumption. Just as an
> (adapted) example, imagine that
> each test had a 20% probability of returning an error. The probability
> of the function returning an error would
> then be 68%..

Otoh if you'd put the unlikely just onto the if (ret) then the
compiler could infer that by necessity all branches leading towards
this one are also unlikely. Dunno whether compilers are this clever
though, and I also don't really care if we throw a few too many
likely/unlikely annotations over the place. Just figured I'll throw
this in.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/5] drm: Make control nodes master-less v2

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

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 0afc6e4..e41ee82 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -307,14 +307,15 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 	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))
+	
+	/* MASTER is only for master or control clients */
+	if (unlikely((flags & DRM_MASTER) &&
+		     !(file_priv->is_master || drm_is_control(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(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 7f2af9a..08a3196 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -259,7 +259,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(priv)) {
 		/* create a new master */
 		priv->minor->master = drm_master_create(priv->minor);
 		if (!priv->minor->master) {
@@ -297,7 +298,7 @@ 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(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 04a7f31..ff68e26 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1246,6 +1246,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(struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_CONTROL;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
-- 
1.7.10.4

Re: [PATCH 3/5] drm: Make control nodes master-less v2

[David Herrmann]

Hi

On Thu, Mar 13, 2014 at 11:57 AM, 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.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  drivers/gpu/drm/drm_drv.c  |    9 +++++----
>  drivers/gpu/drm/drm_fops.c |    5 +++--
>  include/drm/drmP.h         |    5 +++++
>  3 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 0afc6e4..e41ee82 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -307,14 +307,15 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>         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))
> +
> +       /* MASTER is only for master or control clients */
> +       if (unlikely((flags & DRM_MASTER) &&
> +                    !(file_priv->is_master || drm_is_control(file_priv))))

imo this looks nicer:
(flags & XY) && !is_master && !drm_is_control()

but that's probably a matter of taste

>                 return -EACCES;
>
>         /* Control clients must be explicitly allowed */
>         if (unlikely(!(flags & DRM_CONTROL_ALLOW) &&
> -                    file_priv->minor->type == DRM_MINOR_CONTROL))
> +                    drm_is_control(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 7f2af9a..08a3196 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -259,7 +259,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(priv)) {
>                 /* create a new master */
>                 priv->minor->master = drm_master_create(priv->minor);
>                 if (!priv->minor->master) {
> @@ -297,7 +298,7 @@ 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(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 04a7f31..ff68e26 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1246,6 +1246,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(struct drm_file *file_priv)

drm_is_control()? Pretty inexpressive.. Why not keep the _client
suffix? drm_is_control_client()..

Thanks
David

> +{
> +       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 4/5] drm: Improve on minor type helpers

[Thomas Hellstrom]

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

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/drm_crtc.c |   10 +++++-----
 drivers/gpu/drm/drm_fops.c |    5 ++---
 include/drm/drmP.h         |    9 +++++++--
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c9d895a..4190c7e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1429,7 +1429,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(file_priv)) {
 
 		list_for_each(lh, &dev->mode_config.crtc_list)
 			crtc_count++;
@@ -1456,7 +1456,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->minor->type != DRM_MINOR_LEGACY) {
+		if (!drm_is_legacy(file_priv)) {
 			list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 					    head) {
 				DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
@@ -1483,7 +1483,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->minor->type != DRM_MINOR_LEGACY) {
+		if (!drm_is_legacy(file_priv)) {
 			list_for_each_entry(encoder,
 					    &dev->mode_config.encoder_list,
 					    head) {
@@ -1514,7 +1514,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->minor->type != DRM_MINOR_LEGACY) {
+		if (!drm_is_legacy(file_priv)) {
 			list_for_each_entry(connector,
 					    &dev->mode_config.connector_list,
 					    head) {
@@ -2716,7 +2716,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(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 08a3196..31a4655 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -259,8 +259,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(priv)) {
+	if (drm_is_legacy(priv) && !priv->minor->master) {
 		/* create a new master */
 		priv->minor->master = drm_master_create(priv->minor);
 		if (!priv->minor->master) {
@@ -298,7 +297,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(priv)) {
+	} else if (drm_is_legacy(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 ff68e26..5db7f86 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1241,16 +1241,21 @@ 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;
 }
 
-static inline bool drm_is_control(struct drm_file *file_priv)
+static inline bool drm_is_control(const struct drm_file *file_priv)
 {
 	return file_priv->minor->type == DRM_MINOR_CONTROL;
 }
 
+static inline bool drm_is_legacy(const struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_LEGACY;
+}
+
 /******************************************************************/
 /** \name Internal function definitions */
 /*@{*/
-- 
1.7.10.4

[PATCH 5/5] 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 98a33c580..4f17c79 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -152,8 +152,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;
 }
 
@@ -171,8 +169,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);
 
@@ -296,7 +292,6 @@ static int drm_get_minor(struct drm_device *dev, struct drm_minor **minor,
 	new_minor->device = MKDEV(DRM_MAJOR, minor_id);
 	new_minor->dev = dev;
 	new_minor->index = minor_id;
-	INIT_LIST_HEAD(&new_minor->master_list);
 
 	idr_replace(&drm_minors_idr, new_minor, minor_id);
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 5db7f86..33e55c7 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -718,7 +718,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 */
@@ -1050,7 +1049,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 0/5] Control node master fixes WAS:

[Thomas Hellstrom]

Hmm. Screwed up git-send-email a bit :(
Resending with new subject.

/Thomas


On 03/13/2014 11:57 AM, Thomas Hellstrom wrote:
> After a previous patch series and a discussion with Daniel Vetter and
> David Herrmann, I've reworked the patches a bit. Please review.
>
> Patch 5 is already reviewed.
>
> /Thomas
>
> >From Thomas Hellstrom <thellstrom@vmware.com> # This line is ignored.
> From: Thomas Hellstrom <thellstrom@vmware.com>
> Subject: 
> In-Reply-To: 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel