From 873e9ca16e918c76988004890ef5c9f5060669fd Mon Sep 17 00:00:00 2001 From: Kris Van Hees Date: Thu, 17 Oct 2013 19:18:44 -0400 Subject: [PATCH] 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 --- dtrace/dtrace_dev.c | 61 +++++++++------------ dtrace/dtrace_probe.c | 8 +-- dtrace/dtrace_state.c | 6 +- dtrace/include/dtrace/dtrace_impl_defines.h | 12 ++-- 4 files changed, 40 insertions(+), 47 deletions(-) diff --git a/dtrace/dtrace_dev.c b/dtrace/dtrace_dev.c index e4d5da6f4f92..18be9e2bf98c 100644 --- 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 a57d5109c72b..ccda810fbc38 100644 --- 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 6861223512a9..15910c118ad1 100644 --- 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 64d89a1e013d..49a8bbf2b959 100644 --- 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 -- 2.50.1