From a261e1bafeb95aab9fb883568e67079a07dccf4f Mon Sep 17 00:00:00 2001 From: N Balachandran Date: Mon, 29 May 2017 15:21:39 +0530 Subject: 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: 1457054 Signed-off-by: N Balachandran Reviewed-on: https://review.gluster.org/17423 Smoke: Gluster Build System NetBSD-regression: NetBSD Build System Reviewed-by: Raghavendra Talur CentOS-regression: Gluster Build System --- xlators/performance/io-cache/src/ioc-inode.c | 78 ++++++++++++++-------------- 1 file changed, 40 insertions(+), 38 deletions(-) (limited to 'xlators') 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; -- cgit