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().

1 files changed, 88 insertions, 12 deletions

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;