[PATCH 0/7] multipathd: Fix race conditions related to thread termination

[PATCH 0/7] multipathd: Fix race conditions related to thread termination

[Bart Van Assche]

Hello Christophe,

As you may already have noticed the code in multipathd for terminating 
worker threads triggers several race conditions. It would be appreciated 
if you could review and/or apply the following seven patches that fix 
these race conditions:

0001-multipathd-fix-memory-allocation-logic-error-for-pol.patch
0002-libmultipath-Remove-a-data-structure-that-has-been-c.patch
0003-libmultipath-Remove-debugging-code-from-lock.h.patch
0004-libmultipath-Convert-lock-and-unlock-into-inline-fun.patch
0005-libmultipath-Inline-mutex-in-struct-mutex_lock.patch
0006-libmultipath-Introduce-timedlock.patch
0007-multipathd-Remove-a-busy-waiting-loop.patch

Thanks,

Bart.

[PATCH 1/6] libmultipath: Remove a data structure that has been commented out

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/structs_vec.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 8ef547d..46f30af 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -4,11 +4,7 @@
 #include "vector.h"
 #include "config.h"
 #include "lock.h"
-/*
-struct mutex_lock {
-	pthread_mutex_t *mutex;
-	int depth;
-}; */
+
 struct vectors {
 	struct mutex_lock lock; /* defined in lock.h */
 	vector pathvec;
-- 
2.9.2

[PATCH 2/6] libmultipath: Remove debugging code from lock.h

[Bart Van Assche]

There are better tools available to trace mutex lock and unlock
activity than the macros in this header file, e.g.
valgrind --tool=drd --trace-mutex=yes multipathd -d
Hence remove the debugging macros.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/lock.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index c569f01..71baf10 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -12,21 +12,9 @@ struct mutex_lock {
 	int depth;
 };
 
-#ifdef LCKDBG
-#define 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 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 depth: %d (%ld)\n", __FILE__, __FUNCTION__, __LINE__, a.mutex, a.depth, pthread_self()); \
-	pthread_cleanup_pop(1)
-#else
 #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
 
 void cleanup_lock (void * data);
 
-- 
2.9.2

[PATCH 3/6] libmultipath: Convert lock() and unlock() into inline functions

[Bart Van Assche]

Convert lock() and unlock() from macros into inline functions.
This conversion makes it possible for the compiler to perform
type checking on the lock() and unlock() arguments. However,
this makes it necessary to change the argument type from "struct
mutex_lock" into "struct mutex_lock *".

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/lock.c   |  4 +++-
 libmultipath/lock.h   | 14 ++++++++++++--
 libmultipath/waiter.c |  2 +-
 multipathd/cli.c      |  2 +-
 multipathd/main.c     | 28 ++++++++++++++--------------
 5 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/libmultipath/lock.c b/libmultipath/lock.c
index 8d7b2ad..72c70e3 100644
--- a/libmultipath/lock.c
+++ b/libmultipath/lock.c
@@ -2,5 +2,7 @@
 
 void cleanup_lock (void * data)
 {
-	unlock ((*(struct mutex_lock *)data));
+	struct mutex_lock *lock = data;
+
+	unlock(lock);
 }
diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index 71baf10..461b095 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -12,8 +12,18 @@ struct mutex_lock {
 	int depth;
 };
 
-#define lock(a) a.depth++; pthread_mutex_lock(a.mutex)
-#define unlock(a) a.depth--; pthread_mutex_unlock(a.mutex)
+static inline void lock(struct mutex_lock *a)
+{
+	a->depth++;
+	pthread_mutex_lock(a->mutex);
+}
+
+static inline void unlock(struct mutex_lock *a)
+{
+	a->depth--;
+	pthread_mutex_unlock(a->mutex);
+}
+
 #define lock_cleanup_pop(a) pthread_cleanup_pop(1)
 
 void cleanup_lock (void * data);
diff --git a/libmultipath/waiter.c b/libmultipath/waiter.c
index 4079b13..995ea1a 100644
--- a/libmultipath/waiter.c
+++ b/libmultipath/waiter.c
@@ -138,7 +138,7 @@ static int waiteventloop (struct event_thread *waiter)
 		 * 5) a switch group : nothing to do
 		 */
 		pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock);
-		lock(waiter->vecs->lock);
+		lock(&waiter->vecs->lock);
 		pthread_testcancel();
 		r = update_multipath(waiter->vecs, waiter->mapname, 1);
 		lock_cleanup_pop(waiter->vecs->lock);
diff --git a/multipathd/cli.c b/multipathd/cli.c
index 3663c0a..01b5ac8 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -490,7 +490,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 			vecs->lock.depth++;
 			r = pthread_mutex_timedlock(vecs->lock.mutex, &tmo);
 		} else {
-			lock(vecs->lock);
+			lock(&vecs->lock);
 			r = 0;
 		}
 		if (r == 0) {
diff --git a/multipathd/main.c b/multipathd/main.c
index f5e9a01..e0dc045 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -451,7 +451,7 @@ uev_add_map (struct uevent * uev, struct vectors * vecs)
 		}
 	}
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
-	lock(vecs->lock);
+	lock(&vecs->lock);
 	pthread_testcancel();
 	rc = ev_add_map(uev->kernel, alias, vecs);
 	lock_cleanup_pop(vecs->lock);
@@ -557,7 +557,7 @@ uev_remove_map (struct uevent * uev, struct vectors * vecs)
 	minor = uevent_get_minor(uev);
 
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
-	lock(vecs->lock);
+	lock(&vecs->lock);
 	pthread_testcancel();
 	mpp = find_mp_by_minor(vecs->mpvec, minor);
 
@@ -618,7 +618,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
 	}
 
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
-	lock(vecs->lock);
+	lock(&vecs->lock);
 	pthread_testcancel();
 	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
 	if (pp) {
@@ -668,7 +668,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
 		return 1;
 	}
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
-	lock(vecs->lock);
+	lock(&vecs->lock);
 	pthread_testcancel();
 	ret = store_path(vecs->pathvec, pp);
 	if (!ret) {
@@ -831,7 +831,7 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs)
 
 	condlog(2, "%s: remove path (uevent)", uev->kernel);
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
-	lock(vecs->lock);
+	lock(&vecs->lock);
 	pthread_testcancel();
 	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
 	if (pp)
@@ -957,7 +957,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 		condlog(2, "%s: update path write_protect to '%d' (uevent)",
 			uev->kernel, ro);
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
-		lock(vecs->lock);
+		lock(&vecs->lock);
 		pthread_testcancel();
 		/*
 		 * pthread_mutex_lock() and pthread_mutex_unlock()
@@ -1794,7 +1794,7 @@ checkerloop (void *ap)
 		}
 		if (vecs->pathvec) {
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
-			lock(vecs->lock);
+			lock(&vecs->lock);
 			pthread_testcancel();
 			vector_foreach_slot (vecs->pathvec, pp, i) {
 				rc = check_path(vecs, pp, ticks);
@@ -1810,7 +1810,7 @@ checkerloop (void *ap)
 		}
 		if (vecs->mpvec) {
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
-			lock(vecs->lock);
+			lock(&vecs->lock);
 			pthread_testcancel();
 			defered_failback_tick(vecs->mpvec);
 			retry_count_tick(vecs->mpvec);
@@ -1821,7 +1821,7 @@ checkerloop (void *ap)
 			count--;
 		else {
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
-			lock(vecs->lock);
+			lock(&vecs->lock);
 			pthread_testcancel();
 			condlog(4, "map garbage collection");
 			mpvec_garbage_collector(vecs);
@@ -2377,7 +2377,7 @@ child (void * param)
 		pthread_cleanup_pop(1);
 		if (running_state == DAEMON_CONFIGURE) {
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
-			lock(vecs->lock);
+			lock(&vecs->lock);
 			pthread_testcancel();
 			if (!need_to_delay_reconfig(vecs)) {
 				reconfigure(vecs);
@@ -2391,24 +2391,24 @@ child (void * param)
 		}
 	}
 
-	lock(vecs->lock);
+	lock(&vecs->lock);
 	conf = get_multipath_config();
 	if (conf->queue_without_daemon == QUE_NO_DAEMON_OFF)
 		vector_foreach_slot(vecs->mpvec, mpp, i)
 			dm_queue_if_no_path(mpp->alias, 0);
 	put_multipath_config(conf);
 	remove_maps_and_stop_waiters(vecs);
-	unlock(vecs->lock);
+	unlock(&vecs->lock);
 
 	pthread_cancel(check_thr);
 	pthread_cancel(uevent_thr);
 	pthread_cancel(uxlsnr_thr);
 	pthread_cancel(uevq_thr);
 
-	lock(vecs->lock);
+	lock(&vecs->lock);
 	free_pathvec(vecs->pathvec, FREE_PATHS);
 	vecs->pathvec = NULL;
-	unlock(vecs->lock);
+	unlock(&vecs->lock);
 	/* Now all the waitevent threads will start rushing in. */
 	while (vecs->lock.depth > 0) {
 		sleep (1); /* This is weak. */
-- 
2.9.2

[PATCH 4/6] libmultipath: Inline mutex in struct mutex_lock

[Bart Van Assche]

This simplifies the struct mutex_lock allocation and deallocation
code.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/lock.h |  6 +++---
 multipathd/cli.c    |  2 +-
 multipathd/main.c   | 18 ++----------------
 3 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index 461b095..dc83336 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -8,20 +8,20 @@
  * track of how many there are out-standing threads blocking
  * on a mutex. */
 struct mutex_lock {
-	pthread_mutex_t *mutex;
+	pthread_mutex_t mutex;
 	int depth;
 };
 
 static inline void lock(struct mutex_lock *a)
 {
 	a->depth++;
-	pthread_mutex_lock(a->mutex);
+	pthread_mutex_lock(&a->mutex);
 }
 
 static inline void unlock(struct mutex_lock *a)
 {
 	a->depth--;
-	pthread_mutex_unlock(a->mutex);
+	pthread_mutex_unlock(&a->mutex);
 }
 
 #define lock_cleanup_pop(a) pthread_cleanup_pop(1)
diff --git a/multipathd/cli.c b/multipathd/cli.c
index 01b5ac8..9597736 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -488,7 +488,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
 		if (tmo.tv_sec) {
 			vecs->lock.depth++;
-			r = pthread_mutex_timedlock(vecs->lock.mutex, &tmo);
+			r = pthread_mutex_timedlock(&vecs->lock.mutex, &tmo);
 		} else {
 			lock(&vecs->lock);
 			r = 0;
diff --git a/multipathd/main.c b/multipathd/main.c
index e0dc045..fff482c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2045,21 +2045,10 @@ init_vecs (void)
 	if (!vecs)
 		return NULL;
 
-	vecs->lock.mutex =
-		(pthread_mutex_t *)MALLOC(sizeof(pthread_mutex_t));
-
-	if (!vecs->lock.mutex)
-		goto out;
-
-	pthread_mutex_init(vecs->lock.mutex, NULL);
+	pthread_mutex_init(&vecs->lock.mutex, NULL);
 	vecs->lock.depth = 0;
 
 	return vecs;
-
-out:
-	FREE(vecs);
-	condlog(0, "failed to init paths");
-	return NULL;
 }
 
 static void *
@@ -2415,10 +2404,7 @@ child (void * param)
 		condlog(3, "Have %d wait event checkers threads to de-alloc,"
 			" waiting...", vecs->lock.depth);
 	}
-	pthread_mutex_destroy(vecs->lock.mutex);
-	FREE(vecs->lock.mutex);
-	vecs->lock.depth = 0;
-	vecs->lock.mutex = NULL;
+	pthread_mutex_destroy(&vecs->lock.mutex);
 	FREE(vecs);
 	vecs = NULL;
 
-- 
2.9.2

[PATCH 5/6] libmultipath: Introduce timedlock()

[Bart Van Assche]

Introduce the function timedlock(). Ensure that the value of the
"depth" member value is incorrect if pthread_mutex_timedlock()
times out.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/lock.h | 11 +++++++++++
 multipathd/cli.c    |  3 +--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index dc83336..9808480 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -18,6 +18,17 @@ static inline void lock(struct mutex_lock *a)
 	pthread_mutex_lock(&a->mutex);
 }
 
