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

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