vmwgfx render-node support
Subsystems (4)
cover vmwgfx render-node support
Hi!
Some of the patches are already reviewed on dri-devel, but included for completeness.
Patch 6 and 7 are drm patches and in particular patch 6 might be good to have an extra pair of eyes on.
Thanks,
Thomas
PATCH 1/16 drm: Have the crtc code only reference master from legacy nodes v2
control- and render nodes are intended to be master-less.
v2: Replace tests for !legacy with tests for !mode_group for readability.
--- drivers/gpu/drm/drm_crtc.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/gpu/drm/drm_crtc.c +8 -6
@@ -1492,9 +1492,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++;
@@ -1505,6 +1505,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;
@@ -1519,7 +1520,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);
@@ -1546,7 +1547,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) {
@@ -1577,7 +1578,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) {
@@ -2846,7 +2847,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 {
Signature · 1 line
1.7.10.4
PATCH 2/16 drm: Break out ioctl permission check to a separate function v2
In reply to Thomas HellstromHelps reviewing and understanding these checks.
v2: Remove misplaced newlines.
--- drivers/gpu/drm/drm_drv.c | 113 ++++++++++++++++++++++++++++++--------------- 1 file changed, 75 insertions(+), 38 deletions(-)
drivers/gpu/drm/drm_drv.c +75 -38
@@ -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:
Signature · 1 line
1.7.10.4
PATCH 3/16 drm: Make control nodes master-less v3
In reply to Thomas HellstromLike 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().
--- drivers/gpu/drm/drm_drv.c | 7 ++++--- drivers/gpu/drm/drm_fops.c | 6 ++++-- include/drm/drmP.h | 5 +++++ 3 files changed, 13 insertions(+), 5 deletions(-)
drivers/gpu/drm/drm_drv.c +4 -3
@@ -306,13 +306,14 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *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 */
drivers/gpu/drm/drm_fops.c +4 -2
@@ -232,7 +232,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) {
@@ -270,7 +271,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); }
include/drm/drmP.h +5 -0
@@ -1237,6 +1237,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 */ /*@{*/
Signature · 1 line
1.7.10.4
Re: [PATCH 03/16] drm: Make control nodes master-less v3
Hi
Quoted 10 lines · Thomas Hellstrom
Like for render-nodes, there is no point in maintaining the master concept for control nodes, so set the struct
drm_file::masterpointer to NULL.At the same time, make sure
DRM_MASTER|DRM_CONTROL_ALLOWioctls 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. Usedrm_is_control_client()instead ofdrm_is_control().
Looks good now.
Thanks
David
Quoted 76 lines
Signed-off-by Thomas Hellstrom <[email protected]>Reviewed-by Brian Paul <[email protected]>--- drivers/gpu/drm/drm_drv.c | 7 ++++--- drivers/gpu/drm/drm_fops.c | 6 ++++-- include/drm/drmP.h | 5 +++++ 3 files changed, 13 insertions(+), 5 deletions(-)drivers/gpu/drm/drm_drv.c +4
-3@@ -306,13 +306,14 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *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 */ drivers/gpu/drm/drm_fops.c +4
-2@@ -232,7 +232,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) {@@ -270,7 +271,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); }include/drm/drmP.h +5
-1@@ -1237,6 +1237,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 */ /*@{*/ --1.7.10.4
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 4/16 drm: Improve on minor type helpers v2
In reply to Thomas HellstromAdd 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.
--- 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(-)
drivers/gpu/drm/drm_crtc.c +2 -2
@@ -1492,7 +1492,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)
@@ -2848,7 +2848,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 {
drivers/gpu/drm/drm_fops.c +2 -4
@@ -232,8 +232,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) {
@@ -271,8 +270,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); }
include/drm/drmP.h +6 -1
@@ -1232,7 +1232,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 ; }
@@ -1242,6 +1242,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 */ /*@{*/
Signature · 1 line
1.7.10.4
Re: [PATCH 04/16] drm: Improve on minor type helpers v2
Hi
Quoted 4 lines · Thomas Hellstrom
Add a
drm_is_legacy()helper, constify argument todrm_is_render_client(), and use / change helpers where appropriate.v2: s/drm_is_legacy/drm_is_legacy_client/ and adapt to new code context.
Could we avoid using "legacy" for the primary node? The node-pointer is called ->primary and Thierry posted patches to also rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY. The primary node is still the recommended node for mode-setting, so lets not attribute "legacy" to them,
Otherwise, patch looks good.
Thanks
David
Quoted 85 lines
Signed-off-by Thomas Hellstrom <[email protected]>Reviewed-by Brian Paul <[email protected]>--- 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(-)drivers/gpu/drm/drm_crtc.c +2
-2@@ -1492,7 +1492,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)@@ -2848,7 +2848,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 {drivers/gpu/drm/drm_fops.c +2
-4@@ -232,8 +232,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) {@@ -271,8 +270,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); }include/drm/drmP.h +6
-2@@ -1232,7 +1232,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 ; }@@ -1242,6 +1242,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 */ /*@{*/ --1.7.10.4
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 5/16 drm: Remove the minor master list
In reply to Thomas HellstromIt doesn't appear to be used anywhere.
--- drivers/gpu/drm/drm_stub.c | 5 ----- include/drm/drmP.h | 2 -- 2 files changed, 7 deletions(-)
drivers/gpu/drm/drm_stub.c +0 -5
@@ -135,8 +135,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; }
@@ -154,8 +152,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);
@@ -281,7 +277,6 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) minor->type = type; minor->dev = dev; -INIT_LIST_HEAD (&minor->master_list); *drm_minor_get_slot (dev, type) = minor;return 0 ;
include/drm/drmP.h +0 -2
@@ -719,7 +719,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 */
@@ -1052,7 +1051,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; };
Signature · 1 line
1.7.10.4
PATCH 6/16 drm: Protect the master management with a drm_device::master_mutex
In reply to Thomas HellstromThe master management was previously protected by the drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separate master_mutex.
--- drivers/gpu/drm/drm_fops.c | 23 +++++++++++++--------- drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++-------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 44 deletions(-)
drivers/gpu/drm/drm_fops.c +14 -9
@@ -231,12 +231,13 @@ 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); +mutex_lock (&dev->master_mutex);if (drm_is_legacy_client (priv) && !priv->minor->master) {/* create a new master */ +mutex_lock (&dev->struct_mutex); priv->minor->master =drm_master_create (priv->minor); +mutex_unlock (&dev->struct_mutex);if (!priv->minor->master) { -mutex_unlock (&dev->struct_mutex); ret = -ENOMEM ; goto out_close; }
@@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,/* take another reference for the copy in the local file priv */ priv->master =drm_master_get (priv->minor->master); +mutex_lock (&dev->struct_mutex); priv->authenticated =1 ;mutex_unlock (&dev->struct_mutex);if (dev->driver->master_create) { ret = dev->driver->master_create (dev, priv->master);if (ret) { -mutex_lock (&dev->struct_mutex);/* drop both references if this fails */ drm_master_put (&priv->minor->master);drm_master_put (&priv->master); -mutex_unlock (&dev->struct_mutex); goto out_close; } } -mutex_lock (&dev->struct_mutex);if (dev->driver->master_set) { ret = dev->driver->master_set (dev, priv, true);if (ret) {/* drop both references if this fails */ drm_master_put (&priv->minor->master);drm_master_put (&priv->master); -mutex_unlock (&dev->struct_mutex); goto out_close; } }
@@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,/* get a reference to the master */ priv->master =drm_master_get (priv->minor->master); } -mutex_unlock (&dev->struct_mutex); +mutex_unlock (&dev->master_mutex);mutex_lock (&dev->struct_mutex);list_add (&priv->lhead, &dev->filelist);mutex_unlock (&dev->struct_mutex);
@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,return 0 ; out_close: +mutex_unlock (&dev->master_mutex);if (dev->driver->postclose) dev->driver->postclose (dev, priv); out_prime_destroy:
@@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp) }mutex_unlock (&dev->ctxlist_mutex); -mutex_lock (&dev->struct_mutex); +mutex_lock (&dev->master_mutex);if (file_priv->is_master) {struct drm_master *master = file_priv->master;struct drm_file *temp; + +mutex_lock (&dev->struct_mutex);list_for_each_entry (temp, &dev->filelist, lhead) {if ((temp->master == file_priv->master) && (temp != file_priv))
@@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv =NULL ;wake_up_interruptible_all (&master->lock.lock_queue); } +mutex_unlock (&dev->struct_mutex);if (file_priv->minor->master == file_priv->master) {/* drop the reference held my the minor */
@@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp) } } -/* drop the reference held my the file priv */ +/* drop the master reference held by the file priv */ if (file_priv->master)drm_master_put (&file_priv->master); file_priv->is_master =0 ; +mutex_unlock (&dev->master_mutex); + +mutex_lock (&dev->struct_mutex);list_del (&file_priv->lhead);mutex_unlock (&dev->struct_mutex);
drivers/gpu/drm/drm_stub.c +24 -14
@@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref)struct drm_device *dev = master->minor->dev;struct drm_map_list *r_list, *list_temp; +mutex_lock (&dev->struct_mutex);if (dev->driver->master_destroy) dev->driver->master_destroy (dev, master);
@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)drm_ht_remove (&master->magiclist); +mutex_unlock (&dev->struct_mutex);kfree (master); }
@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, {int ret =0 ; +mutex_lock (&dev->master_mutex);if (file_priv->is_master) -return 0 ; + goto out_unlock; -if (file_priv->minor->master && file_priv->minor->master != file_priv->master) -return -EINVAL ; + ret = -EINVAL ; +if (file_priv->minor->master) + goto out_unlock;if (!file_priv->master) -return -EINVAL ; + goto out_unlock; -if (file_priv->minor->master) -return -EINVAL ; - -mutex_lock (&dev->struct_mutex); file_priv->minor->master =drm_master_get (file_priv->master); file_priv->is_master =1 ;if (dev->driver->master_set) {
@@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,drm_master_put (&file_priv->minor->master); } } -mutex_unlock (&dev->struct_mutex); +out_unlock: +mutex_unlock (&dev->master_mutex);return ret; }int drm_dropmaster_ioctl (struct drm_device *dev,void *data,struct drm_file *file_priv) { +int ret = -EINVAL ; + +mutex_lock (&dev->master_mutex);if (!file_priv->is_master) -return -EINVAL ; + goto out_unlock;if (!file_priv->minor->master) -return -EINVAL ; + goto out_unlock; -mutex_lock (&dev->struct_mutex); + ret =0 ;if (dev->driver->master_drop) dev->driver->master_drop (dev, file_priv, false);drm_master_put (&file_priv->minor->master); file_priv->is_master =0 ; -mutex_unlock (&dev->struct_mutex); -return 0 ; + +out_unlock: +mutex_unlock (&dev->master_mutex); +return ret; }/*
@@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,spin_lock_init (&dev->event_lock);mutex_init (&dev->struct_mutex);mutex_init (&dev->ctxlist_mutex); +mutex_init (&dev->master_mutex); dev->anon_inode =drm_fs_inode_new ();if (IS_ERR (dev->anon_inode)) {
@@ -620,6 +627,7 @@ err_minors:drm_minor_free (dev,DRM_MINOR_CONTROL );drm_fs_inode_free (dev->anon_inode); err_free: +mutex_destroy (&dev->master_mutex);kfree (dev);return NULL ; }
@@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref)drm_minor_free (dev,DRM_MINOR_CONTROL );kfree (dev->devname); + +mutex_destroy (&dev->master_mutex);kfree (dev); }
include/drm/drmP.h +25 -21
73 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Re: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
Hi
Quoted 3 lines · Thomas Hellstrom
The master management was previously protected by the
drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separatemaster_mutex.
Could you elaborate on that? What exactly is "master_mutex" protecting? "struct_mutex" is used to serialize all entry-points into the drm-device (and thus the driver) and also, often implicitly, as spin-lock for "struct drm_device" data protection.
Regarding master_mutex I have several questions:
- Can you give an example how vmwgfx dead-locks with your reworked code?
- Why don't add a spin-lock to "drm_file" instead? Use that one to
manage master contexts, but keep "struct_mutex" whenever calling into driver callbacks (set_master/drop_master)
- why is master_mutex per device and not per-minor? I thought masters
on minors are _entirely_ independent?
Few more comments inline.
Quoted 29 lines
Signed-off-by Thomas Hellstrom <[email protected]>Reviewed-by Brian Paul <[email protected]>--- drivers/gpu/drm/drm_fops.c | 23 +++++++++++++--------- drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++-------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 44 deletions(-)drivers/gpu/drm/drm_fops.c +3
-2@@ -231,12 +231,13 @@ 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); +mutex_lock (&dev->master_mutex);if (drm_is_legacy_client (priv) && !priv->minor->master) {/* create a new master */ +mutex_lock (&dev->struct_mutex); priv->minor->master =drm_master_create (priv->minor); +mutex_unlock (&dev->struct_mutex);if (!priv->minor->master) { -mutex_unlock (&dev->struct_mutex); ret = -ENOMEM ; goto out_close; }
drivers/gpu/drm/drm_fops.c static int drm_open_helper(struct inode *inode, struct file *filp, · line 252
Quoted 7 lines
drivers/gpu/drm/drm_fops.c +1
-0@@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,/* take another reference for the copy in the local file priv */ priv->master =drm_master_get (priv->minor->master); +mutex_lock (&dev->struct_mutex); priv->authenticated =1 ;mutex_unlock (&dev->struct_mutex);
What's that struct_mutex doing here? We're in ->open(), there is no-one racing against us.
Quoted 222 lines
drivers/gpu/drm/drm_fops.c +10
-7if (dev->driver->master_create) { ret = dev->driver->master_create (dev, priv->master);if (ret) { -mutex_lock (&dev->struct_mutex);/* drop both references if this fails */ drm_master_put (&priv->minor->master);drm_master_put (&priv->master); -mutex_unlock (&dev->struct_mutex); goto out_close; } } -mutex_lock (&dev->struct_mutex);if (dev->driver->master_set) { ret = dev->driver->master_set (dev, priv, true);if (ret) {/* drop both references if this fails */ drm_master_put (&priv->minor->master);drm_master_put (&priv->master); -mutex_unlock (&dev->struct_mutex); goto out_close; } }@@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,/* get a reference to the master */ priv->master =drm_master_get (priv->minor->master); } -mutex_unlock (&dev->struct_mutex); +mutex_unlock (&dev->master_mutex);mutex_lock (&dev->struct_mutex);list_add (&priv->lhead, &dev->filelist);mutex_unlock (&dev->struct_mutex);@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,return 0 ; out_close: +mutex_unlock (&dev->master_mutex);if (dev->driver->postclose) dev->driver->postclose (dev, priv); out_prime_destroy:@@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp) }mutex_unlock (&dev->ctxlist_mutex); -mutex_lock (&dev->struct_mutex); +mutex_lock (&dev->master_mutex);if (file_priv->is_master) {struct drm_master *master = file_priv->master;struct drm_file *temp; + +mutex_lock (&dev->struct_mutex);list_for_each_entry (temp, &dev->filelist, lhead) {if ((temp->master == file_priv->master) && (temp != file_priv))@@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv =NULL ;wake_up_interruptible_all (&master->lock.lock_queue); } +mutex_unlock (&dev->struct_mutex);if (file_priv->minor->master == file_priv->master) {/* drop the reference held my the minor */ @@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp) } } -/* drop the reference held my the file priv */ +/* drop the master reference held by the file priv */ if (file_priv->master)drm_master_put (&file_priv->master); file_priv->is_master =0 ; +mutex_unlock (&dev->master_mutex); + +mutex_lock (&dev->struct_mutex);list_del (&file_priv->lhead);mutex_unlock (&dev->struct_mutex);drivers/gpu/drm/drm_stub.c +24
-14@@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref)struct drm_device *dev = master->minor->dev;struct drm_map_list *r_list, *list_temp; +mutex_lock (&dev->struct_mutex);if (dev->driver->master_destroy) dev->driver->master_destroy (dev, master);@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)drm_ht_remove (&master->magiclist); +mutex_unlock (&dev->struct_mutex);kfree (master); }@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, {int ret =0 ; +mutex_lock (&dev->master_mutex);if (file_priv->is_master) -return 0 ; + goto out_unlock; -if (file_priv->minor->master && file_priv->minor->master != file_priv->master) -return -EINVAL ; + ret = -EINVAL ; +if (file_priv->minor->master) + goto out_unlock;if (!file_priv->master) -return -EINVAL ; + goto out_unlock; -if (file_priv->minor->master) -return -EINVAL ; - -mutex_lock (&dev->struct_mutex); file_priv->minor->master =drm_master_get (file_priv->master); file_priv->is_master =1 ;if (dev->driver->master_set) {@@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,drm_master_put (&file_priv->minor->master); } } -mutex_unlock (&dev->struct_mutex); +out_unlock: +mutex_unlock (&dev->master_mutex);return ret; }int drm_dropmaster_ioctl (struct drm_device *dev,void *data,struct drm_file *file_priv) { +int ret = -EINVAL ; + +mutex_lock (&dev->master_mutex);if (!file_priv->is_master) -return -EINVAL ; + goto out_unlock;if (!file_priv->minor->master) -return -EINVAL ; + goto out_unlock; -mutex_lock (&dev->struct_mutex); + ret =0 ;if (dev->driver->master_drop) dev->driver->master_drop (dev, file_priv, false);drm_master_put (&file_priv->minor->master); file_priv->is_master =0 ; -mutex_unlock (&dev->struct_mutex); -return 0 ; + +out_unlock: +mutex_unlock (&dev->master_mutex); +return ret; }/* @@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,spin_lock_init (&dev->event_lock);mutex_init (&dev->struct_mutex);mutex_init (&dev->ctxlist_mutex); +mutex_init (&dev->master_mutex); dev->anon_inode =drm_fs_inode_new ();if (IS_ERR (dev->anon_inode)) {@@ -620,6 +627,7 @@ err_minors:drm_minor_free (dev,DRM_MINOR_CONTROL );drm_fs_inode_free (dev->anon_inode); err_free: +mutex_destroy (&dev->master_mutex);kfree (dev);return NULL ; }@@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref)drm_minor_free (dev,DRM_MINOR_CONTROL );kfree (dev->devname); + +mutex_destroy (&dev->master_mutex);kfree (dev); }include/drm/drmP.h +15
-2@@ -435,7 +435,8 @@ struct drm_prime_file_private {struct drm_file {unsigned always_authenticated :1 ;unsigned authenticated :1 ; -unsigned is_master :1 ;/* this file private is a master for a minor */ +/* Whether we're master for a minor. Protected by master_mutex */ +unsigned is_master :1 ;/* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1 ;@@ -714,28 +715,29 @@ struct drm_gem_object {#include <drm/drm_crtc.h> -/* per-master structure */ +/** +* struct drm_master - drm master structure +* +* @refcount: Refcount for this master object. +* @minor: Link back to minor char device we are master for. Immutable. +* @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. +* @unique_len: Length of unique field. Protected by drm_global_mutex. +* @unique_size: Amount allocated. Protected by drm_global_mutex. +* @magiclist: Hash of used authentication tokens. Protected by struct_mutex. +* @magicfree: List of used authentication tokens. Protected by struct_mutex. +* @lock: DRI lock information. +* @driver_priv: Pointer to driver-private information. +*/ struct drm_master {
include/drm/drmP.h struct drm_gem_object { · line 728
Quoted 10 lines
include/drm/drmP.h +0
-10- -struct kref refcount;/* refcount for this master */ - -struct drm_minor *minor;/**< link back to minor we are a master for */ - -char *unique;/**< Unique identifier: e.g., busid */ -int unique_len;/**< Length of unique field */ -int unique_size;/**< amount allocated */ - -int blocked;/**< Blocked due to VC switch? */
You silently dropped that field. At least mention it in the commit-message if it's unused.
Quoted 31 lines
include/drm/drmP.h +9
-9- -/** \name Authentication */ -/*@{ */ +struct kref refcount; +struct drm_minor *minor; +char *unique; +int unique_len; +int unique_size;struct drm_open_hash magiclist;struct list_head magicfree; -/*@} */ - -struct drm_lock_data lock;/**< Information on hardware lock */ - -void *driver_priv;/**< Private structure for driver to use */ +struct drm_lock_data lock; +void *driver_priv; };/* Size of ringbuffer for vblank timestamps. Just double-buffer @@ -1050,7 +1052,8 @@ struct drm_minor {struct list_head debugfs_list;struct mutex debugfs_lock;/* Protects debugfs_list. */ -struct drm_master *master;/* currently active master for this node */ +/* currently active master for this node. Protected by master_mutex */ +struct drm_master *master;struct drm_mode_group mode_group; };
include/drm/drmP.h struct drm_device { · line 1106
Quoted 4 lines
include/drm/drmP.h +1
-0@@ -1100,6 +1103,7 @@ struct drm_device {/*@{ */ spinlock_t count_lock;/**< For inuse, drm_device::open_count, drm_device::buf_use */ struct mutex struct_mutex;/**< For others */ +struct mutex master_mutex;
Comments, comments, comments! Lets avoid adding another undocumented mutex here. Or at least mark it as private to drm_master_*() functions.
Thanks
David
Quoted 9 lines
include/drm/drmP.h +0
-1/*@} */ /** \name Usage Counters */ --1.7.10.4
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
Quoted 8 lines · David Herrmann
Hi
Quoted 3 lines · Thomas Hellstrom
The master management was previously protected by the
drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separatemaster_mutex.Could you elaborate on that? What exactly is "
master_mutex" protecting?
Its protecting drm_file::is_master and drm_minor::master, as per inline comments. It's also a candidate for replacing drm_global_mutex to avoid the dropmaster and setmaster ioctls racing when the drm_global_mutex eventually goes away.
Quoted 3 lines
"
struct_mutex" is used to serialize all entry-points into the drm-device (and thus the driver) and also, often implicitly, as spin-lock for "structdrm_device" data protection.
No. DRM locking was added as an after-though, and is a horrendous mess. Nobody really knows what's protecting what, and that has caused a lot of grief in the past. Probably most so for the Intel driver that relied (relies?) on the struct_mutex to protect everything. The drm_global_mutex is used to serialize the non-lock-audited entry points into the drm device. The struct_mutex is used for data protection of most core drm structures and serializing here and there. Modern drivers have no locks held when entering their ioctls. Also we should not confuse mutexes and spinlocks in this context, as they have very different semantics.
Quoted 3 lines
Regarding
master_mutexI have several questions:
- Can you give an example how vmwgfx dead-locks with your reworked code?
Sure. The reworked driver takes the ttm lock in non-exclusive mode before entering an ioctl. Ioctls will then internally take the struct_mutex. The dropmaster and setmaster ioctls would (before this patch) take the struct_mutex and then in the driver code takes the ttm lock in exclusive mode. We would have a lock inversion and a potential deadlock.
Quoted 3 lines
- Why don't add a spin-lock to "
drm_file" instead? Use that one to
manage master contexts, but keep "struct_mutex" whenever calling into driver callbacks (set_master/drop_master)
See above. We can't have a lock in the drm_file structure since it protects drm_minor data. Also, while it might be possible to restructure some code to be able to use spinlocks instead of mutexes I see no reason to. The established locking order now is master_mutex -> ttm_lock -> struct_mutex which means master_mutex must be a mutex.
Quoted 2 lines
- why is
master_mutexper device and not per-minor? I thought masters
on minors are _entirely_ independent?
Because currently there is only one master capable minor per device, so it would be equivalent. And even if there were more, there is no reason to expect any contention and thus a single master_mutex would be fine.
Quoted 35 lines
Few more comments inline.
Quoted 32 lines
Signed-off-by Thomas Hellstrom <[email protected]>Reviewed-by Brian Paul <[email protected]>--- drivers/gpu/drm/drm_fops.c | 23 +++++++++++++--------- drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++-------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 44 deletions(-)drivers/gpu/drm/drm_fops.c +3
-2@@ -231,12 +231,13 @@ 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); +mutex_lock (&dev->master_mutex);if (drm_is_legacy_client (priv) && !priv->minor->master) {/* create a new master */ +mutex_lock (&dev->struct_mutex); priv->minor->master =drm_master_create (priv->minor); +mutex_unlock (&dev->struct_mutex);if (!priv->minor->master) { -mutex_unlock (&dev->struct_mutex); ret = -ENOMEM ; goto out_close; }@@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,/* take another reference for the copy in the local file priv */ priv->master =drm_master_get (priv->minor->master);
Quoted 6 lines
drivers/gpu/drm/drm_fops.c static int drm_open_helper(struct inode *inode, struct file *filp, · line 252
Quoted 4 lines
drivers/gpu/drm/drm_fops.c +1
-0+mutex_lock (&dev->struct_mutex); priv->authenticated =1 ;mutex_unlock (&dev->struct_mutex);What's that
struct_mutexdoing here? We're in ->open(), there is no-one racing against us.
Well, it was held at that point before, and the purpose of this patch is not to generally clean up struct_mutex usage.
Quoted 227 lines
Quoted 227 lines
drivers/gpu/drm/drm_fops.c +10
-7if (dev->driver->master_create) { ret = dev->driver->master_create (dev, priv->master);if (ret) { -mutex_lock (&dev->struct_mutex);/* drop both references if this fails */ drm_master_put (&priv->minor->master);drm_master_put (&priv->master); -mutex_unlock (&dev->struct_mutex); goto out_close; } } -mutex_lock (&dev->struct_mutex);if (dev->driver->master_set) { ret = dev->driver->master_set (dev, priv, true);if (ret) {/* drop both references if this fails */ drm_master_put (&priv->minor->master);drm_master_put (&priv->master); -mutex_unlock (&dev->struct_mutex); goto out_close; } }@@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,/* get a reference to the master */ priv->master =drm_master_get (priv->minor->master); } -mutex_unlock (&dev->struct_mutex); +mutex_unlock (&dev->master_mutex);mutex_lock (&dev->struct_mutex);list_add (&priv->lhead, &dev->filelist);mutex_unlock (&dev->struct_mutex);@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,return 0 ; out_close: +mutex_unlock (&dev->master_mutex);if (dev->driver->postclose) dev->driver->postclose (dev, priv); out_prime_destroy:@@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp) }mutex_unlock (&dev->ctxlist_mutex); -mutex_lock (&dev->struct_mutex); +mutex_lock (&dev->master_mutex);if (file_priv->is_master) {struct drm_master *master = file_priv->master;struct drm_file *temp; + +mutex_lock (&dev->struct_mutex);list_for_each_entry (temp, &dev->filelist, lhead) {if ((temp->master == file_priv->master) && (temp != file_priv))@@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv =NULL ;wake_up_interruptible_all (&master->lock.lock_queue); } +mutex_unlock (&dev->struct_mutex);if (file_priv->minor->master == file_priv->master) {/* drop the reference held my the minor */ @@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp) } } -/* drop the reference held my the file priv */ +/* drop the master reference held by the file priv */ if (file_priv->master)drm_master_put (&file_priv->master); file_priv->is_master =0 ; +mutex_unlock (&dev->master_mutex); + +mutex_lock (&dev->struct_mutex);list_del (&file_priv->lhead);mutex_unlock (&dev->struct_mutex);drivers/gpu/drm/drm_stub.c +24
-14@@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref)struct drm_device *dev = master->minor->dev;struct drm_map_list *r_list, *list_temp; +mutex_lock (&dev->struct_mutex);if (dev->driver->master_destroy) dev->driver->master_destroy (dev, master);@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)drm_ht_remove (&master->magiclist); +mutex_unlock (&dev->struct_mutex);kfree (master); }@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, {int ret =0 ; +mutex_lock (&dev->master_mutex);if (file_priv->is_master) -return 0 ; + goto out_unlock; -if (file_priv->minor->master && file_priv->minor->master != file_priv->master) -return -EINVAL ; + ret = -EINVAL ; +if (file_priv->minor->master) + goto out_unlock;if (!file_priv->master) -return -EINVAL ; + goto out_unlock; -if (file_priv->minor->master) -return -EINVAL ; - -mutex_lock (&dev->struct_mutex); file_priv->minor->master =drm_master_get (file_priv->master); file_priv->is_master =1 ;if (dev->driver->master_set) {@@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,drm_master_put (&file_priv->minor->master); } } -mutex_unlock (&dev->struct_mutex); +out_unlock: +mutex_unlock (&dev->master_mutex);return ret; }int drm_dropmaster_ioctl (struct drm_device *dev,void *data,struct drm_file *file_priv) { +int ret = -EINVAL ; + +mutex_lock (&dev->master_mutex);if (!file_priv->is_master) -return -EINVAL ; + goto out_unlock;if (!file_priv->minor->master) -return -EINVAL ; + goto out_unlock; -mutex_lock (&dev->struct_mutex); + ret =0 ;if (dev->driver->master_drop) dev->driver->master_drop (dev, file_priv, false);drm_master_put (&file_priv->minor->master); file_priv->is_master =0 ; -mutex_unlock (&dev->struct_mutex); -return 0 ; + +out_unlock: +mutex_unlock (&dev->master_mutex); +return ret; }/* @@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,spin_lock_init (&dev->event_lock);mutex_init (&dev->struct_mutex);mutex_init (&dev->ctxlist_mutex); +mutex_init (&dev->master_mutex); dev->anon_inode =drm_fs_inode_new ();if (IS_ERR (dev->anon_inode)) {@@ -620,6 +627,7 @@ err_minors:drm_minor_free (dev,DRM_MINOR_CONTROL );drm_fs_inode_free (dev->anon_inode); err_free: +mutex_destroy (&dev->master_mutex);kfree (dev);return NULL ; }@@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref)drm_minor_free (dev,DRM_MINOR_CONTROL );kfree (dev->devname); + +mutex_destroy (&dev->master_mutex);kfree (dev); }include/drm/drmP.h +15
-6@@ -435,7 +435,8 @@ struct drm_prime_file_private {struct drm_file {unsigned always_authenticated :1 ;unsigned authenticated :1 ; -unsigned is_master :1 ;/* this file private is a master for a minor */ +/* Whether we're master for a minor. Protected by master_mutex */ +unsigned is_master :1 ;/* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1 ;@@ -714,28 +715,29 @@ struct drm_gem_object {#include <drm/drm_crtc.h> -/* per-master structure */ +/** +* struct drm_master - drm master structure +* +* @refcount: Refcount for this master object. +* @minor: Link back to minor char device we are master for. Immutable. +* @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. +* @unique_len: Length of unique field. Protected by drm_global_mutex. +* @unique_size: Amount allocated. Protected by drm_global_mutex. +* @magiclist: Hash of used authentication tokens. Protected by struct_mutex. +* @magicfree: List of used authentication tokens. Protected by struct_mutex. +* @lock: DRI lock information. +* @driver_priv: Pointer to driver-private information. +*/ struct drm_master { - -struct kref refcount;/* refcount for this master */ - -struct drm_minor *minor;/**< link back to minor we are a master for */
Quoted 8 lines
include/drm/drmP.h struct drm_gem_object { · line 728
Quoted 6 lines
include/drm/drmP.h +0
-6- -char *unique;/**< Unique identifier: e.g., busid */ -int unique_len;/**< Length of unique field */ -int unique_size;/**< amount allocated */ - -int blocked;/**< Blocked due to VC switch? */ You silently dropped that field. At least mention it in the commit-message if it's unused.
Sure.
Quoted 31 lines
Quoted 31 lines
include/drm/drmP.h +9
-9- -/** \name Authentication */ -/*@{ */ +struct kref refcount; +struct drm_minor *minor; +char *unique; +int unique_len; +int unique_size;struct drm_open_hash magiclist;struct list_head magicfree; -/*@} */ - -struct drm_lock_data lock;/**< Information on hardware lock */ - -void *driver_priv;/**< Private structure for driver to use */ +struct drm_lock_data lock; +void *driver_priv; };/* Size of ringbuffer for vblank timestamps. Just double-buffer @@ -1050,7 +1052,8 @@ struct drm_minor {struct list_head debugfs_list;struct mutex debugfs_lock;/* Protects debugfs_list. */ -struct drm_master *master;/* currently active master for this node */ +/* currently active master for this node. Protected by master_mutex */ +struct drm_master *master;struct drm_mode_group mode_group; };
Quoted 8 lines
include/drm/drmP.h struct drm_device { · line 1106
Quoted 5 lines
include/drm/drmP.h +1
-0@@ -1100,6 +1103,7 @@ struct drm_device {/*@{ */ spinlock_t count_lock;/**< For inuse, drm_device::open_count, drm_device::buf_use */ struct mutex struct_mutex;/**< For others */ +struct mutex master_mutex;Comments, comments, comments! Lets avoid adding another undocumented mutex here. Or at least mark it as private to drm_master_*() functions.
Sure.
Quoted 13 lines
Thanks
DavidQuoted 9 lines
include/drm/drmP.h +0
-1/*@} */ /** \name Usage Counters */ --1.7.10.4
_______________________________________________
dri-devel mailing list
[email protected]
https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/dri-devel&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=aQtwaWHv2a0%2B862ao5fkuBGGYVf%2B4WRqrDEgHLSsyzI%3D%0A&s=f27e1a390d6150945a05ccdde58cdd68df313e1b8b255911c3380b60424bba98
Thanks,
Thomas
Re: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
Quoted 5 lines · Thomas Hellstrom
Quoted 3 lines · David Herrmann
"
struct_mutex" is used to serialize all entry-points into the drm-device (and thus the driver) and also, often implicitly, as spin-lock for "structdrm_device" data protection.
Quoted 10 lines
No. DRM locking was added as an after-though, and is a horrendous mess. Nobody really knows what's protecting what, and that has caused a lot of grief in the past. Probably most so for the Intel driver that relied (relies?) on the
struct_mutexto protect everything. Thedrm_global_mutexis used to serialize the non-lock-audited entry points into the drm device. Thestruct_mutexis used for data protection of most core drm structures and serializing here and there. Modern drivers have no locks held when entering their ioctls. Also we should not confuse mutexes and spinlocks in this context, as they have very different semantics.
As the guy who gets to live the locking mess called dev->struct_mutex I holeheartedly welcome any efforts to split out clear subparts away from it. I actually had this very idea of adding a master-data related mutex on my todo. I'll try to review this later if I get around, but definitely Acked!
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
In reply to Thomas HellstromHi
Quoted 9 lines · Thomas Hellstrom
Quoted 3 lines
- Why don't add a spin-lock to "
drm_file" instead? Use that one to
manage master contexts, but keep "struct_mutex" whenever calling into driver callbacks (set_master/drop_master)See above. We can't have a lock in the
drm_filestructure since it protectsdrm_minordata. Also, while it might be possible to restructure some code to be able to use spinlocks instead of mutexes I see no reason to. The established locking order now ismaster_mutex->ttm_lock->struct_mutexwhich meansmaster_mutexmust be a mutex.
Thanks, that order really helps understanding what these locks do. More, actually, than the commit message ;) It also shows how awful struct_mutex is.. Using it as data-protection and execution-sync is really weird. So I'm all for doing more fine-grained locking if it's as simple as with the master-stuff.
Quoted 6 lines
Quoted 2 lines
- why is
master_mutexper device and not per-minor? I thought masters
on minors are _entirely_ independent?How do multiple keysymsBecause currently there is only one master capable minor per device, so it would be equivalent. And even if there were more, there is no reason to expect any contention and thus a single
master_mutexwould be fine.
Fair enough.
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/gpu/drm/drm_fops.c +4
-2@@ -231,12 +231,13 @@ 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); +mutex_lock (&dev->master_mutex);if (drm_is_legacy_client (priv) && !priv->minor->master) {/* create a new master */ +mutex_lock (&dev->struct_mutex); priv->minor->master =drm_master_create (priv->minor); +mutex_unlock (&dev->struct_mutex);if (!priv->minor->master) { -mutex_unlock (&dev->struct_mutex); ret = -ENOMEM ; goto out_close; }@@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,/* take another reference for the copy in the local file priv */ priv->master =drm_master_get (priv->minor->master); +mutex_lock (&dev->struct_mutex); priv->authenticated =1 ;
Quoted 6 lines
Quoted 3 lines
drivers/gpu/drm/drm_fops.c static int drm_open_helper(struct inode *inode, struct file *filp, · line 252
Quoted 1 line
drivers/gpu/drm/drm_fops.c +0
-0mutex_unlock (&dev->struct_mutex);What's that
struct_mutexdoing here? We're in ->open(), there is no-one racing against us.Well, it was held at that point before, and the purpose of this patch is not to generally clean up
struct_mutexusage.
Well, it now looks like this:
mutex_lock(&dev->struct_mutex);priv->authenticated = 1;mutex_unlock(&dev->struct_mutex);
Which looks so _wrong_ that I thought we should fix it right away. But ok, I'm not going to force you to do so.
Quick lock-review:
* drm_authmagic() uses drm_global_mutex to protect setting priv->authenticated (racing against us..)
* current context is ->open() so no-one has access to "priv". We haven't even linked it to dev->filelist, but we called into the driver which might have done anything..
* drm-fops read ->authenticated _unlocked_, so no reason at all to do an explicit locking around a _single_ write (which is only needed in very rare cases, anyway)
* it is never set to anything else but 1; a _single_ barrier after setting it should be enough
In case you don't want to incorporate that, I will send a cleanup.
Would be nice to have the mutex-locking in drm-next to get some testing. v2 looks good, I haven't done a thorough locking review, though. But I guess you did, so feel free to include it in your pull. Thanks
David
PATCH 7/16 drm: Add a function to get the ioctl flags
In reply to Thomas Hellstrom--- drivers/gpu/drm/drm_drv.c | 18 ++++++++++++++++++ include/drm/drmP.h | 1 + 2 files changed, 19 insertions(+)
drivers/gpu/drm/drm_drv.c +18 -0
@@ -450,3 +450,21 @@ long drm_ioctl(struct file *filp,return retcode; }EXPORT_SYMBOL (drm_ioctl); + +/** +* drm_ioctl_flags - Check for core ioctl and return ioctl permission flags +* +* @nr: Ioctl number. +* @flags: Where to return the ioctl permission flags +*/ +bool drm_ioctl_flags (unsigned int nr,unsigned int *flags) +{ +if ((nr >=DRM_COMMAND_END && nr <DRM_CORE_IOCTL_COUNT ) || + (nr <DRM_COMMAND_BASE )) { + *flags = drm_ioctls[nr].flags; +return true; + } + +return false; +} +EXPORT_SYMBOL (drm_ioctl_flags);
include/drm/drmP.h +1 -0
@@ -1259,6 +1259,7 @@ extern long drm_ioctl(struct file *filp,extern long drm_compat_ioctl (struct file *filp,unsigned int cmd,unsigned long arg);extern int drm_lastclose (struct drm_device *dev); +extern bool drm_ioctl_flags (unsigned int nr,unsigned int *flags);/* Device support (drm_fops.h) */ extern struct mutex drm_global_mutex;
Signature · 1 line
1.7.10.4
PATCH 8/16 drm/vmwgfx: Use a per-device semaphore for reservation protection
In reply to Thomas HellstromDon't use a per-master semaphore (ttm lock) for reservation protection, but rather a per-device semaphore. This is needed since clients connecting using render nodes aren't master aware.
The ttm lock used should probably be replaced with a reader-write semaphore once the function down_xx_interruptible() is available.
--- drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c | 15 ++++++--------- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 5 +++++ drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 9 ++++----- drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 10 ++++------ drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++++++--------- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 15 ++++++--------- drivers/gpu/drm/vmwgfx/vmwgfx_shader.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 14 ++++++-------- 11 files changed, 46 insertions(+), 58 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_context.c +2 -3
@@ -462,7 +462,6 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data,struct vmw_resource *tmp;struct drm_vmw_context_arg *arg = (struct drm_vmw_context_arg *)data;struct ttm_object_file *tfile =vmw_fpriv (file_priv)->tfile; -struct vmw_master *vmaster =vmw_master (file_priv->master);int ret;
@@ -474,7 +473,7 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data,if (unlikely (vmw_user_context_size ==0 )) vmw_user_context_size =ttm_round_pot (sizeof (*ctx)) +128 ; - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -521,7 +520,7 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data, out_err:vmw_resource_unreference (&res); out_unlock: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c +6 -9
@@ -52,11 +52,10 @@ int vmw_dmabuf_to_placement(struct vmw_private *dev_priv, struct ttm_placement *placement, bool interruptible) { -struct vmw_master *vmaster = dev_priv->active_master;struct ttm_buffer_object *bo = &buf->base;int ret; - ret =ttm_write_lock (&vmaster->lock, interruptible); + ret =ttm_write_lock (&dev_priv->reservation_sem, interruptible);if (unlikely (ret !=0 ))return ret;
@@ -71,7 +70,7 @@ int vmw_dmabuf_to_placement(struct vmw_private *dev_priv,ttm_bo_unreserve (bo); err: -ttm_write_unlock (&vmaster->lock); +ttm_write_unlock (&dev_priv->reservation_sem);return ret; }
@@ -95,12 +94,11 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv,struct vmw_dma_buffer *buf, bool pin, bool interruptible) { - struct vmw_master *vmaster = dev_priv->active_master; struct ttm_buffer_object *bo = &buf->base;struct ttm_placement *placement;int ret; - ret =ttm_write_lock (&vmaster->lock, interruptible); + ret =ttm_write_lock (&dev_priv->reservation_sem, interruptible);if (unlikely (ret !=0 ))return ret;
@@ -143,7 +141,7 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv, err_unreserve:ttm_bo_unreserve (bo); err: -ttm_write_unlock (&vmaster->lock); +ttm_write_unlock (&dev_priv->reservation_sem);return ret; }
@@ -198,7 +196,6 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv,struct vmw_dma_buffer *buf, bool pin, bool interruptible) { - struct vmw_master *vmaster = dev_priv->active_master; struct ttm_buffer_object *bo = &buf->base;struct ttm_placement placement;int ret =0 ;
@@ -209,7 +206,7 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv, placement = vmw_vram_placement; placement.lpfn = bo->num_pages; - ret =ttm_write_lock (&vmaster->lock, interruptible); + ret =ttm_write_lock (&dev_priv->reservation_sem, interruptible);if (unlikely (ret !=0 ))return ret;
@@ -232,7 +229,7 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv,ttm_bo_unreserve (bo); err_unlock: -ttm_write_unlock (&vmaster->lock); +ttm_write_unlock (&dev_priv->reservation_sem);return ret; }
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +3 -3
@@ -606,6 +606,7 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)mutex_init (&dev_priv->release_mutex);mutex_init (&dev_priv->binding_mutex);rwlock_init (&dev_priv->resource_lock); +ttm_lock_init (&dev_priv->reservation_sem);for (i = vmw_res_context; i < vmw_res_max; ++i) {idr_init (&dev_priv->res_idr[i]);
@@ -1175,12 +1176,11 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val, {struct vmw_private *dev_priv = container_of(nb, struct vmw_private, pm_nb); -struct vmw_master *vmaster = dev_priv->active_master;switch (val) {case PM_HIBERNATION_PREPARE :case PM_SUSPEND_PREPARE : -ttm_suspend_lock (&vmaster->lock); +ttm_suspend_lock (&dev_priv->reservation_sem);/** * This empties VRAM and unbinds all GMR bindings.
@@ -1194,7 +1194,7 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val,case PM_POST_HIBERNATION :case PM_POST_SUSPEND :case PM_POST_RESTORE : -ttm_suspend_unlock (&vmaster->lock); +ttm_suspend_unlock (&dev_priv->reservation_sem);break ;case PM_RESTORE_PREPARE :
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +5 -0
@@ -487,6 +487,11 @@ struct vmw_private {uint32_t num_3d_resources;/* +* Replace this with an rwsem as soon as we have down_xx_interruptible() +*/ +struct ttm_lock reservation_sem; + +/* * Query processing. These members * are protected by the cmdbuf mutex. */
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +2 -3
@@ -2712,7 +2712,6 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data, {struct vmw_private *dev_priv =vmw_priv (dev);struct drm_vmw_execbuf_arg *arg = (struct drm_vmw_execbuf_arg *)data; -struct vmw_master *vmaster =vmw_master (file_priv->master);int ret;/*
@@ -2729,7 +2728,7 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data,return -EINVAL ; } - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -2745,6 +2744,6 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data,vmw_kms_cursor_post_execbuf (dev_priv); out_unlock: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
drivers/gpu/drm/vmwgfx/vmwgfx_fb.c +4 -5
@@ -377,14 +377,13 @@ static int vmw_fb_create_bo(struct vmw_private *vmw_priv, ne_placement.lpfn = (size +PAGE_SIZE -1 ) >>PAGE_SHIFT ; -/* interuptable? */ - ret =ttm_write_lock (&vmw_priv->fbdev_master.lock, false); -if (unlikely (ret !=0 )) -return ret; + (void )ttm_write_lock (&vmw_priv->reservation_sem, false); vmw_bo =kmalloc (sizeof (*vmw_bo),GFP_KERNEL ); -if (!vmw_bo) +if (!vmw_bo) { + ret = -ENOMEM ; goto err_unlock; + } ret = vmw_dmabuf_init(vmw_priv, vmw_bo, size, &ne_placement,
drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c +4 -6
@@ -226,7 +226,6 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,struct drm_vmw_present_arg *arg = (struct drm_vmw_present_arg *)data;struct vmw_surface *surface; -struct vmw_master *vmaster =vmw_master (file_priv->master);struct drm_vmw_rect __user *clips_ptr;struct drm_vmw_rect *clips =NULL ;struct drm_framebuffer *fb;
@@ -271,7 +270,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data, } vfb =vmw_framebuffer_to_vfb (fb); - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 )) goto out_no_ttm_lock;
@@ -291,7 +290,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,vmw_surface_unreference (&surface); out_no_surface: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem); out_no_ttm_lock:drm_framebuffer_unreference (fb); out_no_fb:
@@ -311,7 +310,6 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,struct drm_vmw_fence_rep __user *user_fence_rep = (struct drm_vmw_fence_rep __user *) (unsigned long )arg->fence_rep; -struct vmw_master *vmaster =vmw_master (file_priv->master);struct drm_vmw_rect __user *clips_ptr;struct drm_vmw_rect *clips =NULL ;struct drm_framebuffer *fb;
@@ -361,7 +359,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data, goto out_no_ttm_lock; } - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 )) goto out_no_ttm_lock;
@@ -369,7 +367,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data, vfb, user_fence_rep, clips, num_clips); -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem); out_no_ttm_lock:drm_framebuffer_unreference (fb); out_no_fb:
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +6 -9
@@ -596,7 +596,6 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer, unsigned num_clips) {struct vmw_private *dev_priv =vmw_priv (framebuffer->dev); -struct vmw_master *vmaster =vmw_master (file_priv->master);struct vmw_framebuffer_surface *vfbs =vmw_framebuffer_to_vfbs (framebuffer);struct drm_clip_rect norect;
@@ -611,7 +610,7 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,drm_modeset_lock_all (dev_priv->dev); - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 )) {drm_modeset_unlock_all (dev_priv->dev);return ret;
@@ -632,7 +631,7 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer, flags, color, clips, num_clips, inc, NULL); -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);drm_modeset_unlock_all (dev_priv->dev);
@@ -954,7 +953,6 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer, unsigned num_clips) {struct vmw_private *dev_priv =vmw_priv (framebuffer->dev); -struct vmw_master *vmaster =vmw_master (file_priv->master);struct vmw_framebuffer_dmabuf *vfbd =vmw_framebuffer_to_vfbd (framebuffer);struct drm_clip_rect norect;
@@ -962,7 +960,7 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,drm_modeset_lock_all (dev_priv->dev); - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 )) {drm_modeset_unlock_all (dev_priv->dev);return ret;
@@ -989,7 +987,7 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer, clips, num_clips, increment, NULL); } -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);drm_modeset_unlock_all (dev_priv->dev);
@@ -2022,7 +2020,6 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,struct vmw_private *dev_priv =vmw_priv (dev);struct drm_vmw_update_layout_arg *arg = (struct drm_vmw_update_layout_arg *)data; -struct vmw_master *vmaster =vmw_master (file_priv->master);void __user *user_rects;struct drm_vmw_rect *rects;unsigned rects_size;
@@ -2030,7 +2027,7 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,int i;struct drm_mode_config *mode_config = &dev->mode_config; - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -2072,6 +2069,6 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data, out_free:kfree (rects); out_unlock: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +6 -9
@@ -676,10 +676,9 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,struct drm_vmw_dmabuf_rep *rep = &arg->rep;struct vmw_dma_buffer *dma_buf;uint32_t handle; -struct vmw_master *vmaster =vmw_master (file_priv->master);int ret; - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -696,7 +695,7 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,vmw_dmabuf_unreference (&dma_buf); out_no_dmabuf: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
@@ -873,7 +872,6 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data,struct vmw_resource *tmp;struct drm_vmw_stream_arg *arg = (struct drm_vmw_stream_arg *)data;struct ttm_object_file *tfile =vmw_fpriv (file_priv)->tfile; -struct vmw_master *vmaster =vmw_master (file_priv->master);int ret;/*
@@ -884,7 +882,7 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data,if (unlikely (vmw_user_stream_size ==0 )) vmw_user_stream_size =ttm_round_pot (sizeof (*stream)) +128 ; - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -932,7 +930,7 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data, out_err:vmw_resource_unreference (&res); out_unlock: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
@@ -985,14 +983,13 @@ int vmw_dumb_create(struct drm_file *file_priv, struct drm_mode_create_dumb *args) {struct vmw_private *dev_priv =vmw_priv (dev); -struct vmw_master *vmaster =vmw_master (file_priv->master);struct vmw_dma_buffer *dma_buf;int ret; args->pitch = args->width * ((args->bpp +7 ) /8 ); args->size = args->pitch * args->height; - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -1004,7 +1001,7 @@ int vmw_dumb_create(struct drm_file *file_priv,vmw_dmabuf_unreference (&dma_buf); out_no_dmabuf: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
drivers/gpu/drm/vmwgfx/vmwgfx_shader.c +2 -3
@@ -449,7 +449,6 @@ int vmw_shader_define_ioctl(struct drm_device *dev, void *data,struct drm_vmw_shader_create_arg *arg = (struct drm_vmw_shader_create_arg *)data;struct ttm_object_file *tfile =vmw_fpriv (file_priv)->tfile; -struct vmw_master *vmaster =vmw_master (file_priv->master);struct vmw_dma_buffer *buffer =NULL ;SVGA3dShaderType shader_type;int ret;
@@ -487,14 +486,14 @@ int vmw_shader_define_ioctl(struct drm_device *dev, void *data, goto out_bad_arg; } - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 )) goto out_bad_arg; ret =vmw_shader_alloc (dev_priv, buffer, arg->size, arg->offset, shader_type, tfile, &arg->shader_handle); -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem); out_bad_arg:vmw_dmabuf_unreference (&buffer);return ret;
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +6 -8
@@ -697,7 +697,6 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,struct vmw_surface_offset *cur_offset;uint32_t num_sizes;uint32_t size; -struct vmw_master *vmaster =vmw_master (file_priv->master);const struct svga3d_surface_desc *desc;if (unlikely (vmw_user_surface_size ==0 ))
@@ -723,7 +722,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,return -EINVAL ; } - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -862,7 +861,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, rep->sid = user_srf->prime.base.hash.key;vmw_resource_unreference (&res); -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return 0 ; out_no_copy:kfree (srf->offsets);
@@ -873,7 +872,7 @@ out_no_sizes: out_no_user_srf:ttm_mem_global_free (vmw_mem_glob (dev_priv), size); out_unlock: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
@@ -1173,7 +1172,6 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,struct ttm_object_file *tfile =vmw_fpriv (file_priv)->tfile;int ret;uint32_t size; -struct vmw_master *vmaster =vmw_master (file_priv->master);const struct svga3d_surface_desc *desc;uint32_t backup_handle;
@@ -1189,7 +1187,7 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,return -EINVAL ; } - ret =ttm_read_lock (&vmaster->lock, true); + ret =ttm_read_lock (&dev_priv->reservation_sem, true);if (unlikely (ret !=0 ))return ret;
@@ -1283,12 +1281,12 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,vmw_resource_unreference (&res); -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return 0 ; out_no_user_srf:ttm_mem_global_free (vmw_mem_glob (dev_priv), size); out_unlock: -ttm_read_unlock (&vmaster->lock); +ttm_read_unlock (&dev_priv->reservation_sem);return ret; }
Signature · 1 line
1.7.10.4
PATCH 9/16 drm/vmwgfx: Reinstate and tighten security around legacy master model
In reply to Thomas HellstromThe following restrictions affect clients connecting using legacy nodes:
*) Masters that have dropped master privilieges are not considered
authenticated until they regain master privileges.
*) Clients whose master have dropped master privileges block interruptibly on
ioctls requiring authentication until their master regains master privileges. If their master exits, they are killed.
This is primarily designed to prevent clients authenticated with one master to access data from clients authenticated with another master. (Think fast user-switching or data sniffers enabled while X is vt-switched).
--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 94 +++++++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 5 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +89 -5
@@ -982,12 +982,70 @@ out_no_tfile:return ret; } -static long vmw_unlocked_ioctl (struct file *filp,unsigned int cmd, -unsigned long arg) +static struct vmw_master *vmw_master_check (struct drm_device *dev, +struct drm_file *file_priv, +unsigned int flags) +{ +int ret; +struct vmw_fpriv *vmw_fp =vmw_fpriv (file_priv); +struct vmw_master *vmaster; + +if (file_priv->minor->type !=DRM_MINOR_LEGACY || + !(flags &DRM_AUTH )) +return NULL ; + + ret =mutex_lock_interruptible (&dev->master_mutex); +if (unlikely (ret !=0 )) +return ERR_PTR (-ERESTARTSYS ); + +if (file_priv->is_master) { +mutex_unlock (&dev->master_mutex); +return NULL ; + } + +/* +* Check if we were previously master, but now dropped. +*/ +if (vmw_fp->locked_master) { +mutex_unlock (&dev->master_mutex); +DRM_ERROR ("Dropped master trying to access ioctl that " +"requires authentication.\n" ); +return ERR_PTR (-EACCES ); + } +mutex_unlock (&dev->master_mutex); + +/* +* Taking the drm_global_mutex after the TTM lock might deadlock +*/ +if (!(flags &DRM_UNLOCKED )) { +DRM_ERROR ("Refusing locked ioctl access.\n" ); +return ERR_PTR (-EDEADLK ); + } + +/* +* Take the TTM lock. Possibly sleep waiting for the authenticating +* master to become master again, or for a SIGTERM if the +* authenticating master exits. +*/ + vmaster =vmw_master (file_priv->master); + ret =ttm_read_lock (&vmaster->lock, true); +if (unlikely (ret !=0 )) + vmaster =ERR_PTR (ret); + +return vmaster; +} + +static long vmw_generic_ioctl (struct file *filp,unsigned int cmd, +unsigned long arg, +long (*ioctl_func)(struct file *,unsigned int , +unsigned long )) {struct drm_file *file_priv = filp->private_data;struct drm_device *dev = file_priv->minor->dev;unsigned int nr =DRM_IOCTL_NR (cmd); +struct vmw_master *vmaster; +unsigned int flags; +long ret;/* * Do extra checking on driver private ioctls.
@@ -996,18 +1054,44 @@ static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd,if ((nr >=DRM_COMMAND_BASE ) && (nr <DRM_COMMAND_END ) && (nr <DRM_COMMAND_BASE + dev->driver->num_ioctls)) {const struct drm_ioctl_desc *ioctl = - &vmw_ioctls[nr -DRM_COMMAND_BASE ]; + &vmw_ioctls[nr -DRM_COMMAND_BASE ];if (unlikely (ioctl->cmd_drv != cmd)) {DRM_ERROR ("Invalid command format, ioctl %d\n" , nr -DRM_COMMAND_BASE );return -EINVAL ; } + flags = ioctl->flags; + }else if (!drm_ioctl_flags (nr, &flags)) +return -EINVAL ; + + vmaster =vmw_master_check (dev, file_priv, flags); +if (unlikely (IS_ERR (vmaster))) { +DRM_INFO ("IOCTL ERROR %d\n" , nr); +return PTR_ERR (vmaster); } -return drm_ioctl (filp, cmd, arg); + ret =ioctl_func (filp, cmd, arg); +if (vmaster) +ttm_read_unlock (&vmaster->lock); + +return ret; +} + +static long vmw_unlocked_ioctl (struct file *filp,unsigned int cmd, +unsigned long arg) +{ +return vmw_generic_ioctl (filp, cmd, arg, &drm_ioctl); } +#ifdef CONFIG_COMPAT +static long vmw_compat_ioctl (struct file *filp,unsigned int cmd, +unsigned long arg) +{ +return vmw_generic_ioctl (filp, cmd, arg, &drm_compat_ioctl); +} +#endif +static void vmw_lastclose (struct drm_device *dev) {struct drm_crtc *crtc;
@@ -1315,7 +1399,7 @@ static const struct file_operations vmwgfx_driver_fops = { .poll = vmw_fops_poll, .read = vmw_fops_read, #if defined(CONFIG_COMPAT) - .compat_ioctl = drm_compat_ioctl, + .compat_ioctl = vmw_compat_ioctl, #endif .llseek = noop_llseek, };
Signature · 1 line
1.7.10.4
PATCH 10/16 drm/vmwgfx: Drop authentication requirement on UNREF ioctls
In reply to Thomas HellstromThese ioctls will anyway only succeed if the client previously opened referenced the object. Furthermore, closing the client would implicitly execute the same action. This prevents clients from blocking on UNREF if their master dropped, and will allow masters to UNREF after dropping master privileges.
--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +5 -5
@@ -146,7 +146,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {VMW_IOCTL_DEF (VMW_ALLOC_DMABUF , vmw_dmabuf_alloc_ioctl,DRM_AUTH |DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_UNREF_DMABUF , vmw_dmabuf_unref_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_CURSOR_BYPASS , vmw_kms_cursor_bypass_ioctl, DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED),
@@ -161,11 +161,11 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {VMW_IOCTL_DEF (VMW_CREATE_CONTEXT , vmw_context_define_ioctl,DRM_AUTH |DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_UNREF_CONTEXT , vmw_context_destroy_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_CREATE_SURFACE , vmw_surface_define_ioctl,DRM_AUTH |DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_UNREF_SURFACE , vmw_surface_destroy_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_REF_SURFACE , vmw_surface_reference_ioctl,DRM_AUTH |DRM_UNLOCKED ), VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl,
@@ -176,7 +176,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { vmw_fence_obj_signaled_ioctl, DRM_AUTH | DRM_UNLOCKED),VMW_IOCTL_DEF (VMW_FENCE_UNREF , vmw_fence_obj_unref_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_FENCE_EVENT , vmw_fence_event_ioctl, DRM_AUTH | DRM_UNLOCKED),
@@ -197,7 +197,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_AUTH | DRM_UNLOCKED),VMW_IOCTL_DEF (VMW_UNREF_SHADER , vmw_shader_destroy_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_UNLOCKED ),VMW_IOCTL_DEF (VMW_GB_SURFACE_CREATE , vmw_gb_surface_define_ioctl, DRM_AUTH | DRM_UNLOCKED),
Signature · 1 line
1.7.10.4
PATCH 11/16 drm/vmwgfx: Allow prime fds in the surface reference ioctls
In reply to Thomas HellstromAllow prime fds and at the same time block legacy handles for render-nodes in the surface reference ioctls. This means these ioctls can be used directly from prime-aware clients, and that they can be called from render-nodes.
--- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 105 ++++++++++++++++++++----------- include/uapi/drm/vmwgfx_drm.h | 12 +++- 2 files changed, 81 insertions(+), 36 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +70 -35
@@ -876,6 +876,64 @@ out_unlock:return ret; } + +static int +vmw_surface_handle_reference (struct vmw_private *dev_priv, +struct drm_file *file_priv, +uint32_t u_handle, +enum drm_vmw_handle_type handle_type, +struct ttm_base_object **base_p) +{ +struct ttm_object_file *tfile =vmw_fpriv (file_priv)->tfile; +uint32_t handle; +struct ttm_base_object *base; +int ret; + +if (handle_type ==DRM_VMW_HANDLE_PRIME ) { + ret =ttm_prime_fd_to_handle (tfile, u_handle, &handle); +if (unlikely (ret !=0 )) +return ret; + }else { +if (unlikely (drm_is_render_client (file_priv))) { +DRM_ERROR ("Render client refused legacy " +"surface reference.\n" ); +return -EACCES ; + } + handle = u_handle; + } + + ret = -EINVAL ; + base =ttm_base_object_lookup_for_ref (dev_priv->tdev, handle); +if (unlikely (base ==NULL )) { +DRM_ERROR ("Could not find surface to reference.\n" ); + goto out_no_lookup; + } + +if (unlikely (ttm_base_object_type (base) !=VMW_RES_SURFACE )) { +DRM_ERROR ("Referenced object is not a surface.\n" ); + goto out_bad_resource; + } + +if (handle_type !=DRM_VMW_HANDLE_PRIME ) { + ret =ttm_ref_object_add (tfile, base,TTM_REF_USAGE ,NULL ); +if (unlikely (ret !=0 )) { +DRM_ERROR ("Could not add a reference to a surface.\n" ); + goto out_bad_resource; + } + } + + *base_p = base; +return 0 ; + +out_bad_resource: +ttm_base_object_unref (&base); +out_no_lookup: +if (handle_type ==DRM_VMW_HANDLE_PRIME ) + (void )ttm_ref_object_base_unref (tfile, handle,TTM_REF_USAGE ); + +return ret; +} +/** * vmw_user_surface_define_ioctl - Ioctl function implementing * the user surface reference functionality.
@@ -897,27 +955,16 @@ int vmw_surface_reference_ioctl(struct drm_device *dev, void *data,struct vmw_user_surface *user_srf;struct drm_vmw_size __user *user_sizes;struct ttm_base_object *base; -int ret = -EINVAL ; - - base =ttm_base_object_lookup_for_ref (dev_priv->tdev, req->sid); -if (unlikely (base ==NULL )) { -DRM_ERROR ("Could not find surface to reference.\n" ); -return -EINVAL ; - } +int ret; -if (unlikely (ttm_base_object_type (base) !=VMW_RES_SURFACE )) - goto out_bad_resource; + ret =vmw_surface_handle_reference (dev_priv, file_priv, req->sid, + req->handle_type, &base); +if (unlikely (ret !=0 )) +return ret; user_srf = container_of(base, struct vmw_user_surface, prime.base); srf = &user_srf->srf; - ret =ttm_ref_object_add (tfile, &user_srf->prime.base, -TTM_REF_USAGE ,NULL ); -if (unlikely (ret !=0 )) { -DRM_ERROR ("Could not add a reference to a surface.\n" ); - goto out_no_reference; - } - rep->flags = srf->flags; rep->format = srf->format;memcpy (rep->mip_levels, srf->mip_levels,sizeof (srf->mip_levels));
@@ -930,10 +977,10 @@ int vmw_surface_reference_ioctl(struct drm_device *dev, void *data,if (unlikely (ret !=0 )) {DRM_ERROR ("copy_to_user failed %p %u\n" , user_sizes, srf->num_sizes); +ttm_ref_object_base_unref (tfile, base->hash.key,TTM_REF_USAGE ); ret = -EFAULT ; } -out_bad_resource: -out_no_reference: +ttm_base_object_unref (&base);return ret;
@@ -1313,14 +1360,10 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data,uint32_t backup_handle;int ret = -EINVAL ; - base =ttm_base_object_lookup_for_ref (dev_priv->tdev, req->sid); -if (unlikely (base ==NULL )) { -DRM_ERROR ("Could not find surface to reference.\n" ); -return -EINVAL ; - } - -if (unlikely (ttm_base_object_type (base) !=VMW_RES_SURFACE )) - goto out_bad_resource; + ret =vmw_surface_handle_reference (dev_priv, file_priv, req->sid, + req->handle_type, &base); +if (unlikely (ret !=0 )) +return ret; user_srf = container_of(base, struct vmw_user_surface, prime.base); srf = &user_srf->srf;
@@ -1329,13 +1372,6 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data, goto out_bad_resource; } - ret =ttm_ref_object_add (tfile, &user_srf->prime.base, -TTM_REF_USAGE ,NULL ); -if (unlikely (ret !=0 )) { -DRM_ERROR ("Could not add a reference to a GB surface.\n" ); - goto out_bad_resource; - } -mutex_lock (&dev_priv->cmdbuf_mutex);/* Protect res->backup */ ret =vmw_user_dmabuf_reference (tfile, srf->res.backup, &backup_handle);
@@ -1344,8 +1380,7 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data,if (unlikely (ret !=0 )) {DRM_ERROR ("Could not add a reference to a GB surface " "backup buffer.\n" ); - (void )ttm_ref_object_base_unref (vmw_fpriv (file_priv)->tfile, - req->sid, + (void )ttm_ref_object_base_unref (tfile, base->hash.key,TTM_REF_USAGE ); goto out_bad_resource; }
include/uapi/drm/vmwgfx_drm.h +11 -1
@@ -90,6 +90,15 @@#define DRM_VMW_PARAM_MAX_MOB_SIZE 10/** +* enum drm_vmw_handle_type - handle type for ref ioctls +* +*/ +enum drm_vmw_handle_type { +DRM_VMW_HANDLE_LEGACY =0 , +DRM_VMW_HANDLE_PRIME =1 +}; + +/** * struct drm_vmw_getparam_arg * * @value: Returned value. //Out
@@ -177,6 +186,7 @@ struct drm_vmw_surface_create_req {* struct drm_wmv_surface_arg * * @sid: Surface id of created surface or surface to destroy or reference. +* @handle_type: Handle type for DRM_VMW_REF_SURFACE Ioctl. * * Output data from the DRM_VMW_CREATE_SURFACE Ioctl. * Input argument to the DRM_VMW_UNREF_SURFACE Ioctl.
@@ -185,7 +195,7 @@ struct drm_vmw_surface_create_req {struct drm_vmw_surface_arg {int32_t sid; -uint32_t pad64; +enum drm_vmw_handle_type handle_type; };/**
Signature · 1 line
1.7.10.4
PATCH 12/16 drm/vmwgfx: Tighten security around surface sharing
In reply to Thomas HellstromIf using legacy (non-prime) surface sharing, only allow surfaces to be shared between clients with the same master. This will block malicious clients from peeking at contents at surfaces from other (possibly vt-switched) masters.
--- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +24 -0
@@ -36,11 +36,13 @@* @base: The TTM base object handling user-space visibility. * @srf: The surface metadata. * @size: TTM accounting size for the surface. +* @master: master of the creating client. Used for security check. */ struct vmw_user_surface {struct ttm_prime_object prime;struct vmw_surface srf;uint32_t size; +struct drm_master *master; };/**
@@ -624,6 +626,8 @@ static void vmw_user_surface_free(struct vmw_resource *res)struct vmw_private *dev_priv = srf->res.dev_priv;uint32_t size = user_srf->size; +if (user_srf->master) +drm_master_put (&user_srf->master);kfree (srf->offsets);kfree (srf->sizes);kfree (srf->snooper.image);
@@ -819,6 +823,8 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, user_srf->prime.base.shareable = false; user_srf->prime.base.tfile =NULL ; +if (drm_is_legacy_client (file_priv)) + user_srf->master =drm_master_get (file_priv->master);/** * From this point, the generic resource management functions
@@ -885,6 +891,7 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, struct ttm_base_object **base_p) {struct ttm_object_file *tfile =vmw_fpriv (file_priv)->tfile; +struct vmw_user_surface *user_srf;uint32_t handle;struct ttm_base_object *base;int ret;
@@ -915,6 +922,21 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, }if (handle_type !=DRM_VMW_HANDLE_PRIME ) { + user_srf = container_of(base, struct vmw_user_surface, + prime.base); + +/* +* Make sure the surface creator has the same +* authenticating master. +*/ +if (drm_is_legacy_client (file_priv) && + user_srf->master != file_priv->master) { +DRM_ERROR ("Trying to reference surface outside of" +" master domain.\n" ); + ret = -EACCES ; + goto out_bad_resource; + } + ret =ttm_ref_object_add (tfile, base,TTM_REF_USAGE ,NULL );if (unlikely (ret !=0 )) {DRM_ERROR ("Could not add a reference to a surface.\n" );
@@ -1273,6 +1295,8 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, user_srf->prime.base.shareable = false; user_srf->prime.base.tfile =NULL ; +if (drm_is_legacy_client (file_priv)) + user_srf->master =drm_master_get (file_priv->master);/** * From this point, the generic resource management functions
Signature · 1 line
1.7.10.4
PATCH 13/16 drm/ttm: Add a ttm_ref_object_exists function
In reply to Thomas HellstromA function to be used to check whether a caller has put a ref object (opened) a struct ttm_base_object
--- drivers/gpu/drm/ttm/ttm_object.c | 46 ++++++++++++++++++++++++++++++++++++++ include/drm/ttm/ttm_object.h | 4 ++++ 2 files changed, 50 insertions(+)
drivers/gpu/drm/ttm/ttm_object.c +46 -0
@@ -270,6 +270,52 @@ ttm_base_object_lookup_for_ref(struct ttm_object_device *tdev, uint32_t key) }EXPORT_SYMBOL (ttm_base_object_lookup_for_ref); +/** +* ttm_ref_object_exists - Check whether a caller has a valid ref object +* (has opened) a base object. +* +* @tfile: Pointer to a struct ttm_object_file identifying the caller. +* @base: Pointer to a struct base object. +* +* Checks wether the caller identified by @tfile has put a valid USAGE +* reference object on the base object identified by @base. +*/ +bool ttm_ref_object_exists (struct ttm_object_file *tfile, +struct ttm_base_object *base) +{ +struct drm_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE ]; +struct drm_hash_item *hash; +struct ttm_ref_object *ref; + +rcu_read_lock (); +if (unlikely (drm_ht_find_item_rcu (ht, base->hash.key, &hash) !=0 )) + goto out_false; + +/* +* Verify that the ref object is really pointing to our base object. +* Our base object could actually be dead, and the ref object pointing +* to another base object with the same handle. +*/ + ref = drm_hash_entry(hash, struct ttm_ref_object, hash); +if (unlikely (base != ref->obj)) + goto out_false; + +/* +* Verify that the ref->obj pointer was actually valid! +*/ +rmb (); +if (unlikely (atomic_read (&ref->kref.refcount) ==0 )) + goto out_false; + +rcu_read_unlock (); +return true; + + out_false: +rcu_read_unlock (); +return false; +} +EXPORT_SYMBOL (ttm_ref_object_exists); +int ttm_ref_object_add (struct ttm_object_file *tfile,struct ttm_base_object *base,enum ttm_ref_type ref_type,bool *existed)
include/drm/ttm/ttm_object.h +4 -0
@@ -244,6 +244,10 @@ extern void ttm_base_object_unref(struct ttm_base_object **p_base);extern int ttm_ref_object_add (struct ttm_object_file *tfile,struct ttm_base_object *base,enum ttm_ref_type ref_type,bool *existed); + +extern bool ttm_ref_object_exists (struct ttm_object_file *tfile, +struct ttm_base_object *base); +/** * ttm_ref_object_base_unref *
Signature · 1 line
1.7.10.4
PATCH 14/16 drm/vmwgfx: Tighten the security around buffer maps
In reply to Thomas HellstromMake sure only buffer objects that are referenced by the client can be mapped.
--- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +7 -2
@@ -538,8 +538,13 @@ int vmw_user_dmabuf_verify_access(struct ttm_buffer_object *bo,return -EPERM ; vmw_user_bo =vmw_user_dma_buffer (bo); -return (vmw_user_bo->prime.base.tfile == tfile || - vmw_user_bo->prime.base.shareable) ?0 : -EPERM ; + +/* Check that the caller has opened the object. */ +if (likely (ttm_ref_object_exists (tfile, &vmw_user_bo->prime.base))) +return 0 ; + +DRM_ERROR ("Could not grant buffer access.\n" ); +return -EPERM ; }/**
Signature · 1 line
1.7.10.4
PATCH 15/16 drm/vmwgfx: Enable render nodes
In reply to Thomas Hellstrom--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 43 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 22 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +21 -22
@@ -142,11 +142,11 @@static const struct drm_ioctl_desc vmw_ioctls[] = {VMW_IOCTL_DEF (VMW_GET_PARAM , vmw_getparam_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_ALLOC_DMABUF , vmw_dmabuf_alloc_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_UNREF_DMABUF , vmw_dmabuf_unref_ioctl, -DRM_UNLOCKED ), +DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_CURSOR_BYPASS , vmw_kms_cursor_bypass_ioctl,DRM_MASTER |DRM_CONTROL_ALLOW |DRM_UNLOCKED ),
@@ -159,29 +159,28 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED),VMW_IOCTL_DEF (VMW_CREATE_CONTEXT , vmw_context_define_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_UNREF_CONTEXT , vmw_context_destroy_ioctl, -DRM_UNLOCKED ), +DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_CREATE_SURFACE , vmw_surface_define_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_UNREF_SURFACE , vmw_surface_destroy_ioctl, -DRM_UNLOCKED ), +DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_REF_SURFACE , vmw_surface_reference_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_EXECBUF , vmw_execbuf_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_FENCE_WAIT , vmw_fence_obj_wait_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_FENCE_SIGNALED , vmw_fence_obj_signaled_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_FENCE_UNREF , vmw_fence_obj_unref_ioctl, -DRM_UNLOCKED ), -VMW_IOCTL_DEF (VMW_FENCE_EVENT , - vmw_fence_event_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_UNLOCKED |DRM_RENDER_ALLOW ), +VMW_IOCTL_DEF (VMW_FENCE_EVENT , vmw_fence_event_ioctl, +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_GET_3D_CAP , vmw_get_cap_3d_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),/* these allow direct access to the framebuffers mark as master only */ VMW_IOCTL_DEF(VMW_PRESENT, vmw_present_ioctl,
@@ -194,19 +193,19 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_MASTER | DRM_UNLOCKED),VMW_IOCTL_DEF (VMW_CREATE_SHADER , vmw_shader_define_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_UNREF_SHADER , vmw_shader_destroy_ioctl, -DRM_UNLOCKED ), +DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_GB_SURFACE_CREATE , vmw_gb_surface_define_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_GB_SURFACE_REF , vmw_gb_surface_reference_ioctl, -DRM_AUTH |DRM_UNLOCKED ), +DRM_AUTH |DRM_UNLOCKED |DRM_RENDER_ALLOW ),VMW_IOCTL_DEF (VMW_SYNCCPU , vmw_user_dmabuf_synccpu_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), }; static struct pci_device_id vmw_pci_id_list[] = {
@@ -1406,7 +1405,7 @@ static const struct file_operations vmwgfx_driver_fops = {static struct drm_driver driver = { .driver_features =DRIVER_HAVE_IRQ |DRIVER_IRQ_SHARED | -DRIVER_MODESET |DRIVER_PRIME , +DRIVER_MODESET |DRIVER_PRIME |DRIVER_RENDER , .load = vmw_driver_load, .unload = vmw_driver_unload, .lastclose = vmw_lastclose,
Signature · 1 line
1.7.10.4
PATCH 16/16 drm/vmwgfx: Bump driver minor and date
In reply to Thomas HellstromSignal availability of prime fd reference ioctls and render nodes.
--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +2 -2
@@ -40,9 +40,9 @@#include <drm/ttm/ttm_module.h> #include "vmwgfx_fence.h" -#define VMWGFX_DRIVER_DATE "20140228" +#define VMWGFX_DRIVER_DATE "20140325"#define VMWGFX_DRIVER_MAJOR 2 -#define VMWGFX_DRIVER_MINOR 5 +#define VMWGFX_DRIVER_MINOR 6#define VMWGFX_DRIVER_PATCHLEVEL 0#define VMWGFX_FILE_PAGE_OFFSET 0x00100000#define VMWGFX_FIFO_STATIC_SIZE (1024*1024)
Signature · 1 line
1.7.10.4