+static inline int timedlock(struct mutex_lock *a, struct timespec *tmo)
+{
+	int r;
+
+	a->depth++;
+	r = pthread_mutex_timedlock(&a->mutex, tmo);
+	if (r)
+		a->depth--;
+	return r;
+}
+
 static inline void unlock(struct mutex_lock *a)
 {
 	a->depth--;
diff --git a/multipathd/cli.c b/multipathd/cli.c
index 9597736..9a19728 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -487,8 +487,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
 		if (tmo.tv_sec) {
-			vecs->lock.depth++;
-			r = pthread_mutex_timedlock(&vecs->lock.mutex, &tmo);
+			r = timedlock(&vecs->lock, &tmo);
 		} else {
 			lock(&vecs->lock);
 			r = 0;
-- 
2.9.2

[PATCH 6/6] multipathd: Remove a busy-waiting loop

[Bart Van Assche]

Use pthread_join() to wait until worker threads have finished
instead of using a counter to keep track of how many threads are
trying to grab a mutex. Remove mutex_lock.depth since this member
variable is no longer needed.

This patch fixes two race conditions:
* Incrementing "depth" in lock() without holding a mutex.
* Destroying a mutex from the main thread without waiting for the
  worker threads to finish using that mutex.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/lock.h | 15 +--------------
 multipathd/main.c   | 13 ++++++-------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index 9808480..a170efe 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -3,35 +3,22 @@
 
 #include <pthread.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;
 };
 
 static inline void lock(struct mutex_lock *a)
 {
-	a->depth++;
 	pthread_mutex_lock(&a->mutex);
 }
 
 static inline int timedlock(struct mutex_lock *a, struct timespec *tmo)
 {
-	int r;
-
-	a->depth++;
-	r = pthread_mutex_timedlock(&a->mutex, tmo);
-	if (r)
-		a->depth--;
-	return r;
+	return pthread_mutex_timedlock(&a->mutex, tmo);
 }
 
 static inline void unlock(struct mutex_lock *a)
 {
-	a->depth--;
 	pthread_mutex_unlock(&a->mutex);
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index fff482c..c288195 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2046,7 +2046,6 @@ init_vecs (void)
 		return NULL;
 
 	pthread_mutex_init(&vecs->lock.mutex, NULL);
-	vecs->lock.depth = 0;
 
 	return vecs;
 }
@@ -2394,16 +2393,16 @@ child (void * param)
 	pthread_cancel(uxlsnr_thr);
 	pthread_cancel(uevq_thr);
 
+	pthread_join(check_thr, NULL);
+	pthread_join(uevent_thr, NULL);
+	pthread_join(uxlsnr_thr, NULL);
+	pthread_join(uevq_thr, NULL);
+
 	lock(&vecs->lock);
 	free_pathvec(vecs->pathvec, FREE_PATHS);
 	vecs->pathvec = NULL;
 	unlock(&vecs->lock);
-	/* 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...", vecs->lock.depth);
-	}
+
 	pthread_mutex_destroy(&vecs->lock.mutex);
 	FREE(vecs);
 	vecs = NULL;
-- 
2.9.2

Re: [PATCH 0/7] multipathd: Fix race conditions related to thread termination

[Bart Van Assche]

On 08/15/2016 08:24 AM, Bart Van Assche wrote:
> As you may already have noticed the code in multipathd for terminating
> worker threads triggers several race conditions. It would be appreciated
> if you could review and/or apply the following seven patches that fix
> these race conditions:
>
> 0001-multipathd-fix-memory-allocation-logic-error-for-pol.patch
> 0002-libmultipath-Remove-a-data-structure-that-has-been-c.patch
> 0003-libmultipath-Remove-debugging-code-from-lock.h.patch
> 0004-libmultipath-Convert-lock-and-unlock-into-inline-fun.patch
> 0005-libmultipath-Inline-mutex-in-struct-mutex_lock.patch
> 0006-libmultipath-Introduce-timedlock.patch
> 0007-multipathd-Remove-a-busy-waiting-loop.patch

(replying to my own e-mail)

A correction: this series consists of 6 patches instead of 7.

Bart.

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Hannes Reinecke]

On 08/15/2016 05:28 PM, Bart Van Assche wrote:
> Use pthread_join() to wait until worker threads have finished
> instead of using a counter to keep track of how many threads are
> trying to grab a mutex. Remove mutex_lock.depth since this member
> variable is no longer needed.
> 
> This patch fixes two race conditions:
> * Incrementing "depth" in lock() without holding a mutex.
> * Destroying a mutex from the main thread without waiting for the
>   worker threads to finish using that mutex.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
>  libmultipath/lock.h | 15 +--------------
>  multipathd/main.c   | 13 ++++++-------
>  2 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/libmultipath/lock.h b/libmultipath/lock.h
> index 9808480..a170efe 100644
> --- a/libmultipath/lock.h
> +++ b/libmultipath/lock.h
> @@ -3,35 +3,22 @@
>  
>  #include <pthread.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;
>  };
>  
>  static inline void lock(struct mutex_lock *a)
>  {
> -	a->depth++;
>  	pthread_mutex_lock(&a->mutex);
>  }
>  
>  static inline int timedlock(struct mutex_lock *a, struct timespec *tmo)
>  {
> -	int r;
> -
> -	a->depth++;
> -	r = pthread_mutex_timedlock(&a->mutex, tmo);
> -	if (r)
> -		a->depth--;
> -	return r;
> +	return pthread_mutex_timedlock(&a->mutex, tmo);
>  }
>  
>  static inline void unlock(struct mutex_lock *a)
>  {
> -	a->depth--;
>  	pthread_mutex_unlock(&a->mutex);
>  }
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index fff482c..c288195 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2046,7 +2046,6 @@ init_vecs (void)
>  		return NULL;
>  
>  	pthread_mutex_init(&vecs->lock.mutex, NULL);
> -	vecs->lock.depth = 0;
>  
>  	return vecs;
>  }
> @@ -2394,16 +2393,16 @@ child (void * param)
>  	pthread_cancel(uxlsnr_thr);
>  	pthread_cancel(uevq_thr);
>  
> +	pthread_join(check_thr, NULL);
> +	pthread_join(uevent_thr, NULL);
> +	pthread_join(uxlsnr_thr, NULL);
> +	pthread_join(uevq_thr, NULL);
> +
>  	lock(&vecs->lock);
>  	free_pathvec(vecs->pathvec, FREE_PATHS);
>  	vecs->pathvec = NULL;
>  	unlock(&vecs->lock);
> -	/* 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...", vecs->lock.depth);
> -	}
> +
>  	pthread_mutex_destroy(&vecs->lock.mutex);
>  	FREE(vecs);
>  	vecs = NULL;
> 

Makes one wonder: what happens to the waitevent threads?
We won't be waiting for them after applying this patch, right?
So why did we ever had this busy loop here?
Ben?

(And while we're at the subject: can't we drop the waitevent threads
altogether? We're listening to uevents nowadays, so we should be
notified if something happened to the device-mapper tables. Which should
make the waitevent threads unnecessary, right?)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 0/7] multipathd: Fix race conditions related to thread termination

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 943 bytes --]

Applied.
Thanks.

On Mon, Aug 15, 2016 at 5:29 PM, Bart Van Assche <bart.vanassche@sandisk.com
> wrote:

> On 08/15/2016 08:24 AM, Bart Van Assche wrote:
>
>> As you may already have noticed the code in multipathd for terminating
>> worker threads triggers several race conditions. It would be appreciated
>> if you could review and/or apply the following seven patches that fix
>> these race conditions:
>>
>> 0001-multipathd-fix-memory-allocation-logic-error-for-pol.patch
>> 0002-libmultipath-Remove-a-data-structure-that-has-been-c.patch
>> 0003-libmultipath-Remove-debugging-code-from-lock.h.patch
>> 0004-libmultipath-Convert-lock-and-unlock-into-inline-fun.patch
>> 0005-libmultipath-Inline-mutex-in-struct-mutex_lock.patch
>> 0006-libmultipath-Introduce-timedlock.patch
>> 0007-multipathd-Remove-a-busy-waiting-loop.patch
>>
>
> (replying to my own e-mail)
>
> A correction: this series consists of 6 patches instead of 7.
>
> Bart.
>

[-- Attachment #1.2: Type: text/html, Size: 1543 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On 08/15/2016 11:31 PM, Hannes Reinecke wrote:
> Makes one wonder: what happens to the waitevent threads?
> We won't be waiting for them after applying this patch, right?
> So why did we ever had this busy loop here?
> Ben?
>
> (And while we're at the subject: can't we drop the waitevent threads
> altogether? We're listening to uevents nowadays, so we should be
> notified if something happened to the device-mapper tables. Which should
> make the waitevent threads unnecessary, right?)

Hello Hannes,

Maybe this is not what you had in mind, but would you agree with the 
attached two patches?

Thanks,

Bart.



[-- Attachment #2: 0001-libmultipath-waiter.c-Call-pthread_join-upon-thread-.patch --]
[-- Type: text/x-patch, Size: 1287 bytes --]

From b9e2113b5793706b2d28f4096faad919a625dd9f Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Tue, 16 Aug 2016 08:56:44 -0700
Subject: [PATCH 1/2] libmultipath/waiter.c: Call pthread_join() upon thread
 exit

pthread_kill() delivers a signal asynchronously. Hence add a
pthread_join() call in stop_waiter_thread() to wait until the
waiter thread has stopped. The following section from the
pthread_join() manpage is relevant in this context:

  Failure to join with a thread that is joinable (i.e., one that is not
  detached), produces a "zombie thread". Avoid doing this, since each
  zombie thread consumes some system resources, and when enough zombie
  threads have accumulated, it will no longer be possible to create new
  threads (or processes).

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/waiter.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/waiter.c b/libmultipath/waiter.c
index 995ea1a..6692753 100644
--- a/libmultipath/waiter.c
+++ b/libmultipath/waiter.c
@@ -61,6 +61,7 @@ void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 	mpp->waiter = (pthread_t)0;
 	pthread_cancel(thread);
 	pthread_kill(thread, SIGUSR2);
+	pthread_join(thread, NULL);
 }
 
 /*
-- 
2.9.2


[-- Attachment #3: 0002-libmultipath-checkers-tur-Call-pthread_join-upon-thr.patch --]
[-- Type: text/x-patch, Size: 3685 bytes --]

From 16764e4699efd57321b95f07b4a0553b9f33598a Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Tue, 16 Aug 2016 09:04:02 -0700
Subject: [PATCH 2/2] libmultipath/checkers/tur: Call pthread_join() upon
 thread exit

pthread_cancel() cancels a thread asynchronously. Hence add a
pthread_join() call to avoid that the tur_checker_context is freed
before the tur_thread() function has finished. Introduce a new
variable to indicate whether or not the TUR thread is running such
that the thread ID can be preserved if a TUR thread exits. Ensure
that this new variable is protected consistently by
tur_checker_context.hldr_lock.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/checkers/tur.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index ad66918..7b789e0 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -43,6 +43,7 @@ struct tur_checker_context {
 	pthread_cond_t active;
 	pthread_spinlock_t hldr_lock;
 	int holders;
+	unsigned char thread_running:1;
 	char message[CHECKER_MSG_LEN];
 };
 
@@ -68,11 +69,24 @@ int libcheck_init (struct checker * c)
 	return 0;
 }
 
+static unsigned checker_thread_running(struct tur_checker_context *ct)
+{
+	unsigned thread_running;
+
+	pthread_spin_lock(&ct->hldr_lock);
+	thread_running = ct->thread_running;
+	pthread_spin_unlock(&ct->hldr_lock);
+
+	return thread_running;
+}
+
 void cleanup_context(struct tur_checker_context *ct)
 {
 	pthread_mutex_destroy(&ct->lock);
 	pthread_cond_destroy(&ct->active);
 	pthread_spin_destroy(&ct->hldr_lock);
+	if (ct->thread)
+		pthread_join(ct->thread, NULL);
 	free(ct);
 }
 
@@ -198,7 +212,7 @@ void cleanup_func(void *data)
 	pthread_spin_lock(&ct->hldr_lock);
 	ct->holders--;
 	holders = ct->holders;
-	ct->thread = 0;
+	ct->thread_running = 0;
 	pthread_spin_unlock(&ct->hldr_lock);
 	if (!holders)
 		cleanup_context(ct);
@@ -295,7 +309,7 @@ libcheck_check (struct checker * c)
 
 	if (ct->running) {
 		/* Check if TUR checker is still running */
-		if (ct->thread) {
+		if (checker_thread_running(ct)) {
 			if (tur_check_async_timeout(c)) {
 				condlog(3, "%d:%d: tur checker timeout",
 					TUR_DEVT(ct));
@@ -318,7 +332,7 @@ libcheck_check (struct checker * c)
 		}
 		pthread_mutex_unlock(&ct->lock);
 	} else {
-		if (ct->thread) {
+		if (checker_thread_running(ct)) {
 			/* pthread cancel failed. continue in sync mode */
 			pthread_mutex_unlock(&ct->lock);
 			condlog(3, "%d:%d: tur thread not responding",
@@ -331,6 +345,7 @@ libcheck_check (struct checker * c)
 		ct->timeout = c->timeout;
 		pthread_spin_lock(&ct->hldr_lock);
 		ct->holders++;
+		ct->thread_running = 1;
 		pthread_spin_unlock(&ct->hldr_lock);
 		tur_set_async_timeout(c);
 		setup_thread_attr(&attr, 32 * 1024, 1);
@@ -338,9 +353,9 @@ libcheck_check (struct checker * c)
 		if (r) {
 			pthread_spin_lock(&ct->hldr_lock);
 			ct->holders--;
+			ct->thread_running = 0;
 			pthread_spin_unlock(&ct->hldr_lock);
 			pthread_mutex_unlock(&ct->lock);
-			ct->thread = 0;
 			condlog(3, "%d:%d: failed to start tur thread, using"
 				" sync mode", TUR_DEVT(ct));
 			return tur_check(c->fd, c->timeout, c->message);
@@ -352,7 +367,7 @@ libcheck_check (struct checker * c)
 		strncpy(c->message, ct->message,CHECKER_MSG_LEN);
 		c->message[CHECKER_MSG_LEN - 1] = '\0';
 		pthread_mutex_unlock(&ct->lock);
-		if (ct->thread &&
+		if (checker_thread_running(ct) &&
 		    (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
 			condlog(3, "%d:%d: tur checker still running",
 				TUR_DEVT(ct));
-- 
2.9.2


[-- Attachment #4: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Hannes Reinecke]

On 08/16/2016 10:11 PM, Bart Van Assche wrote:
> On 08/15/2016 11:31 PM, Hannes Reinecke wrote:
>> Makes one wonder: what happens to the waitevent threads?
>> We won't be waiting for them after applying this patch, right?
>> So why did we ever had this busy loop here?
>> Ben?
>>
>> (And while we're at the subject: can't we drop the waitevent threads
>> altogether? We're listening to uevents nowadays, so we should be
>> notified if something happened to the device-mapper tables. Which should
>> make the waitevent threads unnecessary, right?)
> 
> Hello Hannes,
> 
> Maybe this is not what you had in mind, but would you agree with the
> attached two patches?
> 
Well, yes, sort of.
However, it still would mean that the main multipath daemon thread might
terminate before the event threads have completed, though.

Not sure if it matters, just wanted to bring is up.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Bart Van Assche]

On 08/17/2016 07:44 AM, Hannes Reinecke wrote:
> On 08/16/2016 10:11 PM, Bart Van Assche wrote:
>> On 08/15/2016 11:31 PM, Hannes Reinecke wrote:
>>> Makes one wonder: what happens to the waitevent threads?
>>> We won't be waiting for them after applying this patch, right?
>>> So why did we ever had this busy loop here?
>>> Ben?
>>>
>>> (And while we're at the subject: can't we drop the waitevent threads
>>> altogether? We're listening to uevents nowadays, so we should be
>>> notified if something happened to the device-mapper tables. Which should
>>> make the waitevent threads unnecessary, right?)
>>
>> Hello Hannes,
>>
>> Maybe this is not what you had in mind, but would you agree with the
>> attached two patches?
>>
> Well, yes, sort of.
> However, it still would mean that the main multipath daemon thread might
> terminate before the event threads have completed, though.
>
> Not sure if it matters, just wanted to bring is up.

Hello Hannes,

I think that for the tur checker the libcheck_free() function should 
wait until the tur thread has finished. multipathd calls 
remove_maps_and_stop_waiters() during shutdown and that function calls 
libcheck_free() for all checkers that were loaded.

Regarding the waiter threads: stop_waiter_thread() is called by 
_remove_map() so the waiter threads are also stopped during multipathd 
shutdown.

Bart.

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Dragan Stancevic]

[-- Attachment #1.1: Type: text/plain, Size: 3403 bytes --]

Hi Bart-

Acked-by: dragan.stancevic@canonical.com

I agree with your patch, I have been tracking an issue where multipathd
dumps core on the exit path just past the treads being canceled. Your patch
is very similar to mine (minus nuking the depth) that I was going to send
out to a user to test with. The checker thread accesses a valid pointer
with garbage values....

On Mon, Aug 15, 2016 at 10:28 AM, Bart Van Assche <
bart.vanassche@sandisk.com> wrote:

> Use pthread_join() to wait until worker threads have finished
> instead of using a counter to keep track of how many threads are
> trying to grab a mutex. Remove mutex_lock.depth since this member
> variable is no longer needed.
>
> This patch fixes two race conditions:
> * Incrementing "depth" in lock() without holding a mutex.
> * Destroying a mutex from the main thread without waiting for the
>   worker threads to finish using that mutex.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
>  libmultipath/lock.h | 15 +--------------
>  multipathd/main.c   | 13 ++++++-------
>  2 files changed, 7 insertions(+), 21 deletions(-)
>
> diff --git a/libmultipath/lock.h b/libmultipath/lock.h
> index 9808480..a170efe 100644
> --- a/libmultipath/lock.h
> +++ b/libmultipath/lock.h
> @@ -3,35 +3,22 @@
>
>  #include <pthread.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;
>  };
>
>  static inline void lock(struct mutex_lock *a)
>  {
> -       a->depth++;
>         pthread_mutex_lock(&a->mutex);
>  }
>
>  static inline int timedlock(struct mutex_lock *a, struct timespec *tmo)
>  {
> -       int r;
> -
> -       a->depth++;
> -       r = pthread_mutex_timedlock(&a->mutex, tmo);
> -       if (r)
> -               a->depth--;
> -       return r;
> +       return pthread_mutex_timedlock(&a->mutex, tmo);
>  }
>
>  static inline void unlock(struct mutex_lock *a)
>  {
> -       a->depth--;
>         pthread_mutex_unlock(&a->mutex);
>  }
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index fff482c..c288195 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2046,7 +2046,6 @@ init_vecs (void)
>                 return NULL;
>
>         pthread_mutex_init(&vecs->lock.mutex, NULL);
> -       vecs->lock.depth = 0;
>
>         return vecs;
>  }
> @@ -2394,16 +2393,16 @@ child (void * param)
>         pthread_cancel(uxlsnr_thr);
>         pthread_cancel(uevq_thr);
>
> +       pthread_join(check_thr, NULL);
> +       pthread_join(uevent_thr, NULL);
> +       pthread_join(uxlsnr_thr, NULL);
> +       pthread_join(uevq_thr, NULL);
> +
>         lock(&vecs->lock);
>         free_pathvec(vecs->pathvec, FREE_PATHS);
>         vecs->pathvec = NULL;
>         unlock(&vecs->lock);
> -       /* 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...", vecs->lock.depth);
> -       }
> +
>         pthread_mutex_destroy(&vecs->lock.mutex);
>         FREE(vecs);
>         vecs = NULL;
> --
> 2.9.2
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>

[-- Attachment #1.2: Type: text/html, Size: 4700 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Dragan Stancevic]

[-- Attachment #1.1: Type: text/plain, Size: 1294 bytes --]

Hi Bart-

are the semantics below really safe, the don't appear to? Any caller of
checker_thread_running is relaying on stale information. The
c->thread_running could have changed as soon as you unlock.

+static unsigned checker_thread_running(struct tur_checker_context *ct)
+{
+ unsigned thread_running;
+
+ pthread_spin_lock(&ct->hldr_lock);
+ thread_running = ct->thread_running;
+ pthread_spin_unlock(&ct->hldr_lock);
+
+ return thread_running;
+}


On Tue, Aug 16, 2016 at 3:11 PM, Bart Van Assche <bart.vanassche@sandisk.com
> wrote:

> On 08/15/2016 11:31 PM, Hannes Reinecke wrote:
>
>> Makes one wonder: what happens to the waitevent threads?
>> We won't be waiting for them after applying this patch, right?
>> So why did we ever had this busy loop here?
>> Ben?
>>
>> (And while we're at the subject: can't we drop the waitevent threads
>> altogether? We're listening to uevents nowadays, so we should be
>> notified if something happened to the device-mapper tables. Which should
>> make the waitevent threads unnecessary, right?)
>>
>
> Hello Hannes,
>
> Maybe this is not what you had in mind, but would you agree with the
> attached two patches?
>
> Thanks,
>
> Bart.
>
>
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2373 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Bart Van Assche]

On 08/17/2016 12:42 PM, Dragan Stancevic wrote:
> Hi Bart-
>
> are the semantics below really safe, the don't appear to? Any caller of
> checker_thread_running is relaying on stale information. The
> c->thread_running could have changed as soon as you unlock.
>
> +static unsigned checker_thread_running(struct tur_checker_context *ct)
> +{
> +unsigned thread_running;
> +
> +pthread_spin_lock(&ct->hldr_lock);
> +thread_running = ct->thread_running;
> +pthread_spin_unlock(&ct->hldr_lock);
> +
> +return thread_running;
> +}

Hello Dragan,

If you have a look at libcheck_check() in tur.c then you will see that 
it is harmless if checker_thread_running() returns old information.

Bart.

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On 08/17/2016 12:36 PM, Dragan Stancevic wrote:
> Acked-by: dragan.stancevic@canonical.com
> <mailto:dragan.stancevic@canonical.com>
>
> I agree with your patch, I have been tracking an issue where multipathd
> dumps core on the exit path just past the treads being canceled. Your
> patch is very similar to mine (minus nuking the depth) that I was going
> to send out to a user to test with. The checker thread accesses a valid
> pointer with garbage values....

Hello Dragan,

When I prepared my patch I overlooked that multipathd creates most 
threads as detached threads. So I started testing the three attached 
patches on my setup. It would be appreciated if you could have a look at 
these patches.

Thanks,

Bart.

[-- Attachment #2: 0001-multipathd-Change-four-threads-from-detached-into-jo.patch --]
[-- Type: text/x-patch, Size: 1124 bytes --]

From 7d8b5dc15d53f43ae16f7ebebc96052e6208f566 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 17 Aug 2016 09:52:54 -0700
Subject: [PATCH 1/3] multipathd: Change four threads from detached into
 joinable

Change the CLI listener, checker loop, uevent dispatcher and
uevent listener threads from detached into joinable threads.
This change is needed because calling pthread_join() is only
allowed for joinable threads.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 multipathd/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 2be6cb2..6b3c856 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2216,8 +2216,8 @@ child (void * param)
 	signal_init();
 	rcu_init();
 
-	setup_thread_attr(&misc_attr, 64 * 1024, 1);
-	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 1);
+	setup_thread_attr(&misc_attr, 64 * 1024, 0);
+	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
 	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
 
 	if (logsink == 1) {
-- 
2.9.2


[-- Attachment #3: 0002-libmultipath-checkers-tur-Introduce-checker_thread_r.patch --]
[-- Type: text/x-patch, Size: 2134 bytes --]

From 4d0c245641d80a5fd10833333dc288a5a9293ee9 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 17 Aug 2016 09:57:39 -0700
Subject: [PATCH 2/3] libmultipath/checkers/tur: Introduce
 checker_thread_running()

Additionally, protect tur_checker_context.thread reads via
hldr_lock. This avoids that data race detection tools complain
about reads of the member variable 'thread'.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/checkers/tur.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index ad66918..c014b65 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -68,6 +68,17 @@ int libcheck_init (struct checker * c)
 	return 0;
 }
 
+static int checker_thread_running(struct tur_checker_context *ct)
+{
+	pthread_t thread;
+
+	pthread_spin_lock(&ct->hldr_lock);
+	thread = ct->thread;
+	pthread_spin_unlock(&ct->hldr_lock);
+
+	return thread != 0;
+}
+
 void cleanup_context(struct tur_checker_context *ct)
 {
 	pthread_mutex_destroy(&ct->lock);
@@ -295,7 +306,7 @@ libcheck_check (struct checker * c)
 
 	if (ct->running) {
 		/* Check if TUR checker is still running */
-		if (ct->thread) {
+		if (checker_thread_running(ct)) {
 			if (tur_check_async_timeout(c)) {
 				condlog(3, "%d:%d: tur checker timeout",
 					TUR_DEVT(ct));
@@ -318,7 +329,7 @@ libcheck_check (struct checker * c)
 		}
 		pthread_mutex_unlock(&ct->lock);
 	} else {
-		if (ct->thread) {
+		if (checker_thread_running(ct)) {
 			/* pthread cancel failed. continue in sync mode */
 			pthread_mutex_unlock(&ct->lock);
 			condlog(3, "%d:%d: tur thread not responding",
@@ -352,7 +363,7 @@ libcheck_check (struct checker * c)
 		strncpy(c->message, ct->message,CHECKER_MSG_LEN);
 		c->message[CHECKER_MSG_LEN - 1] = '\0';
 		pthread_mutex_unlock(&ct->lock);
-		if (ct->thread &&
+		if (checker_thread_running(ct) &&
 		    (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
 			condlog(3, "%d:%d: tur checker still running",
 				TUR_DEVT(ct));
-- 
2.9.2


[-- Attachment #4: 0003-libmultipath-checkers-tur-Fix-race-related-to-thread.patch --]
[-- Type: text/x-patch, Size: 2384 bytes --]

From e1144d5321f87bc72e6fb29b40cf7728125aa926 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 17 Aug 2016 09:58:09 -0700
Subject: [PATCH 3/3] libmultipath/checkers/tur: Fix race related to thread
 termination

Since the tur thread is a detached thread, all data structures
associated with that thread are freed once the thread stops. Avoid
that pthread_cancel() triggers a use-after-free by protecting
pthread_cancel() calls with the same spinlock that protects ct->thread.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/checkers/tur.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index c014b65..f724350 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2004 Christophe Varoqui
  */
+#include <assert.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -98,10 +99,11 @@ void libcheck_free (struct checker * c)
 		ct->holders--;
 		holders = ct->holders;
 		thread = ct->thread;
-		pthread_spin_unlock(&ct->hldr_lock);
 		if (holders)
 			pthread_cancel(thread);
-		else
+		pthread_spin_unlock(&ct->hldr_lock);
+
+		if (!holders)
 			cleanup_context(ct);
 		c->context = NULL;
 	}
@@ -207,6 +209,7 @@ void cleanup_func(void *data)
 	int holders;
 	struct tur_checker_context *ct = data;
 	pthread_spin_lock(&ct->hldr_lock);
+	assert(ct->holders > 0);
 	ct->holders--;
 	holders = ct->holders;
 	ct->thread = 0;
@@ -310,7 +313,12 @@ libcheck_check (struct checker * c)
 			if (tur_check_async_timeout(c)) {
 				condlog(3, "%d:%d: tur checker timeout",
 					TUR_DEVT(ct));
-				pthread_cancel(ct->thread);
+
+				pthread_spin_lock(&ct->hldr_lock);
+				if (ct->holders > 0)
+					pthread_cancel(ct->thread);
+				pthread_spin_unlock(&ct->hldr_lock);
+
 				ct->running = 0;
 				MSG(c, MSG_TUR_TIMEOUT);
 				tur_status = PATH_TIMEOUT;
@@ -349,9 +357,9 @@ libcheck_check (struct checker * c)
 		if (r) {
 			pthread_spin_lock(&ct->hldr_lock);
 			ct->holders--;
+			ct->thread = 0;
 			pthread_spin_unlock(&ct->hldr_lock);
 			pthread_mutex_unlock(&ct->lock);
-			ct->thread = 0;
 			condlog(3, "%d:%d: failed to start tur thread, using"
 				" sync mode", TUR_DEVT(ct));
 			return tur_check(c->fd, c->timeout, c->message);
-- 
2.9.2


[-- Attachment #5: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Dragan Stancevic]

[-- Attachment #1.1: Type: text/plain, Size: 997 bytes --]

Hi Bart-

it looks ok to me, these three patches combined with your previous patch
that is already in the repo...


Acked-by: dragan.stancevic@canonical.com

On Wed, Aug 17, 2016 at 2:57 PM, Bart Van Assche <bart.vanassche@sandisk.com
> wrote:

> On 08/17/2016 12:36 PM, Dragan Stancevic wrote:
>
>> Acked-by: dragan.stancevic@canonical.com
>> <mailto:dragan.stancevic@canonical.com>
>>
>> I agree with your patch, I have been tracking an issue where multipathd
>> dumps core on the exit path just past the treads being canceled. Your
>> patch is very similar to mine (minus nuking the depth) that I was going
>> to send out to a user to test with. The checker thread accesses a valid
>> pointer with garbage values....
>>
>
> Hello Dragan,
>
> When I prepared my patch I overlooked that multipathd creates most threads
> as detached threads. So I started testing the three attached patches on my
> setup. It would be appreciated if you could have a look at these patches.
>
> Thanks,
>
> Bart.
>

[-- Attachment #1.2: Type: text/html, Size: 1740 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Bart Van Assche]

On 08/18/2016 01:54 PM, Dragan Stancevic wrote:
> it looks ok to me, these three patches combined with your previous patch
> that is already in the repo...

Hello Dan,

Thanks for the feedback. I will send the first patch to Christophe but I 
would like to give the second and third patches a bit more thought.

Bart.

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Benjamin Marzinski]

On Tue, Aug 16, 2016 at 08:31:16AM +0200, Hannes Reinecke wrote:
> On 08/15/2016 05:28 PM, Bart Van Assche wrote:
> > Use pthread_join() to wait until worker threads have finished
> > instead of using a counter to keep track of how many threads are
> > trying to grab a mutex. Remove mutex_lock.depth since this member
> > variable is no longer needed.
> > 
> > This patch fixes two race conditions:
> > * Incrementing "depth" in lock() without holding a mutex.
> > * Destroying a mutex from the main thread without waiting for the
> >   worker threads to finish using that mutex.
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > ---
> >  libmultipath/lock.h | 15 +--------------
> >  multipathd/main.c   | 13 ++++++-------
> >  2 files changed, 7 insertions(+), 21 deletions(-)
> > 
> > diff --git a/libmultipath/lock.h b/libmultipath/lock.h
> > index 9808480..a170efe 100644
> > --- a/libmultipath/lock.h
> > +++ b/libmultipath/lock.h
> > @@ -3,35 +3,22 @@
> >  
> >  #include <pthread.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;
> >  };
> >  
> >  static inline void lock(struct mutex_lock *a)
> >  {
> > -	a->depth++;
> >  	pthread_mutex_lock(&a->mutex);
> >  }
> >  
> >  static inline int timedlock(struct mutex_lock *a, struct timespec *tmo)
> >  {
> > -	int r;
> > -
> > -	a->depth++;
> > -	r = pthread_mutex_timedlock(&a->mutex, tmo);
> > -	if (r)
> > -		a->depth--;
> > -	return r;
> > +	return pthread_mutex_timedlock(&a->mutex, tmo);
> >  }
> >  
> >  static inline void unlock(struct mutex_lock *a)
> >  {
> > -	a->depth--;
> >  	pthread_mutex_unlock(&a->mutex);
> >  }
> >  
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index fff482c..c288195 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2046,7 +2046,6 @@ init_vecs (void)
> >  		return NULL;
> >  
> >  	pthread_mutex_init(&vecs->lock.mutex, NULL);
> > -	vecs->lock.depth = 0;
> >  
> >  	return vecs;
> >  }
> > @@ -2394,16 +2393,16 @@ child (void * param)
> >  	pthread_cancel(uxlsnr_thr);
> >  	pthread_cancel(uevq_thr);
> >  
> > +	pthread_join(check_thr, NULL);
> > +	pthread_join(uevent_thr, NULL);
> > +	pthread_join(uxlsnr_thr, NULL);
> > +	pthread_join(uevq_thr, NULL);
> > +
> >  	lock(&vecs->lock);
> >  	free_pathvec(vecs->pathvec, FREE_PATHS);
> >  	vecs->pathvec = NULL;
> >  	unlock(&vecs->lock);
> > -	/* 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...", vecs->lock.depth);
> > -	}
> > +
> >  	pthread_mutex_destroy(&vecs->lock.mutex);
> >  	FREE(vecs);
> >  	vecs = NULL;
> > 
> 
> Makes one wonder: what happens to the waitevent threads?
> We won't be waiting for them after applying this patch, right?
> So why did we ever had this busy loop here?
> Ben?

Unless something waits for the threads to stop, mutipathd can easily
crash if a thread runs after we deallocate vecs.  At one point, I tried
solving this with non-detached threads, but I kept finding corner cases
where zombie threads could be created.  Probably the easiest way to
avoid this is to simply not free vecs, and not wait for the threads.

> 
> (And while we're at the subject: can't we drop the waitevent threads
> altogether? We're listening to uevents nowadays, so we should be
> notified if something happened to the device-mapper tables. Which should
> make the waitevent threads unnecessary, right?)

This is definitely a discussion worth having.  I would love to see the
waiter threads go. The only issue is that uevents aren't guaranteed to
be received. They were designed to be "best effort" notifications. The
dm events are guaranteed to be issued. This means that multipathd may
miss uevents. Now, we sync the multipath state with the kernel when we
check the path, but there are a number of steps from update_multipath
(which is what gets called by the waitevent threads) that we skip.  It
would take some looking into, but if we can determine that either these
steps aren't necessary (for instance, when we are already calling the
checker, there's no point in adjusting the time of the next check) or
that we can safely do them on every path_check, then at worst, missing a
uevent would delay this work until the next time we check the path.

There are really two things that multipath doesn't want to have to wait
on, failing over, and failing back.  I don't see how missing a uevent
could slow down failing over at all. It could slow down failing back in 
some cases. For instance, if a path just dropped for an very small time, the
kernel would failover, issue a dm event and uevent, and when multipathd
called update_multipath, it would lower the time to the next path check,
if it was too long. I'm not sure that this is that big of a deal,
however.

I should note that the kernel issues dm events when it switches
pathgroups, but not uevents.  Off the top of my head, I don't see
missing these events being a big deal (the code even says that there is
nothing to do here, although update_multipath gets called all the same).

Also, we would need to change some things about how multipath works to
rely on uevents.  For instance, multipathd would probably want to call
update_map() whenever it gets a change uevent. Also, we would need to
sync the old checker state with the kernel state after calling
update_strings in check_path. Otherwise, if we missed a uevent, the
daemon might never realize that a path went down, and needed multipathd
to issue a reinstate so that the kernel started using it again.

There may be other issues that I'm not thinking of right now, but so
far, I can't think of any reason why we couldn't remove the waitevent
threads. I would be greatful if other people gave this some thought to
see if they can think of some reason that I'm missing.

-Ben

> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		   Teamlead Storage & Networking
> hare@suse.de			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop

[Hannes Reinecke]

On 08/25/2016 05:33 AM, Benjamin Marzinski wrote:
> On Tue, Aug 16, 2016 at 08:31:16AM +0200, Hannes Reinecke wrote:
[ .. ]
>>
>> Makes one wonder: what happens to the waitevent threads?
>> We won't be waiting for them after applying this patch, right?
>> So why did we ever had this busy loop here?
>> Ben?
> 
> Unless something waits for the threads to stop, mutipathd can easily
> crash if a thread runs after we deallocate vecs.  At one point, I tried
> solving this with non-detached threads, but I kept finding corner cases
> where zombie threads could be created.  Probably the easiest way to
> avoid this is to simply not free vecs, and not wait for the threads.
> 
Ah, yes. True.

>>
>> (And while we're at the subject: can't we drop the waitevent threads
>> altogether? We're listening to uevents nowadays, so we should be
>> notified if something happened to the device-mapper tables. Which should
>> make the waitevent threads unnecessary, right?)
> 
> This is definitely a discussion worth having.  I would love to see the
> waiter threads go. The only issue is that uevents aren't guaranteed to
> be received. They were designed to be "best effort" notifications. The
> dm events are guaranteed to be issued. This means that multipathd may
> miss uevents. Now, we sync the multipath state with the kernel when we
> check the path, but there are a number of steps from update_multipath
> (which is what gets called by the waitevent threads) that we skip.  It
> would take some looking into, but if we can determine that either these
> steps aren't necessary (for instance, when we are already calling the
> checker, there's no point in adjusting the time of the next check) or
> that we can safely do them on every path_check, then at worst, missing a
> uevent would delay this work until the next time we check the path.
> 
> There are really two things that multipath doesn't want to have to wait
> on, failing over, and failing back.  I don't see how missing a uevent
> could slow down failing over at all. It could slow down failing back in 
> some cases. For instance, if a path just dropped for an very small time, the
> kernel would failover, issue a dm event and uevent, and when multipathd
> called update_multipath, it would lower the time to the next path check,
> if it was too long. I'm not sure that this is that big of a deal,
> however.
> 
Indeed, it would only affect failback.
Failover is handled mainly inside the kernel (redirecting I/O and
whatnot), so that doesn't really need multipath interaction at all.

And if failback is delayed that shouldn't matter, either, as it's hard
to predict when the failback will happen even nowadays.
The only worry we might have is that we're missing failback events
altogether, but the path checker should insulate us against this
eventuality.

> I should note that the kernel issues dm events when it switches
> pathgroups, but not uevents.  Off the top of my head, I don't see
> missing these events being a big deal (the code even says that there is
> nothing to do here, although update_multipath gets called all the same).
> 
> Also, we would need to change some things about how multipath works to
> rely on uevents.  For instance, multipathd would probably want to call
> update_map() whenever it gets a change uevent. Also, we would need to
> sync the old checker state with the kernel state after calling
> update_strings in check_path. Otherwise, if we missed a uevent, the
> daemon might never realize that a path went down, and needed multipathd
> to issue a reinstate so that the kernel started using it again.
> 
> There may be other issues that I'm not thinking of right now, but so
> far, I can't think of any reason why we couldn't remove the waitevent
> threads. I would be greatful if other people gave this some thought to
> see if they can think of some reason that I'm missing.
> 
I don't, and in fact I've been wondering about the reason why we're
keeping waitevents with us for so long :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)