Treat mutex->owner as volatile

When HAVE_MUTEX_OWNER is defined and we are directly accessing
mutex->owner treat is as volative with the ACCESS_ONCE() helper.
Without this you may get a stale cached value when accessing it
from different cpus.  This can result in incorrect behavior from
mutex_owned() and mutex_owner().  This is not a problem for the
!HAVE_MUTEX_OWNER case because in this case all the accesses
are covered by a spin lock which similarly gaurentees we will
not be accessing stale data.

Secondly, check CONFIG_SMP before allowing access to mutex->owner.
I see that for non-SMP setups the kernel does not track the owner
so we cannot rely on it.

Thirdly, check CONFIG_MUTEX_DEBUG when this is defined and the
HAVE_MUTEX_OWNER is defined surprisingly the mutex->owner will
not be cleared on mutex_exit().  When this is the case the SPL
needs to make sure to do it to ensure MUTEX_HELD() behaves as
expected or you will certainly assert in mutex_destroy().

Finally, improve the mutex regression tests.  For mutex_owned() we
now minimally check that it behaves correctly when checked from the
owner thread or the non-owner thread.  This subtle behaviour has bit
me before and I'd like to catch it early next time if it reappears.

As for mutex_owned() regression test additonally verify that
mutex->owner is always cleared on mutex_exit().

2 files changed, 116 insertions, 22 deletions

diff --git a/include/sys/mutex.h b/include/sys/mutex.h
index b36c7e256..d33694766 100644
--- a/include/sys/mutex.h
+++ b/include/sys/mutex.h
@@ -34,20 +34,29 @@ typedef enum {
         MUTEX_ADAPTIVE = 2
 } kmutex_type_t;
 
-#ifdef HAVE_MUTEX_OWNER
+#if defined(HAVE_MUTEX_OWNER) && defined(CONFIG_SMP)
 
 typedef struct mutex kmutex_t;
 
 static inline kthread_t *
 mutex_owner(kmutex_t *mp)
 {
-        if (mp->owner)
-                return (mp->owner)->task;
+	struct thread_info *owner;
+
+	owner = ACCESS_ONCE(mp->owner);
+        if (owner)
+                return owner->task;
 
         return NULL;
 }
-#define mutex_owned(mp)         (mutex_owner(mp) == current)
-#define MUTEX_HELD(mp)          mutex_owned(mp)
+
+static inline int
+mutex_owned(kmutex_t *mp)
+{
+	return (ACCESS_ONCE(mp->owner) == current_thread_info());
+}
+
+#define MUTEX_HELD(mp)                  mutex_owned(mp)
 #undef mutex_init
 #define mutex_init(mp, name, type, ibc)                                 \
 ({                                                                      \
@@ -60,13 +69,22 @@ mutex_owner(kmutex_t *mp)
 #undef mutex_destroy
 #define mutex_destroy(mp)                                               \
 ({                                                                      \
-        VERIFY(!MUTEX_HELD(mp));                                        \
+	VERIFY3P(mutex_owner(mp), ==, NULL);				\
 })
 
-#define mutex_tryenter(mp)      mutex_trylock(mp)
-#define mutex_enter(mp)         mutex_lock(mp)
-#define mutex_exit(mp)          mutex_unlock(mp)
+#define mutex_tryenter(mp)              mutex_trylock(mp)
+#define mutex_enter(mp)                 mutex_lock(mp)
 
+/* mutex->owner is not cleared when CONFIG_DEBUG_MUTEXES is set */
+#ifdef CONFIG_DEBUG_MUTEXES
+# define mutex_exit(mp)                                                 \
+({                                                                      \
+         (mp)->owner = NULL;                                            \
+	 mutex_unlock(mp);                                              \
+})
+#else
+# define mutex_exit(mp)                 mutex_unlock(mp)
+#endif /* CONFIG_DEBUG_MUTEXES */
 
 #ifdef HAVE_GPL_ONLY_SYMBOLS
 # define mutex_enter_nested(mp, sc)     mutex_lock_nested(mp, sc)
@@ -151,7 +169,7 @@ mutex_owner(kmutex_t *mp)
 #undef mutex_destroy
 #define mutex_destroy(mp)                                               \
 ({                                                                      \
-        VERIFY(!MUTEX_HELD(mp));                                        \
+	VERIFY3P(mutex_owner(mp), ==, NULL);				\
 })
 
 #define mutex_tryenter(mp)                                              \
diff --git a/module/splat/splat-mutex.c b/module/splat/splat-mutex.c
index 96ed27297..d134e49ce 100644
--- a/module/splat/splat-mutex.c
+++ b/module/splat/splat-mutex.c
@@ -55,6 +55,7 @@ typedef struct mutex_priv {
         struct file *mp_file;
         kmutex_t mp_mtx;
         int mp_rc;
+        int mp_rc2;
 } mutex_priv_t;
 
 static void
@@ -240,37 +241,96 @@ out:
         return rc;
 }
 
+static void
+splat_mutex_owned(void *priv)
+{
+        mutex_priv_t *mp = (mutex_priv_t *)priv;
+
+        ASSERT(mp->mp_magic == SPLAT_MUTEX_TEST_MAGIC);
+        mp->mp_rc = mutex_owned(&mp->mp_mtx);
+        mp->mp_rc2 = MUTEX_HELD(&mp->mp_mtx);
+}
+
 static int
 splat_mutex_test3(struct file *file, void *arg)
 {
-        kmutex_t mtx;
+        mutex_priv_t mp;
+        taskq_t *tq;
         int rc = 0;
 
-        mutex_init(&mtx, SPLAT_MUTEX_TEST_NAME, MUTEX_DEFAULT, NULL);
-        mutex_enter(&mtx);
+        mp.mp_magic = SPLAT_MUTEX_TEST_MAGIC;
+        mp.mp_file = file;
+        mutex_init(&mp.mp_mtx, SPLAT_MUTEX_TEST_NAME, MUTEX_DEFAULT, NULL);
+
+        if ((tq = taskq_create(SPLAT_MUTEX_TEST_NAME, 1, maxclsyspri,
+                               50, INT_MAX, TASKQ_PREPOPULATE)) == NULL) {
+                splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Taskq '%s' "
+                             "create failed\n", SPLAT_MUTEX_TEST3_NAME);
+                return -EINVAL;
+        }
+
+        mutex_enter(&mp.mp_mtx);
 
         /* Mutex should be owned by current */
-        if (!mutex_owned(&mtx)) {
+        if (!mutex_owned(&mp.mp_mtx)) {
                 splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Unowned mutex "
-                           "should be owned by pid %d\n", current->pid);
+                             "should be owned by pid %d\n", current->pid);
+                rc = -EINVAL;
+                goto out_exit;
+        }
+
+        if (taskq_dispatch(tq, splat_mutex_owned, &mp, TQ_SLEEP) == 0) {
+                splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Failed to "
+                             "dispatch function '%s' to taskq\n",
+                             sym2str(splat_mutex_owned));
+                rc = -EINVAL;
+                goto out_exit;
+        }
+        taskq_wait(tq);
+
+        /* Mutex should not be owned which checked from a different thread */
+        if (mp.mp_rc || mp.mp_rc2) {
+                splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex owned by "
+                             "pid %d not by taskq\n", current->pid);
+                rc = -EINVAL;
+                goto out_exit;
+        }
+
+        mutex_exit(&mp.mp_mtx);
+
+        /* Mutex should not be owned by current */
+        if (mutex_owned(&mp.mp_mtx)) {
+                splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex owned by "
+                             "pid %d it should be unowned\b", current->pid);
                 rc = -EINVAL;
                 goto out;
         }
 
