Hi, again!
I've looked through the code again and have some answers to your questions. Will post an updated patch soon.
On 03/28/2014 01:19 AM, David Herrmann wrote:
Hi
On Thu, Mar 27, 2014 at 9:23 PM, Thomas Hellstrom thellstrom@vmware.com wrote:
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 separate master_mutex.
Also remove drm_master::blocked since it's not used.
v2: Add an inline comment about what drm_device::master_mutex is protecting.
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Brian Paul brianp@vmware.com
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(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1..af55e2b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -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_primary_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);drm_master_create() doesn't need any locking (considering your removal of master_list), so this locking is exclusively to set ->master? See below.
No. The locking here is, as you say, unneeded. drm_minor::master is protected by master_mutex. I'll remove, and while doing that, I'll also remove the unneeded locking around priv->authenticated = 1.
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);drm_master_put() resets the pointers to NULL. So _if_ the locking above is needed, then this one _must_ stay, too.
It's unneeded, so this should be OK. As for drm_file::master, it should be considered immutable, except for drm_file open() and release() where nobody is racing us.
I also don't quite understand why you move the struct_mutex locking into drm_master_destroy() instead of requiring callers to lock it as before? I mean, the whole function is protected by the lock..
goto out_close; } }
mutex_lock(&dev->struct_mutex); if (dev->driver->master_set) { ret = dev->driver->master_set(dev, priv, true);vmwgfx is the only driver using that, so I guess you reviewed this lock-change.
Yes.
if (ret) { /* drop both references if this fails */ drm_master_put(&priv->minor->master); drm_master_put(&priv->master);same as above
No change needed.
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);Weird white-space change..
Will fix.
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);diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d344513..10c8303 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -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, {for setting int ret = 0;
mutex_lock(&dev->master_mutex);Yey! One more step towards DRM_UNLOCKED.
if (file_priv->is_master)
return 0;
goto out_unlock;
if (file_priv->minor->master && file_priv->minor->master != file_priv->master)return -EINVAL;That one was redundant.. yey..
ret = -EINVAL;This breaks all drivers with set_master == NULL. We never return 0 then.. I also dislike setting error-codes before doing the comparison just to drop the curly brackets.. that used to be common kernel-coding-style, but I thought we all stopped doing that. Same for dropmaster below, but just my personal opinion.
Will fix.
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);You set minor->master unlocked here again. See my reasoning above..
Protected by master_mutex.
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);Again setting minor->master unlocked.
Protected by master_mutex.
Thanks David
Thanks, Thomas