From 41f7e1631034e232cb936a53f04fa29d1e31f484 Mon Sep 17 00:00:00 2001 From: N Balachandran Date: Mon, 29 May 2017 15:21:39 +0530 Subject: [PATCH 482/486] 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 > BUG: 1456385 > Signed-off-by: N Balachandran > Reviewed-on: https://review.gluster.org/17410 > Reviewed-by: Raghavendra G > Tested-by: Raghavendra G > Smoke: Gluster Build System > NetBSD-regression: NetBSD Build System > CentOS-regression: Gluster Build System > Reviewed-by: Jeff Darcy Change-Id: I54b064857e2694826d0c03b23f8014e3984a3330 BUG: 1435357 Signed-off-by: N Balachandran Reviewed-on: https://code.engineering.redhat.com/gerrit/107730 Reviewed-by: Atin Mukherjee --- xlators/performance/io-cache/src/ioc-inode.c | 78 ++++++++++++++-------------- 1 file 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 cee3bad..6eb3412 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; -- 1.8.3.1