Race-condition fix with free_waiter threads during shutdown.

When we shutdown, the main process locks the mutex, causing
all of the free_waiter functions to pile up on their lock.
Once we unlock in the main process, all of the free_waiters
start working. However the next instruction in the main proces
is to destroy the mutex. The short window is all the free_waiter
threads have to do their cleanup before they attempt to unlock
the mutex - which might have been de-allocated (and set to NULL).
End result can be a seg-fault.

This fix adds a ref-count to the mutex so that during shutdown
we spin and wait until all of the free_waiter functions
have completed and the ref-count is set to zero.

diff --git a/libmultipath/lock.c b/libmultipath/lock.c
index 0ca87831e177f70c5361520b25a3152cb19eaf9c..54e298880d915df91f81761cffa9309b302a4371 100644 (file)
--- a/libmultipath/lock.c
+++ b/libmultipath/lock.c
@@ -1,8 +1,8 @@
 #include <pthread.h>
 #include "lock.h"
-
+#include <stdio.h>
 void cleanup_lock (void * data)
 {
-       unlock((pthread_mutex_t *)data);
+       unlock ((*(struct mutex_lock *)data));
 }
 

diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index 6afecdab4913f09547f9e034de6cb89574b05b37..1f9a0f36fa819af81ebfc036b2d664da3faa495b 100644 (file)
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -1,19 +1,28 @@
 #ifndef _LOCK_H
 #define _LOCK_H
 
+/*
+ * Wrapper for the mutex. Includes a ref-count to keep
+ * track of how many there are out-standing threads blocking
+ * on a mutex. */
+struct mutex_lock {
+       pthread_mutex_t *mutex;
+       int depth;
+};
+
 #ifdef LCKDBG
 #define lock(a) \
-               fprintf(stderr, "%s:%s(%i) lock %p\n", __FILE__, __FUNCTION__, __LINE__, a); \
-        pthread_mutex_lock(a)
+               fprintf(stderr, "%s:%s(%i) lock %p depth: %d (%ld)\n", __FILE__, __FUNCTION__, __LINE__, a.mutex, a.depth, pthread_self()); \
+               a.depth++; pthread_mutex_lock(a.mutex)
 #define unlock(a) \
-               fprintf(stderr, "%s:%s(%i) unlock %p\n", __FILE__, __FUNCTION__, __LINE__, a); \
-        pthread_mutex_unlock(a)
+               fprintf(stderr, "%s:%s(%i) unlock %p depth: %d (%ld)\n", __FILE__, __FUNCTION__, __LINE__, a.mutex, a.depth, pthread_self()); \
+       a.depth--; pthread_mutex_unlock(a.mutex)
 #define lock_cleanup_pop(a) \
-               fprintf(stderr, "%s:%s(%i) unlock %p\n", __FILE__, __FUNCTION__, __LINE__, a); \
-        pthread_cleanup_pop(1);
+               fprintf(stderr, "%s:%s(%i) unlock %p depth: %d (%ld)\n", __FILE__, __FUNCTION__, __LINE__, a.mutex, a.depth, pthread_self()); \
+       pthread_cleanup_pop(1);
 #else
-#define lock(a) pthread_mutex_lock(a)
-#define unlock(a) pthread_mutex_unlock(a)
+#define lock(a) a.depth++; pthread_mutex_lock(a.mutex)
+#define unlock(a) a.depth--; pthread_mutex_unlock(a.mutex)
 #define lock_cleanup_pop(a) pthread_cleanup_pop(1);
 #endif
 

diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 19a2387897489a6bfa29b14cac23b47e0765413e..78e468af65ea1e9bd0687869b1130c512f11b7d7 100644 (file)
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -1,8 +1,14 @@
 #ifndef _STRUCTS_VEC_H
 #define _STRUCTS_VEC_H
 
+#include "lock.h"
+/*
+struct mutex_lock {
+       pthread_mutex_t *mutex;
+       int depth;
+}; */
 struct vectors {
-       pthread_mutex_t *lock;
+       struct mutex_lock lock; /* defined in lock.h */
        vector pathvec;
        vector mpvec;
 };

diff --git a/libmultipath/waiter.c b/libmultipath/waiter.c
index 54cd19fd00d245abc5d4a08d66c25332d02a25a4..28b810f72daf60c5102a44375a9babb242d70675 100644 (file)
--- a/libmultipath/waiter.c
+++ b/libmultipath/waiter.c
@@ -45,7 +45,10 @@ void free_waiter (void *data)
                 */
                wp->mpp->waiter = NULL;
        else
