perf/ioc: Fix race causing crash when accessing freed page

ioc_inode_wakeup does not lock the ioc_inode for the duration
of the operation, leaving a window where ioc_prune could find
a NULL waitq and hence free the page which ioc_inode_wakeup later
tries to access.

Thanks to Mohit for the analysis.

credit: moagrawa@redhat.com

Change-Id: I54b064857e2694826d0c03b23f8014e3984a3330
BUG: 1456385
Signed-off-by: N Balachandran <nbalacha@redhat.com>
Reviewed-on: https://review.gluster.org/17410
Reviewed-by: Raghavendra G <rgowdapp@redhat.com>
Tested-by: Raghavendra G <rgowdapp@redhat.com>
Smoke: Gluster Build System <jenkins@build.gluster.org>
NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
Reviewed-by: Jeff Darcy <jeff@pl.atyp.us>

1 files changed, 40 insertions, 38 deletions

diff --git a/xlators/performance/io-cache/src/ioc-inode.c b/xlators/performance/io-cache/src/ioc-inode.c
index cee3bad8c22..6eb34124d1f 100644
--- a/xlators/performance/io-cache/src/ioc-inode.c
+++ b/xlators/performance/io-cache/src/ioc-inode.c
@@ -83,47 +83,45 @@ ioc_inode_wakeup (call_frame_t *frame, ioc_inode_t *ioc_inode,
                 goto out;
         }
 
-        ioc_inode_lock (ioc_inode);
-        {
-                waiter = ioc_inode->waitq;
-                ioc_inode->waitq = NULL;
-        }
-        ioc_inode_unlock (ioc_inode);
-
         if (stbuf)
                 cache_still_valid = ioc_cache_still_valid (ioc_inode, stbuf);
         else
                 cache_still_valid = 0;
 
-        if (!waiter) {
-                gf_msg (frame->this->name, GF_LOG_WARNING, 0,
-                        IO_CACHE_MSG_PAGE_WAIT_VALIDATE,
-                        "cache validate called without any "
-                        "page waiting to be validated");
-        }
+        ioc_inode_lock (ioc_inode);
+        {
+
+                waiter = ioc_inode->waitq;
+                if (!waiter) {
+                        gf_msg (frame->this->name, GF_LOG_WARNING, 0,
+                                IO_CACHE_MSG_PAGE_WAIT_VALIDATE,
+                                "cache validate called without any "
+                                "page waiting to be validated");
+
+                        ioc_inode_unlock (ioc_inode);
+                        goto out;
+                }
 
-        while (waiter) {
-                waiter_page = waiter->data;
-                page_waitq = NULL;
+                while (waiter) {
+                        waiter_page = waiter->data;
+                        ioc_inode->waitq = waiter->next;
+                        page_waitq = NULL;
 
-                if (waiter_page) {
-                        if (cache_still_valid) {
-                                /* cache valid, wake up page */
-                                ioc_inode_lock (ioc_inode);
-                                {
+                        if (waiter_page) {
+                                if (cache_still_valid) {
+                                        /* cache valid, wake up page */
                                         page_waitq =
                                                 __ioc_page_wakeup (waiter_page,
-                                                                   waiter_page->op_errno);
-                                }
-                                ioc_inode_unlock (ioc_inode);
-                                if (page_waitq)
-                                        ioc_waitq_return (page_waitq);
-                        } else {
+                                                         waiter_page->op_errno);
+                                       if (page_waitq) {
+                                                ioc_inode_unlock (ioc_inode);
+                                                ioc_waitq_return (page_waitq);
+                                                ioc_inode_lock (ioc_inode);
+                                        }
+                                } else {
                                 /* cache invalid, generate page fault and set
                                  * page->ready = 0, to avoid double faults
                                  */
-                                ioc_inode_lock (ioc_inode);
-                                {
                                         if (waiter_page->ready) {
                                                 waiter_page->ready = 0;
                                                 need_fault = 1;
@@ -138,24 +136,28 @@ ioc_inode_wakeup (call_frame_t *frame, ioc_inode_t *ioc_inode,
                                                               frame,
                                                               waiter_page);
                                         }
-                                }
-                                ioc_inode_unlock (ioc_inode);
 
-                                if (need_fault) {
-                                        need_fault = 0;
-                                        ioc_page_fault (ioc_inode, frame,
-                                                        local->fd,
-                                                        waiter_page->offset);
+
+                                        if (need_fault) {
+                                                need_fault = 0;
+                                                ioc_inode_unlock (ioc_inode);
+                                                ioc_page_fault (ioc_inode,
+                                                                frame,
+                                                                local->fd,
+                                                                waiter_page->offset);
+                                                ioc_inode_lock (ioc_inode);
+                                        }
                                 }
                         }
-                }
 
                 waited = waiter;
-                waiter = waiter->next;
+                waiter = ioc_inode->waitq;
 
                 waited->data = NULL;
                 GF_FREE (waited);
+                }
         }
+        ioc_inode_unlock (ioc_inode);
 
 out:
         return;