* [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES
@ 2015-01-02 9:47 Chris Wilson
2015-01-05 10:51 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2015-01-02 9:47 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson, stable
If CONFIG_DEBUG_MUTEXES is set, the mutex->owner field is only cleared
if the mutex debugging is enabled which introduces a race in our
mutex_is_locked_by() - i.e. we may inspect the old owner value before it
is acquired by the new task.
This is the root cause of this error:
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
index 5cf6731..3ef3736 100644
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -80,13 +80,13 @@ void debug_mutex_unlock(struct mutex *lock)
DEBUG_LOCKS_WARN_ON(lock->owner != current);
DEBUG_LOCKS_WARN_ON(!lock->wait_list.prev && !lock->wait_list.next);
- mutex_clear_owner(lock);
}
/*
* __mutex_slowpath_needs_to_unlock() is explicitly 0 for debug
* mutexes so that we can do it here after we've verified state.
*/
+ mutex_clear_owner(lock);
atomic_set(&lock->count, 1);
}
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87955
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ad55b06a3cb1..4c14db9587a6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5024,7 +5024,7 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
if (!mutex_is_locked(mutex))
return false;
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
+#if defined(CONFIG_SMP) && !defined(CONFIG_DEBUG_MUTEXES)
return mutex->owner == task;
#else
/* Since UP may be pre-empted, we cannot assume that we own the lock */
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES
2015-01-02 9:47 [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES Chris Wilson
@ 2015-01-05 10:51 ` Daniel Vetter
2015-01-07 12:12 ` [Intel-gfx] " Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2015-01-05 10:51 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, stable
On Fri, Jan 02, 2015 at 09:47:10AM +0000, Chris Wilson wrote:
> If CONFIG_DEBUG_MUTEXES is set, the mutex->owner field is only cleared
> if the mutex debugging is enabled which introduces a race in our
> mutex_is_locked_by() - i.e. we may inspect the old owner value before it
> is acquired by the new task.
>
> This is the root cause of this error:
>
> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> index 5cf6731..3ef3736 100644
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -80,13 +80,13 @@ void debug_mutex_unlock(struct mutex *lock)
> DEBUG_LOCKS_WARN_ON(lock->owner != current);
>
> DEBUG_LOCKS_WARN_ON(!lock->wait_list.prev && !lock->wait_list.next);
> - mutex_clear_owner(lock);
> }
>
> /*
> * __mutex_slowpath_needs_to_unlock() is explicitly 0 for debug
> * mutexes so that we can do it here after we've verified state.
> */
> + mutex_clear_owner(lock);
> atomic_set(&lock->count, 1);
> }
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87955
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ad55b06a3cb1..4c14db9587a6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5024,7 +5024,7 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
> if (!mutex_is_locked(mutex))
> return false;
>
> -#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
> +#if defined(CONFIG_SMP) && !defined(CONFIG_DEBUG_MUTEXES)
> return mutex->owner == task;
> #else
> /* Since UP may be pre-empted, we cannot assume that we own the lock */
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES
2015-01-05 10:51 ` Daniel Vetter
@ 2015-01-07 12:12 ` Jani Nikula
2015-01-07 13:20 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2015-01-07 12:12 UTC (permalink / raw)
To: Daniel Vetter, Chris Wilson; +Cc: intel-gfx, stable
On Mon, 05 Jan 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jan 02, 2015 at 09:47:10AM +0000, Chris Wilson wrote:
>> If CONFIG_DEBUG_MUTEXES is set, the mutex->owner field is only cleared
>> if the mutex debugging is enabled which introduces a race in our
>> mutex_is_locked_by() - i.e. we may inspect the old owner value before it
>> is acquired by the new task.
>>
>> This is the root cause of this error:
>>
>> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
>> index 5cf6731..3ef3736 100644
>> --- a/kernel/locking/mutex-debug.c
>> +++ b/kernel/locking/mutex-debug.c
>> @@ -80,13 +80,13 @@ void debug_mutex_unlock(struct mutex *lock)
>> DEBUG_LOCKS_WARN_ON(lock->owner != current);
>>
>> DEBUG_LOCKS_WARN_ON(!lock->wait_list.prev && !lock->wait_list.next);
>> - mutex_clear_owner(lock);
>> }
>>
>> /*
>> * __mutex_slowpath_needs_to_unlock() is explicitly 0 for debug
>> * mutexes so that we can do it here after we've verified state.
>> */
>> + mutex_clear_owner(lock);
>> atomic_set(&lock->count, 1);
>> }
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87955
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: stable@vger.kernel.org
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Pushed to drm-intel-fixes, thanks for the patch and review.
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index ad55b06a3cb1..4c14db9587a6 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -5024,7 +5024,7 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
>> if (!mutex_is_locked(mutex))
>> return false;
>>
>> -#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
>> +#if defined(CONFIG_SMP) && !defined(CONFIG_DEBUG_MUTEXES)
>> return mutex->owner == task;
>> #else
>> /* Since UP may be pre-empted, we cannot assume that we own the lock */
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES
2015-01-07 12:12 ` [Intel-gfx] " Jani Nikula
@ 2015-01-07 13:20 ` Chris Wilson
2015-01-07 13:27 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2015-01-07 13:20 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, stable
On Wed, Jan 07, 2015 at 02:12:14PM +0200, Jani Nikula wrote:
> On Mon, 05 Jan 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jan 02, 2015 at 09:47:10AM +0000, Chris Wilson wrote:
> >> If CONFIG_DEBUG_MUTEXES is set, the mutex->owner field is only cleared
> >> if the mutex debugging is enabled which introduces a race in our
> >> mutex_is_locked_by() - i.e. we may inspect the old owner value before it
> >> is acquired by the new task.
> >>
> >> This is the root cause of this error:
> >>
> >> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> >> index 5cf6731..3ef3736 100644
> >> --- a/kernel/locking/mutex-debug.c
> >> +++ b/kernel/locking/mutex-debug.c
> >> @@ -80,13 +80,13 @@ void debug_mutex_unlock(struct mutex *lock)
> >> DEBUG_LOCKS_WARN_ON(lock->owner != current);
> >>
> >> DEBUG_LOCKS_WARN_ON(!lock->wait_list.prev && !lock->wait_list.next);
> >> - mutex_clear_owner(lock);
> >> }
> >>
> >> /*
> >> * __mutex_slowpath_needs_to_unlock() is explicitly 0 for debug
> >> * mutexes so that we can do it here after we've verified state.
> >> */
> >> + mutex_clear_owner(lock);
> >> atomic_set(&lock->count, 1);
> >> }
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87955
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: stable@vger.kernel.org
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Pushed to drm-intel-fixes, thanks for the patch and review.
For the record, I plan to post a revert of this to -next if the core fix
lands upstream (looks good so far).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES
2015-01-07 13:20 ` Chris Wilson
@ 2015-01-07 13:27 ` Jani Nikula
2015-01-07 13:38 ` [Intel-gfx] " Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2015-01-07 13:27 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, stable
On Wed, 07 Jan 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Jan 07, 2015 at 02:12:14PM +0200, Jani Nikula wrote:
>> On Mon, 05 Jan 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Fri, Jan 02, 2015 at 09:47:10AM +0000, Chris Wilson wrote:
>> >> If CONFIG_DEBUG_MUTEXES is set, the mutex->owner field is only cleared
>> >> if the mutex debugging is enabled which introduces a race in our
>> >> mutex_is_locked_by() - i.e. we may inspect the old owner value before it
>> >> is acquired by the new task.
>> >>
>> >> This is the root cause of this error:
>> >>
>> >> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
>> >> index 5cf6731..3ef3736 100644
>> >> --- a/kernel/locking/mutex-debug.c
>> >> +++ b/kernel/locking/mutex-debug.c
>> >> @@ -80,13 +80,13 @@ void debug_mutex_unlock(struct mutex *lock)
>> >> DEBUG_LOCKS_WARN_ON(lock->owner != current);
>> >>
>> >> DEBUG_LOCKS_WARN_ON(!lock->wait_list.prev && !lock->wait_list.next);
>> >> - mutex_clear_owner(lock);
>> >> }
>> >>
>> >> /*
>> >> * __mutex_slowpath_needs_to_unlock() is explicitly 0 for debug
>> >> * mutexes so that we can do it here after we've verified state.
>> >> */
>> >> + mutex_clear_owner(lock);
>> >> atomic_set(&lock->count, 1);
>> >> }
>> >>
>> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87955
>> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >> Cc: stable@vger.kernel.org
>> >
>> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Pushed to drm-intel-fixes, thanks for the patch and review.
>
> For the record, I plan to post a revert of this to -next if the core fix
> lands upstream (looks good so far).
Does it look like that could be cc:stable?
BR,
Jani.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES
2015-01-07 13:27 ` Jani Nikula
@ 2015-01-07 13:38 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2015-01-07 13:38 UTC (permalink / raw)
To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx, stable
On Wed, Jan 07, 2015 at 03:27:41PM +0200, Jani Nikula wrote:
> On Wed, 07 Jan 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, Jan 07, 2015 at 02:12:14PM +0200, Jani Nikula wrote:
> >> On Mon, 05 Jan 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> > On Fri, Jan 02, 2015 at 09:47:10AM +0000, Chris Wilson wrote:
> >> >> If CONFIG_DEBUG_MUTEXES is set, the mutex->owner field is only cleared
> >> >> if the mutex debugging is enabled which introduces a race in our
> >> >> mutex_is_locked_by() - i.e. we may inspect the old owner value before it
> >> >> is acquired by the new task.
> >> >>
> >> >> This is the root cause of this error:
> >> >>
> >> >> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
> >> >> index 5cf6731..3ef3736 100644
> >> >> --- a/kernel/locking/mutex-debug.c
> >> >> +++ b/kernel/locking/mutex-debug.c
> >> >> @@ -80,13 +80,13 @@ void debug_mutex_unlock(struct mutex *lock)
> >> >> DEBUG_LOCKS_WARN_ON(lock->owner != current);
> >> >>
> >> >> DEBUG_LOCKS_WARN_ON(!lock->wait_list.prev && !lock->wait_list.next);
> >> >> - mutex_clear_owner(lock);
> >> >> }
> >> >>
> >> >> /*
> >> >> * __mutex_slowpath_needs_to_unlock() is explicitly 0 for debug
> >> >> * mutexes so that we can do it here after we've verified state.
> >> >> */
> >> >> + mutex_clear_owner(lock);
> >> >> atomic_set(&lock->count, 1);
> >> >> }
> >> >>
> >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87955
> >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> Cc: stable@vger.kernel.org
> >> >
> >> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> Pushed to drm-intel-fixes, thanks for the patch and review.
> >
> > For the record, I plan to post a revert of this to -next if the core fix
> > lands upstream (looks good so far).
>
> Does it look like that could be cc:stable?
I didn't suggest it should be, so I wait with bated breath. I know
disabling lock-stealing in i915 is fairly safe (some corner cases may
now get oom, but that's a perenial risk anyway) and so felt more
comfortable with pushing this i915 patch through stable.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-01-07 13:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-02 9:47 [PATCH] drm/i915: Fix mutex->owner inspection race under DEBUG_MUTEXES Chris Wilson
2015-01-05 10:51 ` Daniel Vetter
2015-01-07 12:12 ` [Intel-gfx] " Jani Nikula
2015-01-07 13:20 ` Chris Wilson
2015-01-07 13:27 ` Jani Nikula
2015-01-07 13:38 ` [Intel-gfx] " Chris Wilson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox