From 4265b6bb74aebe4723a6748d29fd5d728f1ea169 Mon Sep 17 00:00:00 2001 From: Pranith Kumar K Date: Wed, 28 Jun 2017 09:10:53 +0530 Subject: [PATCH 534/539] features/shard: Remove ctx from LRU in shard_forget Problem: There is a race when the following two commands are executed on the mount in parallel from two different terminals on a sharded volume, which leads to use-after-free. Terminal-1: while true; do dd if=/dev/zero of=file1 bs=1M count=4; done Terminal-2: while true; do cat file1 > /dev/null; done In the normal case this is the life-cycle of a shard-inode 1) Shard is added to LRU when it is first looked-up 2) For every operation on the shard it is moved up in LRU 3) When "unlink of the shard"/"LRU limit is hit" happens it is removed from LRU But we are seeing a race where the inode stays in Shard LRU even after it is forgotten which leads to Use-after-free and then some memory-corruptions. These are the steps: 1) Shard is added to LRU when it is first looked-up 2) For every operation on the shard it is moved up in LRU Reader-handler Truncate-handler 1) Reader handler needs shard-x to be read. 1) Truncate has just deleted shard-x 2) In shard_common_resolve_shards(), it does inode_resolve() and that leads to a hit in LRU, so it is going to call __shard_update_shards_inode_list() to move the inode to top of LRU 2) shard-x gets unlinked from the itable and inode_forget(inode, 0) is called to make sure the inode can be purged upon last unref 3) when __shard_update_shards_inode_list() is called it finds that the inode is not in LRU so it adds it back to the LRU-list Both these operations complete and call inode_unref(shard-x) which leads to the inode getting freed and forgotten, even when it is in Shard LRU list. When more inodes are added to LRU, use-after-free will happen and it leads to undefined behaviors. Fix: I see that the inode can be removed from LRU even by the protocol layers like gfapi/gNFS when LRU limit is reached. So it is better to add a check in shard_forget() to remove itself from LRU list if it exists. >BUG: 1466037 >Change-Id: Ia79c0c5c9d5febc56c41ddb12b5daf03e5281638 >Signed-off-by: Pranith Kumar K >Reviewed-on: https://review.gluster.org/17644 >Smoke: Gluster Build System >CentOS-regression: Gluster Build System >Reviewed-by: Krutika Dhananjay BUG: 1464453 Change-Id: Ia79c0c5c9d5febc56c41ddb12b5daf03e5281638 Signed-off-by: Pranith Kumar K Reviewed-on: https://code.engineering.redhat.com/gerrit/110565 --- tests/bugs/shard/parallel-truncate-read.t | 48 +++++++++++++++++++++++++++++++ xlators/features/shard/src/shard.c | 13 +++++++++ 2 files changed, 61 insertions(+) create mode 100644 tests/bugs/shard/parallel-truncate-read.t diff --git a/tests/bugs/shard/parallel-truncate-read.t b/tests/bugs/shard/parallel-truncate-read.t new file mode 100644 index 0000000..4de876f --- /dev/null +++ b/tests/bugs/shard/parallel-truncate-read.t @@ -0,0 +1,48 @@ +#!/bin/bash + +#This test will crash if shard's LRU contains a shard's inode even after the +#inode is forgotten. Minimum time for crash to happen I saw was 180 seconds + +. $(dirname $0)/../../include.rc + +function keep_writing { + cd $M0; + while [ -f /tmp/parallel-truncate-read ] + do + dd if=/dev/zero of=file1 bs=1M count=16 + done + cd +} + +function keep_reading { + cd $M0; + while [ -f /tmp/parallel-truncate-read ] + do + cat file1 > /dev/null + done + cd +} + +cleanup; + +TEST touch /tmp/parallel-truncate-read +TEST glusterd +TEST pidof glusterd +TEST $CLI volume create $V0 replica 3 $H0:$B0/${V0}{0,1,2} +TEST $CLI volume set $V0 features.shard on +TEST $CLI volume set $V0 performance.quick-read off +TEST $CLI volume set $V0 performance.io-cache off +TEST $CLI volume set $V0 performance.stat-prefetch off +TEST $CLI volume set $V0 performance.read-ahead off +TEST $CLI volume start $V0 + +TEST $GFS --volfile-id=$V0 --volfile-server=$H0 $M0 +keep_writing & +keep_reading & +sleep 180 +TEST rm -f /tmp/parallel-truncate-read +wait +#test that the mount is operational +TEST stat $M0 + +cleanup; diff --git a/xlators/features/shard/src/shard.c b/xlators/features/shard/src/shard.c index 141c42f..b97608f 100644 --- a/xlators/features/shard/src/shard.c +++ b/xlators/features/shard/src/shard.c @@ -5018,13 +5018,26 @@ shard_forget (xlator_t *this, inode_t *inode) { uint64_t ctx_uint = 0; shard_inode_ctx_t *ctx = NULL; + shard_priv_t *priv = NULL; + priv = this->private; inode_ctx_del (inode, this, &ctx_uint); if (!ctx_uint) return 0; ctx = (shard_inode_ctx_t *)ctx_uint; + /* When LRU limit reaches inode will be forcefully removed from the + * table, inode needs to be removed from LRU of shard as well. + */ + if (!list_empty (&ctx->ilist)) { + LOCK(&priv->lock); + { + list_del_init (&ctx->ilist); + priv->inode_count--; + } + UNLOCK(&priv->lock); + } GF_FREE (ctx); return 0; -- 1.8.3.1