[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v3
Hi On Fri, Mar 28, 2014 at 12:52 PM, Thomas Hellstrom wrote: > Thanks. Want a Reviewed-By: or Acked-By: added? Oh, sure: Reviewed-by: David Herrmann Thanks David
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v3
On 03/28/2014 12:23 PM, David Herrmann wrote: > Hi > > On Fri, Mar 28, 2014 at 10:34 AM, 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. >> Locking order is master_mutex -> struct_mutex. >> >> Also remove drm_master::blocked since it's not used. >> >> v2: Add an inline comment about what drm_device::master_mutex is protecting. >> v3: Remove unneeded struct_mutex locks. Fix error returns in >> drm_setmaster_ioctl(). > Thanks, looks much better now. Tested on hsw, so I'm fine with pushing > this into 3.15. > > Thanks > David > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel Thanks. Want a Reviewed-By: or Acked-By: added? /Thomas
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v3
Hi On Fri, Mar 28, 2014 at 10:34 AM, 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. > Locking order is master_mutex -> struct_mutex. > > Also remove drm_master::blocked since it's not used. > > v2: Add an inline comment about what drm_device::master_mutex is protecting. > v3: Remove unneeded struct_mutex locks. Fix error returns in > drm_setmaster_ioctl(). Thanks, looks much better now. Tested on hsw, so I'm fine with pushing this into 3.15. Thanks David
[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 v3
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.
Locking order is master_mutex -> struct_mutex.
Also remove drm_master::blocked since it's not used.
v2: Add an inline comment about what drm_device::master_mutex is protecting.
v3: Remove unneeded struct_mutex locks. Fix error returns in
drm_setmaster_ioctl().
Signed-off-by: Thomas Hellstrom
Reviewed-by: Brian Paul
---
drivers/gpu/drm/drm_fops.c | 22 ++---
drivers/gpu/drm/drm_stub.c | 43 ++---
include/drm/drmP.h | 46
3 files changed, 64 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c7792b1..a0ce39c 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -231,12 +231,11 @@ 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 */
priv->minor->master = drm_master_create(priv->minor);
if (!priv->minor->master) {
- mutex_unlock(>struct_mutex);
ret = -ENOMEM;
goto out_close;
}
@@ -244,29 +243,23 @@ static int drm_open_helper(struct inode *inode, struct
file *filp,
priv->is_master = 1;
/* take another reference for the copy in the local file priv */
priv->master = drm_master_get(priv->minor->master);
-
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,7 +267,7 @@ 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);
@@ -302,6 +295,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 +483,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 +508,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 +518,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)
[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
Hi
On Wed, Mar 26, 2014 at 9:40 PM, Thomas Hellstrom
wrote:
>> - 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.
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.
>> - why is master_mutex per device and not per-minor? I thought masters
>> on minors are _entirely_ independent?How do multiple keysyms
>
> 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.
Fair enough.
>>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>>> index e6cdd0f..dad571f 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_legacy_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);
>> What's that struct_mutex doing 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.
Well, it now looks like this:
mutex_lock(>struct_mutex);
priv->authenticated = 1;
mutex_unlock(>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 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)
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
On Wed, Mar 26, 2014 at 09:40:18PM +0100, Thomas Hellstrom wrote: > On 03/26/2014 08:08 PM, David Herrmann wrote: > > "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. > > 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. 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
On 03/26/2014 08:08 PM, David Herrmann wrote:
> Hi
>
> On Tue, Mar 25, 2014 at 2:18 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.
> 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.
> "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.
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.
>
> Regarding master_mutex I 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.
> - 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.
> - why is master_mutex per 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.
>
> Few more comments inline.
>
>> 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 e6cdd0f..dad571f 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_legacy_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);
> What's that struct_mutex doing 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.
>
>> if (dev->driver->master_create) {
>> ret = dev->driver->master_create(dev, priv->master);
>> if (ret) {
>> -
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
Hi
On Tue, Mar 25, 2014 at 2:18 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.
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.
> 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 e6cdd0f..dad571f 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_legacy_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);
What's that struct_mutex doing here? We're in ->open(), there is
no-one racing against us.
> 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;
> +
> +
[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex
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.
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 e6cdd0f..dad571f 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_legacy_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)
drm_master_put(_priv->master);
file_priv->is_master = 0;
+ mutex_unlock(>master_mutex);
+
+ mutex_lock(>struct_mutex);
