Bug 639429

Description of problem:
The patch for bug 459629 seems to introduce a race:

revision 1.69.2.17 date: 2009/04/03 15:09:03; author: bmarzins; state: Exp; lines: +11 -2 Fix for bz #459629. Multipathd needs to wait for the waiter threads to finish before freeing the vecs locks. This bug has already been fixed upstream. 

This was a fix to prevent a hang in multipathd when shutting down (and some related problems):

https://bugzilla.redhat.com/show_bug.cgi?id=459629#c34

The patch adds code to NULL out vecs->pathvec and synchronize with the waiter threads before destroying vecs->lock:

+ vecs->pathvec=NULL; 
[...]
+ ret = 0; 
+ while (num_waiters > 0 && ret == 0)
+         ret = pthread_cond_wait(&waiter_cond, vecs->lock);
+ if (ret) {
+         condlog(0, "error while waiting for event threads: %s", + strerror(errno));
+ exit(1);
+ } 

This gives us:

1679         /*
1680          * exit path
1681          */
1682         block_signal(SIGHUP, NULL);
1683         lock(vecs->lock);
1684         if (conf->queue_without_daemon == QUE_NO_DAEMON_OFF)
1685                 vector_foreach_slot(vecs->mpvec, mpp, i)
1686                         dm_queue_if_no_path(mpp->alias, 0);
1687         remove_maps(vecs, stop_waiter_thread);
1688         free_pathvec(vecs->pathvec, FREE_PATHS);
1689         vecs->pathvec=NULL;
1690 
1691         pthread_cancel(check_thr);
1692         pthread_cancel(uevent_thr);
1693         pthread_cancel(uxlsnr_thr);
1694 
1695         free_keys(keys);
1696         keys = NULL;
1697         free_handlers(handlers);
1698         handlers = NULL;
1699         free_polls();
1700 
1701         ret = 0;
1702         while (num_waiters > 0 && ret == 0)
1703                 ret = pthread_cond_wait(&waiter_cond, vecs->lock);
1704         if (ret) {
1705                 condlog(0, "error while waiting for event threads: %s",
1706                         strerror(errno));
1707                 exit(1);
1708         }
1709         unlock(vecs->lock);
1710         pthread_mutex_destroy(vecs->lock);
1711         FREE(vecs->lock);
1712         vecs->lock = NULL;
1713         FREE(vecs);
1714         vecs = NULL;

But since pthread_cond_wait will automatically unlock vecs->lock when putting the calling thread to sleep this introduces a race condition with the thread running the checkerloop routine:

 964 checkerloop (void *ap)
 965 {
[...]
 985         while (1) {
 986                 block_signal(SIGHUP, &old);
 987                 pthread_cleanup_push(cleanup_lock, vecs->lock);
 988                 lock(vecs->lock);
 989                 condlog(4, "tick");
 990 
 991                 vector_foreach_slot (vecs->pathvec, pp, i) {
 992                         if (!pp->mpp)
[...]
1160         }
1161         return NULL;
1162 }

Although the checker thread has been sent a cancellation request at this point multipathd's pthreads are created with the default deferred cancellation type. This means that the thread will continue to run until it calls a function that is a defined cancellation point.

Since none of the functions called between lock(vecs->lock) and the vector_foreach_slot() is a cancellation point (and the pthread_mutex_lock in lock() is not itself a cancellation point) the checker thread is still running and depending on scheduler timing it is possible for us to acquire the lock and attempt to access vecs->pathvec before the process terminates.

These two behaviours combined with the changes for bug 459629 lead to a situation in which:

- a cancellation request has been sent to check_thr
- the cancellation request has NOT been acted upon
- the main thread has released vecs->lock and is blocked in pthread_cond_wait
- vecs->pathvec has been de-allocated and set to NULL 

Version-Release number of selected component (if applicable):
device-mapper-multipath-0.4.7-34.el5_5.4

How reproducible:
Difficult. The user who provided the core file rebooted the system 1230 times before seeing the problem.

Steps to Reproduce:
1. Repeatedly reboot a system running multipathd
 
Actual results:
Core was generated by `/sbin/multipathd'.
Program terminated with signal 11, Segmentation fault.
#0  0x000000000040628a in checkerloop (ap=0x1b412030) at main.c:953
953			vector_foreach_slot (vecs->pathvec, pp, i) {
(gdb) bt
#0  0x000000000040628a in checkerloop (ap=0x1b412030) at main.c:953
#1  0x0000003b45e06367 in start_thread (arg=<value optimized out>) at pthread_create.c:297
#2  0x0000003b452d30ad in clone () from /lib64/libc.so.6

Expected results:
No segfault during shutdown.

Additional info:
There are a number of ways that we could address this:
- convert threads to use asynchronous cancellation type (causes threads to be cancellable at any time, usually resulting in immediate cancellation but this is not guaranteed)
- insert a call to a function that is required to be a cancellation point between acquisition of the vecs->lock mutex and access to vecs->pathvec
- modify the checker loop to test vecs->pathvec and exit if the field is NULL

I'm leaning toward the 2nd option; the 1st may not close the race completely and the 3rd is a bit blunt.