| Message ID | 1394708244-6565-3-git-send-email-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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:
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(-)