-               condlog(3, "free_waiter, mpp freed before wp=%p,", wp);
+               /*
+               * This is OK condition during shutdown.
+               */
+               condlog(3, "free_waiter, mpp freed before wp=%p (%s).", wp, wp->mapname);
 
        unlock(wp->vecs->lock);
 
@@ -58,13 +61,16 @@ void free_waiter (void *data)
 void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 {
        struct event_thread *wp = (struct event_thread *)mpp->waiter;
+       pthread_t thread;
 
        if (!wp) {
                condlog(3, "%s: no waiter thread", mpp->alias);
                return;
        }
-       condlog(2, "%s: stop event checker thread", wp->mapname);
-       pthread_kill((pthread_t)wp->thread, SIGUSR1);
+       thread = wp->thread;
+       condlog(2, "%s: stop event checker thread (%lu)", wp->mapname, thread);
+
+       pthread_kill(thread, SIGUSR1);
 }
 
 static sigset_t unblock_signals(void)
@@ -148,7 +154,7 @@ int waiteventloop (struct event_thread *waiter)
                 * 4) a path reinstate : nothing to do
                 * 5) a switch group : nothing to do
                 */
-               pthread_cleanup_push(cleanup_lock, waiter->vecs->lock);
+               pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock);
                lock(waiter->vecs->lock);
                r = update_multipath(waiter->vecs, waiter->mapname);
                lock_cleanup_pop(waiter->vecs->lock);

diff --git a/multipath/main.c b/multipath/main.c
index 8b38a6e7bb197a8e88e56e423faa26f23082d0b1..2f227d17f4d78f10416de0071f63e58b834b9328 100644 (file)
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -439,6 +439,7 @@ main (int argc, char *argv[])
                condlog(3, "restart multipath configuration process");
        
 out:
+
        sysfs_cleanup();
        dm_lib_release();
        dm_lib_exit();

diff --git a/multipathd/main.c b/multipathd/main.c
index 36aa93cee804ce4c771c24b0c92e87e81f7b3a22..7d2316a3d3d6a6107920c0df3d0732ee584ab336 100644 (file)
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -582,7 +582,7 @@ uxsock_trigger (char * str, char ** reply, int * len, void * trigger_data)
        *len = 0;
        vecs = (struct vectors *)trigger_data;
 
-       pthread_cleanup_push(cleanup_lock, vecs->lock);
+       pthread_cleanup_push(cleanup_lock, &vecs->lock);
        lock(vecs->lock);
 
        r = parse_cmd(str, reply, len, vecs);
@@ -740,9 +740,9 @@ exit_daemon (int status)
        condlog(3, "unlink pidfile");
        unlink(DEFAULT_PIDFILE);
 
-       lock(&exit_mutex);
+       pthread_mutex_lock(&exit_mutex);
        pthread_cond_signal(&exit_cond);
-       unlock(&exit_mutex);
+       pthread_mutex_unlock(&exit_mutex);
 
        return status;
 }
@@ -1020,7 +1020,7 @@ checkerloop (void *ap)
        }
 
        while (1) {
-               pthread_cleanup_push(cleanup_lock, vecs->lock);
+               pthread_cleanup_push(cleanup_lock, &vecs->lock);
                lock(vecs->lock);
                condlog(4, "tick");
 
@@ -1163,13 +1163,14 @@ init_vecs (void)
        if (!vecs)
                return NULL;
 
-       vecs->lock =
+       vecs->lock.mutex =
                (pthread_mutex_t *)MALLOC(sizeof(pthread_mutex_t));
 
-       if (!vecs->lock)
+       if (!vecs->lock.mutex)
                goto out;
 
-       pthread_mutex_init(vecs->lock, NULL);
+       pthread_mutex_init(vecs->lock.mutex, NULL);
+       vecs->lock.depth = 0;
 
        return vecs;
 
@@ -1341,7 +1342,6 @@ child (void * param)
                condlog(0, "failure during configuration");
                exit(1);
        }
-
        /*
         * start threads
         */
@@ -1375,9 +1375,15 @@ child (void * param)
        free_polls();
 
        unlock(vecs->lock);
-       pthread_mutex_destroy(vecs->lock);
-       FREE(vecs->lock);
-       vecs->lock = NULL;
+       /* Now all the waitevent threads will start rushing in. */
+       while (vecs->lock.depth > 0) {
+               sleep (1); /* This is weak. */
+               condlog(3,"Have %d wait event checkers threads to de-alloc, waiting..\n", vecs->lock.depth);
+       }
+       pthread_mutex_destroy(vecs->lock.mutex);
+       FREE(vecs->lock.mutex);
+       vecs->lock.depth = 0;
+       vecs->lock.mutex = NULL;
        FREE(vecs);
        vecs = NULL;