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:
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 {
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
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:
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; }
} elsememset(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; }} elsememset(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
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 */ /*@{*/
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
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 */ /*@{*/
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; };
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
David Herrmann -
Thomas Hellstrom