dtrace: fix lock ordering issues, mutex_owned(), and mutex debugging

Several cases of potential lock ordering issues were identified and resolved.
Both static and dynamic analysis of locking comes clean for DTrace after this
commit is applied.

The mutex_owned() function was not accounting for the possibility that a lock
might have an owner registered while unlocked.

Orabug: 17624236

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>

diff --git a/dtrace/dtrace_dev.c b/dtrace/dtrace_dev.c
index e4d5da6f4f928e975fa4b0a48c427465ba93e326..18be9e2bf98c35efb076e08b501bb729dcb84212 100644 (file)
--- a/dtrace/dtrace_dev.c
+++ b/dtrace/dtrace_dev.c
@@ -125,8 +125,8 @@ static int dtrace_open(struct inode *inode, struct file *file)
         */
        if (kdi_dtrace_set(KDI_DTSET_DTRACE_ACTIVATE) != 0) {
                dtrace_opens--;
-               mutex_unlock(&cpu_lock);
                mutex_unlock(&dtrace_lock);
+               mutex_unlock(&cpu_lock);
                return -EBUSY;
        }
 #endif
@@ -592,9 +592,9 @@ static long dtrace_ioctl(struct file *file,
 
                probe = dtrace_probe_lookup_id(desc.dtargd_id);
                if (probe == NULL) {
-                       mutex_unlock(&dtrace_lock);
-//                     mutex_unlock(&module_mutex); /* FIXME */
                        mutex_unlock(&dtrace_provider_lock);
+//                     mutex_unlock(&module_mutex); /* FIXME */
+                       mutex_unlock(&dtrace_lock);
 
                        return -EINVAL;
                }
@@ -1200,18 +1200,18 @@ dtrace_module_unloaded(struct module *module)
                 * The DTrace module is loaded (obviously) but not attached;
                 * we don't have any work to do.
                 */
-               mutex_unlock(&dtrace_provider_lock);
-               /* FIXME: mutex_unlock(&mod_lock); */
                mutex_unlock(&dtrace_lock);
+               /* FIXME: mutex_unlock(&mod_lock); */
+               mutex_unlock(&dtrace_provider_lock);
                return;
        }
 
        for (probe = first = dtrace_hash_lookup(dtrace_bymod, &template);
            probe != NULL; probe = probe->dtpr_nextmod) {
                if (probe->dtpr_ecb != NULL) {
-                       mutex_unlock(&dtrace_provider_lock);
-                       /* FIXME: mutex_unlock(&mod_lock); */
                        mutex_unlock(&dtrace_lock);
+                       /* FIXME: mutex_unlock(&mod_lock); */
+                       mutex_unlock(&dtrace_provider_lock);
 
                        /*
                         * This shouldn't _actually_ be possible -- we're
@@ -1359,21 +1359,6 @@ int dtrace_dev_init(void)
        dtrace_provider_id_t    id;
        int                     rc = 0;
 
-       mutex_lock(&cpu_lock);
-       mutex_lock(&dtrace_provider_lock);
-       mutex_lock(&dtrace_lock);
-
-       rc = dtrace_probe_init();
-       if (rc) {
-               pr_err("Failed to initialize DTrace core\n");
-
-               mutex_unlock(&cpu_lock);
-               mutex_unlock(&dtrace_provider_lock);
-               mutex_unlock(&dtrace_lock);
-
-               return rc;
-       }
-
        /*
         * Register the device for the DTrace core.
         */
@@ -1382,10 +1367,6 @@ int dtrace_dev_init(void)
                pr_err("%s: Can't register misc device %d\n",
                       dtrace_dev.name, dtrace_dev.minor);
 
-               mutex_unlock(&cpu_lock);
-               mutex_unlock(&dtrace_provider_lock);
-               mutex_unlock(&dtrace_lock);
-
                return rc;
        }
 
@@ -1397,15 +1378,26 @@ int dtrace_dev_init(void)
                pr_err("%s: Can't register misc device %d\n",
                       helper_dev.name, helper_dev.minor);
 
-               mutex_unlock(&cpu_lock);
-               mutex_unlock(&dtrace_provider_lock);
-               mutex_unlock(&dtrace_lock);
-
                return rc;
        }
 
        ctf_forceload();
 
+       mutex_lock(&cpu_lock);
+       mutex_lock(&dtrace_provider_lock);
+       mutex_lock(&dtrace_lock);
+
+       rc = dtrace_probe_init();
+       if (rc) {
+               pr_err("Failed to initialize DTrace core\n");
+
+               mutex_unlock(&dtrace_lock);
+               mutex_unlock(&dtrace_provider_lock);
+               mutex_unlock(&cpu_lock);
+
+               return rc;
+       }
+
        dtrace_modload = dtrace_module_loaded;
        dtrace_modunload = dtrace_module_unloaded;
        dtrace_helpers_cleanup = dtrace_helpers_destroy;
@@ -1484,7 +1476,6 @@ int dtrace_dev_init(void)
                                NULL, "ERROR", 0, NULL);
 
        dtrace_anon_property();
-       mutex_unlock(&cpu_lock);
 
        /*
         * If DTrace helper tracing is enabled, we need to allocate a trace
@@ -1513,8 +1504,9 @@ int dtrace_dev_init(void)
         * the first provider causing the core to be loaded.
         */
 #endif
