After a previous patch series and a discussion with Daniel Vetter and David Herrmann, I've reworked the patches a bit. Please review.
Patch 1,2 and 5 already reviewed.
Thanks, Thomas
control- and render nodes are intended to be master-less.
v2: Replace tests for !legacy with tests for !mode_group for readability.
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_crtc.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3b7d32d..740a9ba3 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1429,9 +1429,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data, mutex_unlock(&file_priv->fbs_lock);
drm_modeset_lock_all(dev); - mode_group = &file_priv->master->minor->mode_group; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (file_priv->minor->type != DRM_MINOR_LEGACY) {
+ mode_group = NULL; list_for_each(lh, &dev->mode_config.crtc_list) crtc_count++;
@@ -1442,6 +1442,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 +1457,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, if (card_res->count_crtcs >= crtc_count) { copied = 0; crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (!mode_group) { list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id); @@ -1483,7 +1484,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, if (card_res->count_encoders >= encoder_count) { copied = 0; encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (!mode_group) { list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { @@ -1514,7 +1515,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, if (card_res->count_connectors >= connector_count) { copied = 0; connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (!mode_group) { list_for_each_entry(connector, &dev->mode_config.connector_list, head) { @@ -2715,7 +2716,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 {
Helps reviewing and understanding these checks. v2: Remove misplaced newlines.
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_drv.c | 113 ++++++++++++++++++++++++++++++--------------- 1 file changed, 75 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 345be03..002596c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -286,6 +286,44 @@ static int drm_version(struct drm_device *dev, void *data, }
/** + * drm_ioctl_permit - Check ioctl permissions against caller + * + * @flags: ioctl permission flags. + * @file_priv: Pointer to struct drm_file identifying the caller. + * + * Checks whether the caller is allowed to run an ioctl with the + * indicated permissions. If so, returns zero. Otherwise returns an + * error code suitable for ioctl return. + */ +static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) +{ + /* ROOT_ONLY is only for CAP_SYS_ADMIN */ + if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN))) + return -EACCES; + + /* AUTH is only for authenticated or render client */ + if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && + !file_priv->authenticated)) + return -EACCES; + + /* MASTER is only for master */ + if (unlikely((flags & DRM_MASTER) && !file_priv->is_master)) + return -EACCES; + + /* Control clients must be explicitly allowed */ + if (unlikely(!(flags & DRM_CONTROL_ALLOW) && + file_priv->minor->type == DRM_MINOR_CONTROL)) + return -EACCES; + + /* Render clients must be explicitly allowed */ + if (unlikely(!(flags & DRM_RENDER_ALLOW) && + drm_is_render_client(file_priv))) + return -EACCES; + + return 0; +} + +/** * Called whenever a process performs an ioctl on /dev/drm. * * \param inode device inode. @@ -350,52 +388,51 @@ long drm_ioctl(struct file *filp, /* Do not trust userspace, use our own definition */ func = ioctl->func;
- if (!func) { + if (unlikely(!func)) { DRM_DEBUG("no function\n"); retcode = -EINVAL; - } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || - ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || - ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || - (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) { - retcode = -EACCES; - } else { - if (cmd & (IOC_IN | IOC_OUT)) { - if (asize <= sizeof(stack_kdata)) { - kdata = stack_kdata; - } else { - kdata = kmalloc(asize, GFP_KERNEL); - if (!kdata) { - retcode = -ENOMEM; - goto err_i1; - } - } - if (asize > usize) - memset(kdata + usize, 0, asize - usize); - } + goto err_i1; + }
- if (cmd & IOC_IN) { - if (copy_from_user(kdata, (void __user *)arg, - usize) != 0) { - retcode = -EFAULT; + retcode = drm_ioctl_permit(ioctl->flags, file_priv); + if (unlikely(retcode)) + goto err_i1; + + if (cmd & (IOC_IN | IOC_OUT)) { + if (asize <= sizeof(stack_kdata)) { + kdata = stack_kdata; + } else { + kdata = kmalloc(asize, GFP_KERNEL); + if (!kdata) { + retcode = -ENOMEM; goto err_i1; } - } else - memset(kdata, 0, usize); - - if (ioctl->flags & DRM_UNLOCKED) - retcode = func(dev, kdata, file_priv); - else { - mutex_lock(&drm_global_mutex); - retcode = func(dev, kdata, file_priv); - mutex_unlock(&drm_global_mutex); } + if (asize > usize) + memset(kdata + usize, 0, asize - usize); + }
- if (cmd & IOC_OUT) { - if (copy_to_user((void __user *)arg, kdata, - usize) != 0) - retcode = -EFAULT; + if (cmd & IOC_IN) { + if (copy_from_user(kdata, (void __user *)arg, + usize) != 0) { + retcode = -EFAULT; + goto err_i1; } + } else + memset(kdata, 0, usize); + + if (ioctl->flags & DRM_UNLOCKED) + retcode = func(dev, kdata, file_priv); + else { + mutex_lock(&drm_global_mutex); + retcode = func(dev, kdata, file_priv); + mutex_unlock(&drm_global_mutex); + } + + if (cmd & IOC_OUT) { + if (copy_to_user((void __user *)arg, kdata, + usize) != 0) + retcode = -EFAULT; }
err_i1:
Like for render-nodes, there is no point in maintaining the master concept for control nodes, so set the struct drm_file::master pointer to NULL.
At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always allowed when called through the control node. Previously the caller also needed to be master.
v2: Adapt to refactoring of ioctl permission check. v3: Formatting of logical expression. Use drm_is_control_client() instead of drm_is_control().
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com --- drivers/gpu/drm/drm_drv.c | 9 +++++---- drivers/gpu/drm/drm_fops.c | 6 ++++-- include/drm/drmP.h | 5 +++++ 3 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 002596c..552dbe8 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -305,14 +305,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_client(file_priv))) return -EACCES;
/* Control clients must be explicitly allowed */ if (unlikely(!(flags & DRM_CONTROL_ALLOW) && - file_priv->minor->type == DRM_MINOR_CONTROL)) + drm_is_control_client(file_priv))) return -EACCES;
/* Render clients must be explicitly allowed */ diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 7f2af9a..547ab7f 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_client(priv)) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -297,7 +298,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, goto out_close; } } - } else if (!drm_is_render_client(priv)) { + } else if (!drm_is_render_client(priv) && + !drm_is_control_client(priv)) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 04a7f31..229d307 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_client(const struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_CONTROL; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/
Add a drm_is_legacy() helper, constify argument to drm_is_render_client(), and use / change helpers where appropriate.
v2: s/drm_is_legacy/drm_is_legacy_client/ and adapt to new code context.
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com --- drivers/gpu/drm/drm_crtc.c | 4 ++-- drivers/gpu/drm/drm_fops.c | 6 ++---- include/drm/drmP.h | 7 ++++++- 3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 740a9ba3..c39e54b 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_client(file_priv)) {
mode_group = NULL; list_for_each(lh, &dev->mode_config.crtc_list) @@ -2717,7 +2717,7 @@ int drm_mode_getfb(struct drm_device *dev, r->pitch = fb->pitches[0]; if (fb->funcs->create_handle) { if (file_priv->is_master || capable(CAP_SYS_ADMIN) || - file_priv->minor->type == DRM_MINOR_CONTROL) { + drm_is_control_client(file_priv)) { ret = fb->funcs->create_handle(fb, file_priv, &r->handle); } else { diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 547ab7f..260b5b1 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_client(priv)) { + if (drm_is_legacy_client(priv) && !priv->minor->master) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -298,8 +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_client(priv)) { + } else if (drm_is_legacy_client(priv)) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 229d307..0465ab2 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1241,7 +1241,7 @@ static inline bool drm_modeset_is_locked(struct drm_device *dev) return mutex_is_locked(&dev->mode_config.mutex); }
-static inline bool drm_is_render_client(struct drm_file *file_priv) +static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; } @@ -1251,6 +1251,11 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_CONTROL; }
+static inline bool drm_is_legacy_client(const struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_LEGACY; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/
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 0465ab2..49ead1a 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; };
dri-devel@lists.freedesktop.org
-
Thomas Hellstrom