[PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Jason Low]

The mutex owner can get read and written to without the wait_lock.
Use WRITE_ONCE when setting and clearing the owner field in order
to avoid optimizations such as store tearing. This avoids
situations where the owner field gets written to with multiple
stores and another thread could concurrently read and use a
partially written owner value.

Signed-off-by: Jason Low <jason.low2@hpe.com>
---
 kernel/locking/mutex.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
index 5cda397..469b61e 100644
--- a/kernel/locking/mutex.h
+++ b/kernel/locking/mutex.h
@@ -17,14 +17,20 @@
 		__list_del((waiter)->list.prev, (waiter)->list.next)
 
 #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
+/*
+ * The mutex owner can get read and written to locklessly.
+ * We should use WRITE_ONCE() when writing the owner value to
+ * avoid store tearing, otherwise, a thread could potentially
+ * read a partially written and incomplete owner value.
+ */
 static inline void mutex_set_owner(struct mutex *lock)
 {
-	lock->owner = current;
+	WRITE_ONCE(lock->owner, current);
 }
 
 static inline void mutex_clear_owner(struct mutex *lock)
 {
-	lock->owner = NULL;
+	WRITE_ONCE(lock->owner, NULL);
 }
 #else
 static inline void mutex_set_owner(struct mutex *lock)
-- 
2.1.4

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Davidlohr Bueso]

On Thu, 19 May 2016, Jason Low wrote:

>The mutex owner can get read and written to without the wait_lock.
>Use WRITE_ONCE when setting and clearing the owner field in order
>to avoid optimizations such as store tearing. This avoids
>situations where the owner field gets written to with multiple
>stores and another thread could concurrently read and use a
>partially written owner value.

Right, I also ran into this with rtmutexes when avoiding the schedule()
and the spin on owner thingie.

>Signed-off-by: Jason Low <jason.low2@hpe.com>

Acked-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Waiman Long]

On 05/19/2016 06:23 PM, Jason Low wrote:
> The mutex owner can get read and written to without the wait_lock.
> Use WRITE_ONCE when setting and clearing the owner field in order
> to avoid optimizations such as store tearing. This avoids
> situations where the owner field gets written to with multiple
> stores and another thread could concurrently read and use a
> partially written owner value.
>
> Signed-off-by: Jason Low<jason.low2@hpe.com>
> ---
>   kernel/locking/mutex.h | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
> index 5cda397..469b61e 100644
> --- a/kernel/locking/mutex.h
> +++ b/kernel/locking/mutex.h
> @@ -17,14 +17,20 @@
>   		__list_del((waiter)->list.prev, (waiter)->list.next)
>
>   #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
> +/*
> + * The mutex owner can get read and written to locklessly.
> + * We should use WRITE_ONCE() when writing the owner value to
> + * avoid store tearing, otherwise, a thread could potentially
> + * read a partially written and incomplete owner value.
> + */
>   static inline void mutex_set_owner(struct mutex *lock)
>   {
> -	lock->owner = current;
> +	WRITE_ONCE(lock->owner, current);
>   }
>
>   static inline void mutex_clear_owner(struct mutex *lock)
>   {
> -	lock->owner = NULL;
> +	WRITE_ONCE(lock->owner, NULL);
>   }
>   #else
>   static inline void mutex_set_owner(struct mutex *lock)

I think mutex-debug.h also needs similar changes for completeness.

Cheers,
Longman

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Jason Low]

On Fri, 2016-05-20 at 16:27 -0400, Waiman Long wrote:
> On 05/19/2016 06:23 PM, Jason Low wrote:
> > The mutex owner can get read and written to without the wait_lock.
> > Use WRITE_ONCE when setting and clearing the owner field in order
> > to avoid optimizations such as store tearing. This avoids
> > situations where the owner field gets written to with multiple
> > stores and another thread could concurrently read and use a
> > partially written owner value.
> >
> > Signed-off-by: Jason Low<jason.low2@hpe.com>
> > ---
> >   kernel/locking/mutex.h | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
> > index 5cda397..469b61e 100644
> > --- a/kernel/locking/mutex.h
> > +++ b/kernel/locking/mutex.h
> > @@ -17,14 +17,20 @@
> >   		__list_del((waiter)->list.prev, (waiter)->list.next)
> >
> >   #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
> > +/*
> > + * The mutex owner can get read and written to locklessly.
> > + * We should use WRITE_ONCE() when writing the owner value to
> > + * avoid store tearing, otherwise, a thread could potentially
> > + * read a partially written and incomplete owner value.
> > + */
> >   static inline void mutex_set_owner(struct mutex *lock)
> >   {
> > -	lock->owner = current;
> > +	WRITE_ONCE(lock->owner, current);
> >   }
> >
> >   static inline void mutex_clear_owner(struct mutex *lock)
> >   {
> > -	lock->owner = NULL;
> > +	WRITE_ONCE(lock->owner, NULL);
> >   }
> >   #else
> >   static inline void mutex_set_owner(struct mutex *lock)
> 
> I think mutex-debug.h also needs similar changes for completeness.