-       mutex_unlock(&dtrace_provider_lock);
        mutex_unlock(&dtrace_lock);
+       mutex_unlock(&dtrace_provider_lock);
+       mutex_unlock(&cpu_lock);
 
        return 0;
 }
@@ -1554,9 +1546,10 @@ void dtrace_dev_exit(void)
        dtrace_byname = NULL;
 
        kmem_cache_destroy(dtrace_state_cachep);
-       misc_deregister(&helper_dev);
-       misc_deregister(&dtrace_dev);
 
        mutex_unlock(&dtrace_lock);
        mutex_unlock(&dtrace_provider_lock);
+
+       misc_deregister(&helper_dev);
+       misc_deregister(&dtrace_dev);
 }

diff --git a/dtrace/dtrace_probe.c b/dtrace/dtrace_probe.c
index a57d5109c72bd3838f80e492fa6743393746a893..ccda810fbc3818669f3dc379dc1e797fbb77a540 100644 (file)
--- a/dtrace/dtrace_probe.c
+++ b/dtrace/dtrace_probe.c
@@ -1283,15 +1283,15 @@ int dtrace_probe_init(void)
         * get them back right after.
         */
 again:
-       mutex_unlock(&cpu_lock);
-       mutex_unlock(&dtrace_provider_lock);
        mutex_unlock(&dtrace_lock);
+       mutex_unlock(&dtrace_provider_lock);
+       mutex_unlock(&cpu_lock);
 
        idr_pre_get(&dtrace_probe_idr, __GFP_NOFAIL);
 
-       mutex_lock(&dtrace_lock);
-       mutex_lock(&dtrace_provider_lock);
        mutex_lock(&cpu_lock);
+       mutex_lock(&dtrace_provider_lock);
+       mutex_lock(&dtrace_lock);
 
        err = idr_get_new(&dtrace_probe_idr, NULL, &id);
        if (err == -EAGAIN)

diff --git a/dtrace/dtrace_state.c b/dtrace/dtrace_state.c
index 6861223512a9ac757d3ea5ac4727119558433448..15910c118ad190faad205426a728391f106b69af 100644 (file)
--- a/dtrace/dtrace_state.c
+++ b/dtrace/dtrace_state.c
@@ -342,8 +342,8 @@ dtrace_state_t *dtrace_state_create(struct file *file)
        int             err;
        dtrace_aggid_t  aggid;
 
-       ASSERT(MUTEX_HELD(&dtrace_lock));
        ASSERT(MUTEX_HELD(&cpu_lock));
+       ASSERT(MUTEX_HELD(&dtrace_lock));
 
        state = kzalloc(sizeof (dtrace_state_t), GFP_KERNEL);
        if (state == NULL)
@@ -374,13 +374,13 @@ dtrace_state_t *dtrace_state_create(struct file *file)
         * that gets used as meaning 'none'.
         */
 again:
-       mutex_unlock(&cpu_lock);
        mutex_unlock(&dtrace_lock);
+       mutex_unlock(&cpu_lock);
 
        idr_pre_get(&state->dts_agg_idr, __GFP_NOFAIL);
 
-       mutex_lock(&dtrace_lock);
        mutex_lock(&cpu_lock);
+       mutex_lock(&dtrace_lock);
 
        err = idr_get_new(&state->dts_agg_idr, NULL, &aggid);
        if (err == -EAGAIN)

diff --git a/dtrace/include/dtrace/dtrace_impl_defines.h b/dtrace/include/dtrace/dtrace_impl_defines.h
index 64d89a1e013d54ee7f08f02a27f372b6959b3819..49a8bbf2b959a5e7bbfda3e96e410cec3ca3b1ec 100644 (file)
--- a/dtrace/include/dtrace/dtrace_impl_defines.h
+++ b/dtrace/include/dtrace/dtrace_impl_defines.h
@@ -201,26 +201,26 @@ typedef enum dtrace_vtime_state {
 #endif
 
 #ifdef CONFIG_DT_DEBUG_MUTEX
-# define _mutex_lock(x)                mutex_lock(x)
-# define _mutex_unlock(x)      mutex_unlock(x)
+# define real_mutex_lock(x)            mutex_lock(x)
+# define real_mutex_unlock(x)          mutex_unlock(x)
 
 # define mutex_lock(x)         do {                                          \
                                    printk(KERN_DEBUG                         \
                                           "mutex_lock(%s) at %s::%d "        \
-                                          "for %p(PID %d)\n",                \
+                                          " for %p (PID %d)\n",              \
                                           __stringify(x),                    \
                                           __FILE__, __LINE__, current,       \
                                           current ? current->pid : -1);      \
-                                   _mutex_lock(x);                           \
+                                   real_mutex_lock(x);                       \
                                } while (0)
 # define mutex_unlock(x)       do {                                          \
                                    printk(KERN_DEBUG                         \
                                           "mutex_unlock(%s) at %s::%d"       \
-                                          "for %p(PID %d)\n",                \
+                                          " for %p (PID %d)\n",              \
                                           __stringify(x),                    \
                                           __FILE__, __LINE__, current,       \
                                           current ? current->pid : -1);      \
-                                   _mutex_unlock(x);                         \
+                                   real_mutex_unlock(x);                     \
                                } while (0)
 #endif