multipath: fix scsi async tur checker corruption

Since the tur checker runs asynchronously in its own thread, there is nothing
that keeps a path from being orphaned or deleted before the tur thread has
finished. When this happenes the checker struct gets deleted.  However, the tur
thread might still we writing to that memory.  This can lead to memory
corruption.  This patch adds all of the necessary data to the checker context,
and makes the tur thread only use that. This way, if the checker is deleted
while the thread is still using the context, the thread will clean up the
context itself.

Since the context can only be freed when both the thread and the paths checker
structure have stopped needing it, and these can get get finished with the
context asychronously with respect to each other, the context has a holders
counter, protected by a spinlock, to keep track of the users.  When the
counter drops to zero, the context gets freed.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 224cf76b628660b2b0b5a7b684b1109b11c7b149..9c141aac03577065493c06fa359eca1090a6aa44 100644 (file)
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -35,10 +35,15 @@ struct tur_checker_context {
        dev_t devt;
        int state;
        int running;
-       time_t timeout;
+       int fd;
+       unsigned int timeout;
+       time_t time;
        pthread_t thread;
        pthread_mutex_t lock;
        pthread_cond_t active;
+       pthread_spinlock_t hldr_lock;
+       int holders;
+       char message[CHECKER_MSG_LEN];
 };
 
 #define TUR_DEVT(c) major((c)->devt), minor((c)->devt)
@@ -53,28 +58,49 @@ int libcheck_init (struct checker * c)
        memset(ct, 0, sizeof(struct tur_checker_context));
 
        ct->state = PATH_UNCHECKED;
+       ct->fd = -1;
+       ct->holders = 1;
        pthread_cond_init(&ct->active, NULL);
        pthread_mutex_init(&ct->lock, NULL);
+       pthread_spin_init(&ct->hldr_lock, PTHREAD_PROCESS_PRIVATE);
        c->context = ct;
 
        return 0;
 }
 
+void cleanup_context(struct tur_checker_context *ct)
+{
+       pthread_mutex_destroy(&ct->lock);
+       pthread_cond_destroy(&ct->active);
+       pthread_spin_destroy(&ct->hldr_lock);
+       free(ct);
+}
+
 void libcheck_free (struct checker * c)
 {
        if (c->context) {
                struct tur_checker_context *ct = c->context;
-
-               pthread_mutex_destroy(&ct->lock);
-               pthread_cond_destroy(&ct->active);
-               free(c->context);
+               int holders;
+               pthread_t thread;
+
+               pthread_spin_lock(&ct->hldr_lock);
+               ct->holders--;
+               holders = ct->holders;
+               thread = ct->thread;
+               pthread_spin_unlock(&ct->hldr_lock);
+               if (holders)
+                       pthread_cancel(thread);
+               else
+                       cleanup_context(ct);
                c->context = NULL;
        }
        return;
 }
 
+#define TUR_MSG(msg, fmt, args...) snprintf(msg, CHECKER_MSG_LEN, fmt, ##args);
+
 int
-tur_check (struct checker * c)
+tur_check(int fd, unsigned int timeout, char *msg)
 {
        struct sg_io_hdr io_hdr;
        unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
@@ -90,10 +116,10 @@ tur_check (struct checker * c)
        io_hdr.dxfer_direction = SG_DXFER_NONE;
        io_hdr.cmdp = turCmdBlk;
        io_hdr.sbp = sense_buffer;
-       io_hdr.timeout = c->timeout;
+       io_hdr.timeout = timeout;
        io_hdr.pack_id = 0;
-       if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
-               MSG(c, MSG_TUR_DOWN);
+       if (ioctl(fd, SG_IO, &io_hdr) < 0) {
+               TUR_MSG(msg, MSG_TUR_DOWN);
                return PATH_DOWN;
        }
        if ((io_hdr.status & 0x7e) == 0x18) {
@@ -101,7 +127,7 @@ tur_check (struct checker * c)
                 * SCSI-3 arrays might return
                 * reservation conflict on TUR
                 */
-               MSG(c, MSG_TUR_UP);
+               TUR_MSG(msg, MSG_TUR_UP);
                return PATH_UP;
        }
        if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -146,14 +172,14 @@ tur_check (struct checker * c)
                                 * LOGICAL UNIT NOT ACCESSIBLE,
                                 * TARGET PORT IN STANDBY STATE
                                 */
-                               MSG(c, MSG_TUR_GHOST);
+                               TUR_MSG(msg, MSG_TUR_GHOST);
                                return PATH_GHOST;
                        }
                }
-               MSG(c, MSG_TUR_DOWN);
+               TUR_MSG(msg, MSG_TUR_DOWN);
                return PATH_DOWN;
        }
-       MSG(c, MSG_TUR_UP);
+       TUR_MSG(msg, MSG_TUR_UP);
        return PATH_UP;
 }
 
