Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: "saranyamohan via sendgmr" 
        <saranyamohan@saranyamohan.svl.corp.google.com>
Date:   Wed, 18 Nov 2020 07:39:40 -0800
In-Reply-To: <20201118153947.3394530-1-saranyamohan@google.com>
Message-Id: <20201118153947.3394530-55-saranyamohan@google.com>
Mime-Version: 1.0
References: <20201118153947.3394530-1-saranyamohan@google.com>
Subject: [RFC PATCH v3 54/61] e2fsck: fix race in ext2fs_read_bitmaps()
From:   Saranya Muruganandam <saranyamohan@google.com>
To:     linux-ext4@vger.kernel.org, tytso@mit.edu
Cc:     adilger.kernel@dilger.ca, Wang Shilong <wshilong@ddn.com>,
        Saranya Muruganandam <saranyamohan@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

From: Wang Shilong <wshilong@ddn.com>

During corruption testing hiting following segfault:

Multiple threads triggered to read bitmaps
Signal (11) SIGSEGV si_code=SEGV_MAPERR fault addr=0x200
./e2fsck[0x4382ae]
/lib64/libpthread.so.0(+0x14b20)[0x7f5854d2fb20]
./e2fsck(ext2fs_rb_insert_color+0xc)[0x46ac0c]
./e2fsck[0x467bb4]
./e2fsck[0x467e6d]
./e2fsck[0x45ba95]
./e2fsck[0x45c124]
/lib64/libpthread.so.0(+0x94e2)[0x7f5854d244e2]
/lib64/libc.so.6(clone+0x43)[0x7f5854beb6c3]

Problem is @block_map might be set NULL if one of
thread exit, move such kind of cleanup operation
to main thread after all threads exit.

Another potential problem is e2fsck_read_bitmap()
could be called during pass1, this need be serialized,
serialize it in the pass1.

Signed-off-by: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Saranya Muruganandam <saranyamohan@google.com>
---
 e2fsck/pass1.c          |  2 +-
 lib/ext2fs/rw_bitmaps.c | 25 ++++++++-----------------
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index e98cda9f..3899d710 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -4183,9 +4183,9 @@ report_problem:
 					}
 					continue;
 				}
+				e2fsck_pass1_fix_lock(ctx);
 				e2fsck_read_bitmaps(ctx);
 				pb->inode_modified = 1;
-				e2fsck_pass1_fix_lock(ctx);
 				pctx->errcode =
 					ext2fs_extent_delete(ehandle, 0);
 				e2fsck_pass1_fix_unlock(ctx);
diff --git a/lib/ext2fs/rw_bitmaps.c b/lib/ext2fs/rw_bitmaps.c
index 95de9b1c..eb791202 100644
--- a/lib/ext2fs/rw_bitmaps.c
+++ b/lib/ext2fs/rw_bitmaps.c
@@ -445,14 +445,6 @@ success_cleanup:
 	return 0;
 
 cleanup:
-	if (do_block) {
-		ext2fs_free_block_bitmap(fs->block_map);
-		fs->block_map = 0;
-	}
-	if (do_inode) {
-		ext2fs_free_inode_bitmap(fs->inode_map);
-		fs->inode_map = 0;
-	}
 	if (inode_bitmap)
 		ext2fs_free_mem(&inode_bitmap);
 	if (block_bitmap)
@@ -463,9 +455,12 @@ cleanup:
 
 }
 
-static errcode_t read_bitmaps_range_end(ext2_filsys fs, int do_inode, int do_block)
+static errcode_t read_bitmaps_range_end(ext2_filsys fs, int do_inode, int do_block,
+					errcode_t retval)
 {
-	errcode_t retval = 0;
+
+	if (retval)
+		goto cleanup;
 
 	/* Mark group blocks for any BLOCK_UNINIT groups */
 	if (do_block) {
@@ -474,7 +469,7 @@ static errcode_t read_bitmaps_range_end(ext2_filsys fs, int do_inode, int do_blo
 			goto cleanup;
 	}
 
-	return retval;
+	return 0;
 cleanup:
 	if (do_block) {
 		ext2fs_free_block_bitmap(fs->block_map);
@@ -497,10 +492,8 @@ static errcode_t read_bitmaps_range(ext2_filsys fs, int do_inode, int do_block,
 		return retval;
 
 	retval = read_bitmaps_range_start(fs, do_inode, do_block, start, end, NULL, NULL);
-	if (retval)
-		return retval;
 
-	return read_bitmaps_range_end(fs, do_inode, do_block);
+	return read_bitmaps_range_end(fs, do_inode, do_block, retval);
 }
 
 #ifdef CONFIG_PFSCK
@@ -636,9 +629,7 @@ out:
 	free(thread_infos);
 	free(thread_ids);
 
-	if (!retval)
-		retval = read_bitmaps_range_end(fs, do_inode, do_block);
-
+	retval = read_bitmaps_range_end(fs, do_inode, do_block, retval);
 	if (!retval) {
 		if (do_inode)
 			fs->flags &= ~EXT2_FLAG_IBITMAP_TAIL_PROBLEM;
-- 
2.29.2.299.gdc1121823c-goog