[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2
On 03/28/2014 01:19 AM, David Herrmann wrote: > Hi > > > > 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.. Before, struct_mutex was required outside of drm_master_put(), because the argument to drm_master_put was also protected by struct_mutex. Now that's no longer the case and I chose to move the locking into the destructor, avoiding a number of unnecessary struct_mutex locks (the master destructor is called more seldom than master_put()). Also in general I believe one should avoid locking around unreferencing if possible, because it leads to ugly constructs (and even static code analyzers complaining) if the lock has to be temporarily released in the destructor to avoid locking order violations. But in the end I guess that's really a matter of taste. /Thomas
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2
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
> 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
>> Reviewed-by: Brian Paul
>> ---
>> 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(>struct_mutex);
>> + mutex_lock(>master_mutex);
>> if (drm_is_primary_client(priv) && !priv->minor->master) {
>> /* create a new master */
>> + mutex_lock(>struct_mutex);
>> priv->minor->master = drm_master_create(priv->minor);
>> + mutex_unlock(>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(>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(>struct_mutex);
>> priv->authenticated = 1;
>>
>> mutex_unlock(>struct_mutex);
>> if (dev->driver->master_create) {
>> ret = dev->driver->master_create(dev, priv->master);
>> if (ret) {
>> - mutex_lock(>struct_mutex);
>> /* drop both references if this fails */
>> drm_master_put(>minor->master);
>> drm_master_put(>master);
>> - mutex_unlock(>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(>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(>minor->master);
>> drm_master_put(>master);
> same as above
No change needed.
>
>> - mutex_unlock(>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(>struct_mutex);
>>
>> + mutex_unlock(>master_mutex);
> Weird white-space change..
Will fix.
>
>> mutex_lock(>struct_mutex);
>> list_add(>lhead, >filelist);
>> mutex_unlock(>struct_mutex);
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2
Hi, David.
Thanks for reviewing.
I'll try to address all your concerns and resend.
/Thomas
On 03/28/2014 01:19 AM, David Herrmann wrote:
> Hi
>
> On Thu, Mar 27, 2014 at 9:23 PM, Thomas Hellstrom
> 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
>> Reviewed-by: Brian Paul
>> ---
>> 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(>struct_mutex);
>> + mutex_lock(>master_mutex);
>> if (drm_is_primary_client(priv) && !priv->minor->master) {
>> /* create a new master */
>> + mutex_lock(>struct_mutex);
>> priv->minor->master = drm_master_create(priv->minor);
>> + mutex_unlock(>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.
>
>> if (!priv->minor->master) {
>> - mutex_unlock(>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(>struct_mutex);
>> priv->authenticated = 1;
>>
>> mutex_unlock(>struct_mutex);
>> if (dev->driver->master_create) {
>> ret = dev->driver->master_create(dev, priv->master);
>> if (ret) {
>> - mutex_lock(>struct_mutex);
>> /* drop both references if this fails */
>> drm_master_put(>minor->master);
>> drm_master_put(>master);
>> - mutex_unlock(>struct_mutex);
> drm_master_put() resets the pointers to NULL. So _if_ the locking
> above is needed, then this one _must_ stay, too.
>
> 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(>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.
>
>> if (ret) {
>> /* drop both references if this fails */
>> drm_master_put(>minor->master);
>> drm_master_put(>master);
> same as above
>
>> - mutex_unlock(>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(>struct_mutex);
>>
>> + mutex_unlock(>master_mutex);
> Weird white-space change..
>
>> mutex_lock(>struct_mutex);
>> list_add(>lhead, >filelist);
>> mutex_unlock(>struct_mutex);
>> @@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct
>> file *filp,
>> return 0;
>>
>> out_close:
>> + mutex_unlock(>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(>ctxlist_mutex);
>>
>> -
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2
Hi
On Thu, Mar 27, 2014 at 9:23 PM, Thomas Hellstrom
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
> Reviewed-by: Brian Paul
> ---
> 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(>struct_mutex);
> + mutex_lock(>master_mutex);
> if (drm_is_primary_client(priv) && !priv->minor->master) {
> /* create a new master */
> + mutex_lock(>struct_mutex);
> priv->minor->master = drm_master_create(priv->minor);
> + mutex_unlock(>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.
> if (!priv->minor->master) {
> - mutex_unlock(>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(>struct_mutex);
> priv->authenticated = 1;
>
> mutex_unlock(>struct_mutex);
> if (dev->driver->master_create) {
> ret = dev->driver->master_create(dev, priv->master);
> if (ret) {
> - mutex_lock(>struct_mutex);
> /* drop both references if this fails */
> drm_master_put(>minor->master);
> drm_master_put(>master);
> - mutex_unlock(>struct_mutex);
drm_master_put() resets the pointers to NULL. So _if_ the locking
above is needed, then this one _must_ stay, too.
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(>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.
> if (ret) {
> /* drop both references if this fails */
> drm_master_put(>minor->master);
> drm_master_put(>master);
same as above
> - mutex_unlock(>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(>struct_mutex);
>
> + mutex_unlock(>master_mutex);
Weird white-space change..
> mutex_lock(>struct_mutex);
> list_add(>lhead, >filelist);
> mutex_unlock(>struct_mutex);
> @@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct
> file *filp,
> return 0;
>
> out_close:
> + mutex_unlock(>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(>ctxlist_mutex);
>
> - mutex_lock(>struct_mutex);
> + mutex_lock(>master_mutex);
>
> if (file_priv->is_master) {
> struct drm_master *master = file_priv->master;
> struct drm_file *temp;
> +
> + mutex_lock(>struct_mutex);
>
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2
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
Reviewed-by: Brian Paul
---
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(>struct_mutex);
+ mutex_lock(>master_mutex);
if (drm_is_primary_client(priv) && !priv->minor->master) {
/* create a new master */
+ mutex_lock(>struct_mutex);
priv->minor->master = drm_master_create(priv->minor);
+ mutex_unlock(>struct_mutex);
if (!priv->minor->master) {
- mutex_unlock(>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(>struct_mutex);
priv->authenticated = 1;
mutex_unlock(>struct_mutex);
if (dev->driver->master_create) {
ret = dev->driver->master_create(dev, priv->master);
if (ret) {
- mutex_lock(>struct_mutex);
/* drop both references if this fails */
drm_master_put(>minor->master);
drm_master_put(>master);
- mutex_unlock(>struct_mutex);
goto out_close;
}
}
- mutex_lock(>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(>minor->master);
drm_master_put(>master);
- mutex_unlock(>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(>struct_mutex);
+ mutex_unlock(>master_mutex);
mutex_lock(>struct_mutex);
list_add(>lhead, >filelist);
mutex_unlock(>struct_mutex);
@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file
*filp,
return 0;
out_close:
+ mutex_unlock(>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(>ctxlist_mutex);
- mutex_lock(>struct_mutex);
+ mutex_lock(>master_mutex);
if (file_priv->is_master) {
struct drm_master *master = file_priv->master;
struct drm_file *temp;
+
+ mutex_lock(>struct_mutex);
list_for_each_entry(temp, >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(>lock.lock_queue);
}
+ mutex_unlock(>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)
