[patch] Fix unchecked atomic_cmpset when unlocking mutex

John Baldwin jhb at freebsd.org
Wed Dec 21 18:46:28 UTC 2011


On Tuesday, December 20, 2011 5:26:17 pm Jilles Tjoelker wrote:
> When unlocking a contested mutex that is not PI or PP, the mutex is made
> available for new lockers faster by unlocking it in userland and only
> calling the kernel to wake sleeping threads. This seems to make sense.
> 
> Although the atomic_cmpset here should never fail because this thread
> owns the mutex and other threads may only set UMUTEX_CONTESTED which is
> already set, it seems prudent anyway to check for this.
> 
> If the atomic_cmpset fails, use the slow path that unlocks in the
> kernel.
> 
> Index: lib/libthr/thread/thr_umtx.c
> ===================================================================
> --- lib/libthr/thread/thr_umtx.c	(revision 228504)
> +++ lib/libthr/thread/thr_umtx.c	(working copy)
> @@ -154,10 +154,9 @@
>  {
>  #ifndef __ia64__
>  	/* XXX this logic has a race-condition on ia64. */
> -	if ((mtx->m_flags & (UMUTEX_PRIO_PROTECT | UMUTEX_PRIO_INHERIT)) == 0) {
> -		atomic_cmpset_rel_32(&mtx->m_owner, id | UMUTEX_CONTESTED, UMUTEX_CONTESTED);
> +	if ((mtx->m_flags & (UMUTEX_PRIO_PROTECT | UMUTEX_PRIO_INHERIT)) == 0 &&
> +	    atomic_cmpset_rel_32(&mtx->m_owner, id | UMUTEX_CONTESTED, UMUTEX_CONTESTED))
>  		return _umtx_op_err(mtx, UMTX_OP_MUTEX_WAKE, 0, 0, 0);
> -	}
>  #endif /* __ia64__ */
>  	return _umtx_op_err(mtx, UMTX_OP_MUTEX_UNLOCK, 0, 0, 0);
>  }

Not checking for a failing atomic_cmpset seems inherently broken to me.  When
I first saw this, I wondered if fixing this would fix the "race" on ia64.

-- 
John Baldwin


More information about the freebsd-threads mailing list