Good point, I will add the changes to those functions in the debug case
to this patch.

Thanks,
Jason

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Davidlohr Bueso]

On Fri, 20 May 2016, Waiman Long wrote:

>I think mutex-debug.h also needs similar changes for completeness.

Maybe, but given that with debug the wait_lock is unavoidable, doesn't
this send the wrong message?

Thanks,
Davidlohr

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Waiman Long]

On 05/20/2016 09:00 PM, Davidlohr Bueso wrote:
> On Fri, 20 May 2016, Waiman Long wrote:
>
>> I think mutex-debug.h also needs similar changes for completeness.
>
> Maybe, but given that with debug the wait_lock is unavoidable, doesn't
> this send the wrong message?
>
> Thanks,
> Davidlohr

You are right. Optimistic spinning is disabled when DEBUG_MUTEXES is 
true. So WRITE_ONCE() is not really needed. However, I think we should 
have at least a comment in mutex-debug.h as to why the owner field is 
handled differently from mutex.h.

Cheers,
Longman

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Jason Low]

On Fri, 2016-05-20 at 18:00 -0700, Davidlohr Bueso wrote:
> On Fri, 20 May 2016, Waiman Long wrote:
> 
> >I think mutex-debug.h also needs similar changes for completeness.
> 
> Maybe, but given that with debug the wait_lock is unavoidable, doesn't
> this send the wrong message?

The mutex_set_owner() and mutex_clear_owner() can still get called in
the fastpath without the wait_lock for the debug case too correct? If
that's the case, then the WRITE_ONCE would still make sense to have in
the debug case.

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Davidlohr Bueso]

On Mon, 23 May 2016, Jason Low wrote:

>On Fri, 2016-05-20 at 18:00 -0700, Davidlohr Bueso wrote:
>> On Fri, 20 May 2016, Waiman Long wrote:
>>
>> >I think mutex-debug.h also needs similar changes for completeness.
>>
>> Maybe, but given that with debug the wait_lock is unavoidable, doesn't
>> this send the wrong message?
>
>The mutex_set_owner() and mutex_clear_owner() can still get called in
>the fastpath without the wait_lock for the debug case too correct?

The fastpath is a nop, see mutex-null:

#define __mutex_fastpath_lock(count, fail_fn)		fail_fn(count)

In anycase, if there were a fastpath, that would at least have an
ACQUIRE barrier (unless you set ->owner it before it or something
weird). But this isn't the case, so debug should be safe afaict.

Thanks,
Davidlohr

Re: [PATCH] locking/mutex: Set and clear owner using WRITE_ONCE()

[Jason Low]

On Mon, 2016-05-23 at 14:31 -0700, Davidlohr Bueso wrote:
> On Mon, 23 May 2016, Jason Low wrote:
> 
> >On Fri, 2016-05-20 at 18:00 -0700, Davidlohr Bueso wrote:
> >> On Fri, 20 May 2016, Waiman Long wrote:
> >>
> >> >I think mutex-debug.h also needs similar changes for completeness.
> >>
> >> Maybe, but given that with debug the wait_lock is unavoidable, doesn't
> >> this send the wrong message?
> >
> >The mutex_set_owner() and mutex_clear_owner() can still get called in
> >the fastpath without the wait_lock for the debug case too correct?
> 
> The fastpath is a nop, see mutex-null:
> 
> #define __mutex_fastpath_lock(count, fail_fn)		fail_fn(count)

Oh right, so then the mutex_set_owner() call in mutex_lock() doesn't
really matter. I'll update the patch to remove the use of WRITE_ONCE for
the debug case then, and add the comments about why it is different as
suggested.

Thanks,
Jason