-        mutex_exit(&mtx);
+        if (taskq_dispatch(tq, splat_mutex_owned, &mp, TQ_SLEEP) == 0) {
+                splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Failed to "
+                             "dispatch function '%s' to taskq\n",
+                             sym2str(splat_mutex_owned));
+                rc = -EINVAL;
+                goto out;
+        }
+        taskq_wait(tq);
 
-        /* Mutex should not be owned by any task */
-        if (mutex_owned(&mtx)) {
+        /* Mutex should be owned by no one */
+        if (mp.mp_rc || mp.mp_rc2) {
                 splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex owned by "
-                           "pid %d should be unowned\b", current->pid);
+                             "no one, %d/%d disagrees\n", mp.mp_rc, mp.mp_rc2);
                 rc = -EINVAL;
                 goto out;
         }
 
         splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "%s",
                    "Correct mutex_owned() behavior\n");
+        goto out;
+out_exit:
+        mutex_exit(&mp.mp_mtx);
 out:
-        mutex_destroy(&mtx);
+        mutex_destroy(&mp.mp_mtx);
+        taskq_destroy(tq);
 
         return rc;
 }
@@ -283,12 +343,28 @@ splat_mutex_test4(struct file *file, void *arg)
         int rc = 0;
 
         mutex_init(&mtx, SPLAT_MUTEX_TEST_NAME, MUTEX_DEFAULT, NULL);
+
+        /*
+         * Verify mutex owner is cleared after being dropped.  Depending
+         * on how you build your kernel this behavior changes, ensure the
+         * SPL mutex implementation is properly detecting this.
+         */
+        mutex_enter(&mtx);
+        msleep(100);
+        mutex_exit(&mtx);
+        if (MUTEX_HELD(&mtx)) {
+                splat_vprint(file, SPLAT_MUTEX_TEST4_NAME, "Mutex should "
+                           "not be held, bit is by %p\n", mutex_owner(&mtx));
+                rc = -EINVAL;
+                goto out;
+        }
+
         mutex_enter(&mtx);
 
         /* Mutex should be owned by current */
         owner = mutex_owner(&mtx);
         if (current != owner) {
-                splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex should "
+                splat_vprint(file, SPLAT_MUTEX_TEST4_NAME, "Mutex should "
                            "be owned by pid %d but is owned by pid %d\n",
                            current->pid, owner ? owner->pid : -1);
                 rc = -EINVAL;
@@ -300,7 +376,7 @@ splat_mutex_test4(struct file *file, void *arg)
         /* Mutex should not be owned by any task */
         owner = mutex_owner(&mtx);
         if (owner) {
-                splat_vprint(file, SPLAT_MUTEX_TEST3_NAME, "Mutex should not "
+                splat_vprint(file, SPLAT_MUTEX_TEST4_NAME, "Mutex should not "
                            "be owned but is owned by pid %d\n", owner->pid);
                 rc = -EINVAL;
                 goto out;