[PATCH] Memory leaks and race condition fixes.

[PATCH] Memory leaks and race condition fixes.

[Konrad Rzeszutek]

Various set of fixes I've carried in my tree. These patches
came as a result of running valgrind on multipathd/multipath
and finding simple bugs and some complex.

Konrad Rzeszutek (4):
      Free prio_name and checker_name now that we dynamically allocate them.
      Unload prio and checkers libraries during shutdown.
      Don't display the state of the newly added map during addition in the daemon and don't switch groups.
      Race-condition fix with free_waiter threads during shutdown.

[PATCH] Free prio_name and checker_name now that we dynamically allocate them.

[Konrad Rzeszutek]

From: Konrad Rzeszutek <konrad@mars.virtualiron.com>

In earlier releases the prio_name and checker_name would point
to statically allocated values. Now that we strdup them, we need
to free them during shutdown.
---
 libmultipath/config.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 6d731d2..7307fc0 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -152,6 +152,11 @@ free_hwe (struct hwentry * hwe)
 	if (hwe->bl_product)
 		FREE(hwe->bl_product);
 
+	if (hwe->prio_name)
+		FREE(hwe->prio_name);
+
+	if (hwe->checker_name)
+		FREE(hwe->checker_name);
 	FREE(hwe);
 }
 
@@ -250,7 +255,7 @@ set_param_str(char * str)
 #define merge_str(s) \
 	if (hwe2->s) { \
 		if (hwe1->s) \
-			free(hwe1->s); \
+			FREE(hwe1->s); \
 		if (!(hwe1->s = set_param_str(hwe2->s))) \
 			return 1; \
 	}
@@ -393,6 +398,12 @@ free_config (struct config * conf)
 	if (conf->hwhandler)
 		FREE(conf->hwhandler);
 
+	if (conf->prio_name)
+		FREE(conf->prio_name);
+
+	if (conf->checker_name)
+		FREE(conf->checker_name);
+
 	free_blacklist(conf->blist_devnode);
 	free_blacklist(conf->blist_wwid);
 	free_blacklist_device(conf->blist_device);
-- 
1.5.4.1

[PATCH] Unload prio and checkers libraries during shutdown.

[Konrad Rzeszutek]

From: Konrad Rzeszutek <konrad@mars.virtualiron.com>

