From 0a682531f4904fc97d172234557fdd02dd495032 Mon Sep 17 00:00:00 2001 From: Andriy Gapon Date: Thu, 27 Oct 2016 07:11:31 +0000 Subject: [PATCH] 3746 ZRLs are racy illumos/illumos-gate@260af64db74a52d64de8c6c5f67dd0a71d228ca5 https://github.com/illumos/illumos-gate/commit/260af64db74a52d64de8c6c5f67dd0a71d228ca5 https://www.illumos.org/issues/3746 From the original change log: It was possible for a reference to be added even with the lock held, and for references added just after a lock release to be lost. This bug was also independently found and reported in wesunsolve.net issues 6985013 6995524. In zrl_add(), always use an atomic operation to update the refcount. The mutex in the ZRL only guarantees that wakeups occur for waiters on the lock. It offers no protection against concurrent updates of the refcount. The only refcount transition that is safe to perform without an atomic operation is from ZRL_LOCKED back to 0, since this can only be performed by the thread which has the ZRL locked. Authored by: Will Andrews Reviewed by: Boris Protopopov Reviewed by: Pavel Zakharov Reviewed by: Yuri Pankov Reviewed by: Justin T. Gibbs Approved by: Matt Ahrens Author: Youzhong Yang --- uts/common/fs/zfs/zrlock.c | 48 +++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/uts/common/fs/zfs/zrlock.c b/uts/common/fs/zfs/zrlock.c index 7f6beeed6149..ec333e54d2a8 100644 --- a/uts/common/fs/zfs/zrlock.c +++ b/uts/common/fs/zfs/zrlock.c @@ -21,6 +21,7 @@ /* * Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved. * Copyright (c) 2014, 2015 by Delphix. All rights reserved. + * Copyright 2016 The MathWorks, Inc. All rights reserved. */ /* @@ -71,37 +72,32 @@ zrl_destroy(zrlock_t *zrl) void zrl_add_impl(zrlock_t *zrl, const char *zc) { - uint32_t n = (uint32_t)zrl->zr_refcount; - - while (n != ZRL_LOCKED) { - uint32_t cas = atomic_cas_32( - (uint32_t *)&zrl->zr_refcount, n, n + 1); - if (cas == n) { - ASSERT3S((int32_t)n, >=, 0); + for (;;) { + uint32_t n = (uint32_t)zrl->zr_refcount; + while (n != ZRL_LOCKED) { + uint32_t cas = atomic_cas_32( + (uint32_t *)&zrl->zr_refcount, n, n + 1); + if (cas == n) { + ASSERT3S((int32_t)n, >=, 0); #ifdef ZFS_DEBUG - if (zrl->zr_owner == curthread) { - DTRACE_PROBE2(zrlock__reentry, - zrlock_t *, zrl, uint32_t, n); + if (zrl->zr_owner == curthread) { + DTRACE_PROBE2(zrlock__reentry, + zrlock_t *, zrl, uint32_t, n); + } + zrl->zr_owner = curthread; + zrl->zr_caller = zc; +#endif + return; } - zrl->zr_owner = curthread; - zrl->zr_caller = zc; -#endif - return; + n = cas; } - n = cas; - } - mutex_enter(&zrl->zr_mtx); - while (zrl->zr_refcount == ZRL_LOCKED) { - cv_wait(&zrl->zr_cv, &zrl->zr_mtx); + mutex_enter(&zrl->zr_mtx); + while (zrl->zr_refcount == ZRL_LOCKED) { + cv_wait(&zrl->zr_cv, &zrl->zr_mtx); + } + mutex_exit(&zrl->zr_mtx); } - ASSERT3S(zrl->zr_refcount, >=, 0); - zrl->zr_refcount++; -#ifdef ZFS_DEBUG - zrl->zr_owner = curthread; - zrl->zr_caller = zc; -#endif - mutex_exit(&zrl->zr_mtx); } void