From 41f7e1631034e232cb936a53f04fa29d1e31f484 Mon Sep 17 00:00:00 2001
From: N Balachandran <nbalacha@redhat.com>
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 <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>
Change-Id: I54b064857e2694826d0c03b23f8014e3984a3330
BUG: 1435357
Signed-off-by: N Balachandran <nbalacha@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/107730
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
---
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