Our statup sequence is 'load_config', 'init_checkers', and 'init_prio'.
Both init_* functions reset the list of prio and checkers, which is
unfortunate as in load_config, depending on the multipat.conf, would
load prio and checker libraries. This results in double-loading of
the libraries and a memory leak.
---
 libmultipath/checkers.c |    7 +++----
 libmultipath/checkers.h |    1 +
 libmultipath/prio.c     |    7 +++----
 libmultipath/prio.h     |    1 +
 multipath/main.c        |    2 ++
 multipathd/main.c       |    2 ++
 6 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 5132081..19d0781 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -27,7 +27,6 @@ char * checker_state_name (int i)
 
 int init_checkers (void)
 {
-	INIT_LIST_HEAD(&checkers);
 	if (!add_checker(DEFAULT_CHECKER))
 		return 1;
 	return 0;
@@ -35,12 +34,12 @@ int init_checkers (void)
 
 struct checker * alloc_checker (void)
 {
-	return zalloc(sizeof(struct checker));
+	return MALLOC(sizeof(struct checker));
 }
 
 void free_checker (struct checker * c)
 {
-	free(c);
+	FREE(c);
 }
 
 void cleanup_checkers (void)
@@ -50,7 +49,7 @@ void cleanup_checkers (void)
 
 	list_for_each_entry_safe(checker_loop, checker_temp, &checkers, node) {
 		list_del(&checker_loop->node);
-		free(checker_loop);
+		free_checker(checker_loop);
 	}
 }
 
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index e735250..b610e6b 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -111,6 +111,7 @@ struct checker {
 
 char * checker_state_name (int);
 int init_checkers (void);
+void cleanup_checkers (void);
 struct checker * add_checker (char *);
 struct checker * checker_lookup (char *);
 int checker_init (struct checker *, void **);
diff --git a/libmultipath/prio.c b/libmultipath/prio.c
index c9d2873..4c5f4f0 100644
--- a/libmultipath/prio.c
+++ b/libmultipath/prio.c
@@ -11,7 +11,6 @@ static LIST_HEAD(prioritizers);
 
 int init_prio (void)
 {
-	INIT_LIST_HEAD(&prioritizers);
 	if (!add_prio(DEFAULT_PRIO))
 		return 1;
 	return 0;
@@ -19,12 +18,12 @@ int init_prio (void)
 
 struct prio * alloc_prio (void)
 {
-	return zalloc(sizeof(struct prio));
+	return MALLOC(sizeof(struct prio));
 }
 
 void free_prio (struct prio * p)
 {
-	free(p);
+	FREE(p);
 }
 
 void cleanup_prio(void)
@@ -34,7 +33,7 @@ void cleanup_prio(void)
 
 	list_for_each_entry_safe(prio_loop, prio_temp, &prioritizers, node) {
 		list_del(&prio_loop->node);
-		free(prio_loop);
+		free_prio(prio_loop);
 	}
 }
 
diff --git a/libmultipath/prio.h b/libmultipath/prio.h
index 491e6fc..fc9277f 100644
--- a/libmultipath/prio.h
+++ b/libmultipath/prio.h
@@ -42,6 +42,7 @@ struct prio {
 };
 
 int init_prio (void);
+void cleanup_prio (void);
 struct prio * add_prio (char *);
 struct prio * prio_lookup (char *);
 int prio_getprio (struct prio *, struct path *);
diff --git a/multipath/main.c b/multipath/main.c
index 8b38a6e..e60b9bc 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -443,6 +443,8 @@ out:
 	dm_lib_release();
 	dm_lib_exit();
 
+	cleanup_prio();
+	cleanup_checkers();
 	/*
 	 * Freeing config must be done after dm_lib_exit(), because
 	 * the logging function (dm_write_log()), which is called there,
diff --git a/multipathd/main.c b/multipathd/main.c
index 36aa93c..7de41a0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1389,6 +1389,8 @@ child (void * param)
 	dm_lib_release();
 	dm_lib_exit();
 
+	cleanup_prio();
+	cleanup_checkers();
 	/*
 	 * Freeing config must be done after condlog() and dm_lib_exit(),
 	 * because logging functions like dlog() and dm_write_log()
-- 
1.5.4.1

[PATCH] Don't display the state of the newly added map during addition in the daemon and don't switch groups.

[Konrad Rzeszutek]

From: Konrad Rzeszutek <konrad@mars.virtualiron.com>

A previous commit mass-changed #ifdef DAEMON to check for 'mpp->waiter'. Unfortunatly
when the 'domap' function is called with ACT_CREATE in the daemon, the mpp->waiter is not
set, hence the multipath client mode logic is choosen. Fixing this triggers another
issues which is that newly added path via ACT_CREATE won't have their waitevent thread
created as the caller checks mpp->action (which changed to ACT_NOTHING) and won't
start the thread.
---
 libmultipath/config.h    |    1 +
 libmultipath/configure.c |    9 ++++++---
 multipath/main.c         |    1 +
 multipathd/main.c        |    2 +-
 multipathd/main.h        |    1 -
 5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index fb917f4..07aa8c0 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -64,6 +64,7 @@ struct config {
 	int pg_timeout;
 	int max_fds;
 	int force_reload;
+	int daemon;
 
 	char * dev;
 	char * sysfs_dir;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 83ee0ae..e00582f 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -369,7 +369,7 @@ domap (struct multipath * mpp)
 		 * DM_DEVICE_CREATE, DM_DEVICE_RENAME, or DM_DEVICE_RELOAD
 		 * succeeded
 		 */
-		if (!mpp->waiter) {
+		if (!conf->daemon) {
 			/* multipath client mode */
 			dm_switchgroup(mpp->alias, mpp->bestpg);
 			if (mpp->action != ACT_NOTHING)
@@ -380,9 +380,12 @@ domap (struct multipath * mpp)
 			condlog(2, "%s: load table [0 %llu %s %s]", mpp->alias,
 				mpp->size, TGT_MPATH, mpp->params);
 			/*
-			 * Required action is over, reset for the stateful daemon
+			 * Required action is over, reset for the stateful daemon.
+			 * But don't do it for creation as we use in the caller the
+			 * mpp->action to figure out whether to start the watievent checker.
 			 */
-			mpp->action = ACT_NOTHING;
+			if (mpp->action != ACT_CREATE)
+				mpp->action = ACT_NOTHING;
 		}
 		return DOMAP_OK;
 	}
diff --git a/multipath/main.c b/multipath/main.c
index e60b9bc..ade858d 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -421,6 +421,7 @@ main (int argc, char *argv[])
 			conf->dev_type = DEV_DEVMAP;
 
 	}