@@ -162,18 +188,25 @@ tur_check (struct checker * c)
 
 void cleanup_func(void *data)
 {
+       int holders;
        struct tur_checker_context *ct = data;
+       pthread_spin_lock(&ct->hldr_lock);
+       ct->holders--;
+       holders = ct->holders;
        ct->thread = 0;
+       pthread_spin_unlock(&ct->hldr_lock);
+       if (!holders)
+               cleanup_context(ct);
 }
 
 void *tur_thread(void *ctx)
 {
-       struct checker *c = ctx;
-       struct tur_checker_context *ct = c->context;
+       struct tur_checker_context *ct = ctx;
        int state;
 
        condlog(3, "%d:%d: tur checker starting up", TUR_DEVT(ct));
 
+       ct->message[0] = '\0';
        /* This thread can be canceled, so setup clean up */
        tur_thread_cleanup_push(ct)
 
@@ -182,7 +215,7 @@ void *tur_thread(void *ctx)
        ct->state = PATH_PENDING;
        pthread_mutex_unlock(&ct->lock);
 
-       state = tur_check(c);
+       state = tur_check(ct->fd, ct->timeout, ct->message);
 
        /* TUR checker done */
        pthread_mutex_lock(&ct->lock);
@@ -213,7 +246,7 @@ void tur_set_async_timeout(struct checker *c)
        struct timeval now;
 
        gettimeofday(&now, NULL);
-       ct->timeout = now.tv_sec + c->timeout;
+       ct->time = now.tv_sec + c->timeout;
 }
 
 int tur_check_async_timeout(struct checker *c)
@@ -222,7 +255,7 @@ int tur_check_async_timeout(struct checker *c)
        struct timeval now;
 
        gettimeofday(&now, NULL);
-       return (now.tv_sec > ct->timeout);
+       return (now.tv_sec > ct->time);
 }
 
 extern int
@@ -242,7 +275,7 @@ libcheck_check (struct checker * c)
                ct->devt = sb.st_rdev;
 
        if (c->sync)
-               return tur_check(c);
+               return tur_check(c->fd, c->timeout, c->message);
 
        /*
         * Async mode
@@ -276,6 +309,8 @@ libcheck_check (struct checker * c)
                        /* TUR checker done */
                        ct->running = 0;
                        tur_status = ct->state;
+                       strncpy(c->message, ct->message, CHECKER_MSG_LEN);
+                       c->message[CHECKER_MSG_LEN - 1] = '\0';
                }
                pthread_mutex_unlock(&ct->lock);
        } else {
@@ -284,24 +319,32 @@ libcheck_check (struct checker * c)
                        pthread_mutex_unlock(&ct->lock);
                        condlog(3, "%d:%d: tur thread not responding, "
                                "using sync mode", TUR_DEVT(ct));
-                       return tur_check(c);
+                       return tur_check(c->fd, c->timeout, c->message);
                }
                /* Start new TUR checker */
                ct->state = PATH_UNCHECKED;
+               ct->fd = c->fd;
+               ct->timeout = c->timeout;
+               pthread_spin_lock(&ct->hldr_lock);
+               ct->holders++;
+               pthread_spin_unlock(&ct->hldr_lock);
                tur_set_async_timeout(c);
                setup_thread_attr(&attr, 32 * 1024, 1);
-               r = pthread_create(&ct->thread, &attr, tur_thread, c);
+               r = pthread_create(&ct->thread, &attr, tur_thread, ct);
                if (r) {
                        pthread_mutex_unlock(&ct->lock);
                        ct->thread = 0;
+                       ct->holders--;
                        condlog(3, "%d:%d: failed to start tur thread, using"
                                " sync mode", TUR_DEVT(ct));
-                       return tur_check(c);
+                       return tur_check(c->fd, c->timeout, c->message);
                }
                pthread_attr_destroy(&attr);
                tur_timeout(&tsp);
                r = pthread_cond_timedwait(&ct->active, &ct->lock, &tsp);
                tur_status = ct->state;
+               strncpy(c->message, ct->message,CHECKER_MSG_LEN);
+               c->message[CHECKER_MSG_LEN -1] = '\0';
                pthread_mutex_unlock(&ct->lock);
                if (ct->thread &&
                    (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 7adda2aa84ad2a74006abcfed5f251878bfca126..c4087215046c32bdbb022c878bf3baea691a4b85 100644 (file)
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -747,6 +747,7 @@ get_state (struct path * pp, int daemon)
                }
                checker_set_fd(c, pp->fd);
                if (checker_init(c, pp->mpp?&pp->mpp->mpcontext:NULL)) {
+                       memset(c, 0x0, sizeof(struct checker));
                        condlog(3, "%s: checker init failed", pp->dev);
                        return PATH_UNCHECKED;
                }