+	conf->daemon = 0;
 	dm_init();
 
 	if (conf->remove == FLUSH_ONE) {
diff --git a/multipathd/main.c b/multipathd/main.c
index 7de41a0..323ed7f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1333,7 +1333,7 @@ child (void * param)
 		condlog(0, "can not find sysfs mount point");
 		exit(1);
 	}
-
+	conf->daemon = 1;
 	/*
 	 * fetch and configure both paths and multipaths
 	 */
diff --git a/multipathd/main.h b/multipathd/main.h
index 1a6dc55..b3a90f8 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -1,7 +1,6 @@
 #ifndef MAIN_H
 #define MAIN_H
 
-#define DAEMON 1
 #define MAPGCINT 5
 
 int reconfigure (struct vectors *);
-- 
1.5.4.1

[PATCH] Race-condition fix with free_waiter threads during shutdown.

[Konrad Rzeszutek]

From: Konrad Rzeszutek <konrad@mars.virtualiron.com>

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.
---
 libmultipath/lock.c        |    4 ++--
 libmultipath/lock.h        |   23 ++++++++++++++++-------
 libmultipath/structs_vec.h |    8 +++++++-
 libmultipath/waiter.c      |   14 ++++++++++----
 multipath/main.c           |    1 +
 multipathd/main.c          |   28 +++++++++++++++++-----------
 6 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/libmultipath/lock.c b/libmultipath/lock.c
index 0ca8783..54e2988 100644
--- 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 6afecda..f0e0bfa 100644
--- 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); \
+	        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 19a2387..78e468a 100644
--- 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 54cd19f..e9cde8b 100644
--- 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 ade858d..dacae1f 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -440,6 +440,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 323ed7f..b7532f1 100644
--- 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;
 
-- 
1.5.4.1

Re: [PATCH] Don't display the state of the newly added map during addition in the daemon and don't switch groups.

[Hannes Reinecke]

Hi Konrad,

Konrad Rzeszutek wrote:
> From: Konrad Rzeszutek <konrad@mars.virtualiron.com>
> 
> A previous commit mass-changed #ifdef DAEMON to check for 'mpp->waiter'. Unfortunatly
> when the 'domap' function is called with ACT_CREATE in the daemon, the mpp->waiter is not
> set, hence the multipath client mode logic is choosen. Fixing this triggers another
> issues which is that newly added path via ACT_CREATE won't have their waitevent thread
> created as the caller checks mpp->action (which changed to ACT_NOTHING) and won't
> start the thread.

Nice. Far cleaner fix. But you forgot this:

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index f1975fb..2d024ff 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -13,6 +13,7 @@
 #include "structs.h"
 #include "util.h"
 #include "debug.h"
+#include "config.h"
 
 #define WORD_SIZE 64
 
@@ -297,7 +298,7 @@ disassemble_map (vector pathvec, char * params, struct multi
path * mpp)
                                strncpy(pp->dev_t, word, BLK_DEV_SIZE);
 
                                /* Only call this in multipath client mode */
-                               if (!mpp->waiter && store_path(pathvec, pp))
+                               if (!conf->daemon && store_path(pathvec, pp))
                                        goto out1;
                        }
                        FREE(word);

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)