ext4: fix race in buffer_head read fault injection

Long Li · tytso · commit 2f3d93e210b9 · 2024-11-12T23:54:14.000-05:00
When I enabled ext4 debug for fault injection testing, I encountered the following warning: EXT4-fs error (device sda): ext4_read_inode_bitmap:201: comm fsstress: Cannot read inode bitmap - block_group = 8, inode_bitmap = 1051 WARNING: CPU: 0 PID: 511 at fs/buffer.c:1181 mark_buffer_dirty+0x1b3/0x1d0 The root cause of the issue lies in the improper implementation of ext4's buffer_head read fault injection. The actual completion of buffer_head read and the buffer_head fault injection are not atomic, which can lead to the uptodate flag being cleared on normally used buffer_heads in race conditions. [CPU0] [CPU1] [CPU2] ext4_read_inode_bitmap ext4_read_bh() <bh read complete> ext4_read_inode_bitmap if (buffer_uptodate(bh)) return bh jbd2_journal_commit_transaction __jbd2_journal_refile_buffer __jbd2_journal_unfile_buffer __jbd2_journal_temp_unlink_buffer ext4_simulate_fail_bh() clear_buffer_uptodate mark_buffer_dirty <report warning> WARN_ON_ONCE(!buffer_uptodate(bh)) The best approach would be to perform fault injection in the IO completion callback function, rather than after IO completion. However, the IO completion callback function cannot get the fault injection code in sb. Fix it by passing the result of fault injection into the bh read function, we simulate faults within the bh read function itself. This requires adding an extra parameter to the bh read functions that need fault injection. Fixes: 46f870d ("ext4: simulate various I/O and checksum errors when reading metadata") Signed-off-by: Long Li <leo.lilong@huawei.com> Link: https://patch.msgid.link/20240906091746.510163-1-leo.lilong@huawei.com Signed-off-by: Theodore Ts'o <tytso@mit.edu>
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
@@ -550,7 +550,8 @@ ext4_read_block_bitmap_nowait(struct super_block *sb, ext4_group_t block_group,
 	trace_ext4_read_block_bitmap_load(sb, block_group, ignore_locked);
 	ext4_read_bh_nowait(bh, REQ_META | REQ_PRIO |
 			    (ignore_locked ? REQ_RAHEAD : 0),
-			    ext4_end_bitmap_read);
+			    ext4_end_bitmap_read,
+			    ext4_simulate_fail(sb, EXT4_SIM_BBITMAP_EIO));
 	return bh;
 verify:
 	err = ext4_validate_block_bitmap(sb, desc, block_group, bh);
@@ -577,7 +578,6 @@ int ext4_wait_block_bitmap(struct super_block *sb, ext4_group_t block_group,
 	if (!desc)
 		return -EFSCORRUPTED;
 	wait_on_buffer(bh);
-	ext4_simulate_fail_bh(sb, bh, EXT4_SIM_BBITMAP_EIO);
 	if (!buffer_uptodate(bh)) {
 		ext4_error_err(sb, EIO, "Cannot read block bitmap - "
 			       "block_group = %u, block_bitmap = %llu",
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
@@ -1865,14 +1865,6 @@ static inline bool ext4_simulate_fail(struct super_block *sb,
 	return false;
 }
 
-static inline void ext4_simulate_fail_bh(struct super_block *sb,
-					 struct buffer_head *bh,
-					 unsigned long code)
-{
-	if (!IS_ERR(bh) && ext4_simulate_fail(sb, code))
-		clear_buffer_uptodate(bh);
-}
-
 /*
  * Error number codes for s_{first,last}_error_errno
  *
@@ -3100,9 +3092,9 @@ extern struct buffer_head *ext4_sb_bread(struct super_block *sb,
 extern struct buffer_head *ext4_sb_bread_unmovable(struct super_block *sb,
 						   sector_t block);
 extern void ext4_read_bh_nowait(struct buffer_head *bh, blk_opf_t op_flags,
-				bh_end_io_t *end_io);
+				bh_end_io_t *end_io, bool simu_fail);
 extern int ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,
-			bh_end_io_t *end_io);
+			bh_end_io_t *end_io, bool simu_fail);
 extern int ext4_read_bh_lock(struct buffer_head *bh, blk_opf_t op_flags, bool wait);
 extern void ext4_sb_breadahead_unmovable(struct super_block *sb, sector_t block);
 extern int ext4_seq_options_show(struct seq_file *seq, void *offset);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
@@ -568,7 +568,7 @@ __read_extent_tree_block(const char *function, unsigned int line,
 
 	if (!bh_uptodate_or_lock(bh)) {
 		trace_ext4_ext_load_extent(inode, pblk, _RET_IP_);
-		err = ext4_read_bh(bh, 0, NULL);
+		err = ext4_read_bh(bh, 0, NULL, false);
 		if (err < 0)
 			goto errout;
 	}
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
@@ -193,8 +193,9 @@ ext4_read_inode_bitmap(struct super_block *sb, ext4_group_t block_group)
 	 * submit the buffer_head for reading
 	 */
 	trace_ext4_load_inode_bitmap(sb, block_group);
-	ext4_read_bh(bh, REQ_META | REQ_PRIO, ext4_end_bitmap_read);
-	ext4_simulate_fail_bh(sb, bh, EXT4_SIM_IBITMAP_EIO);
+	ext4_read_bh(bh, REQ_META | REQ_PRIO,
+		     ext4_end_bitmap_read,
+		     ext4_simulate_fail(sb, EXT4_SIM_IBITMAP_EIO));
 	if (!buffer_uptodate(bh)) {
 		put_bh(bh);
 		ext4_error_err(sb, EIO, "Cannot read inode bitmap - "
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
@@ -170,7 +170,7 @@ static Indirect *ext4_get_branch(struct inode *inode, int depth,
 		}
 
 		if (!bh_uptodate_or_lock(bh)) {
-			if (ext4_read_bh(bh, 0, NULL) < 0) {
+			if (ext4_read_bh(bh, 0, NULL, false) < 0) {
 				put_bh(bh);
 				goto failure;
 			}
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
@@ -4504,10 +4504,10 @@ static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
 	 * Read the block from disk.
 	 */
 	trace_ext4_load_inode(sb, ino);
-	ext4_read_bh_nowait(bh, REQ_META | REQ_PRIO, NULL);
+	ext4_read_bh_nowait(bh, REQ_META | REQ_PRIO, NULL,
+			    ext4_simulate_fail(sb, EXT4_SIM_INODE_EIO));
 	blk_finish_plug(&plug);
 	wait_on_buffer(bh);
-	ext4_simulate_fail_bh(sb, bh, EXT4_SIM_INODE_EIO);
 	if (!buffer_uptodate(bh)) {
 		if (ret_block)
 			*ret_block = block;
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
@@ -94,7 +94,7 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
 	}
 
 	lock_buffer(*bh);
-	ret = ext4_read_bh(*bh, REQ_META | REQ_PRIO, NULL);
+	ret = ext4_read_bh(*bh, REQ_META | REQ_PRIO, NULL, false);
 	if (ret)
 		goto warn_exit;
 
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
@@ -213,7 +213,7 @@ static int mext_page_mkuptodate(struct folio *folio, size_t from, size_t to)
 			unlock_buffer(bh);
 			continue;
 		}
-		ext4_read_bh_nowait(bh, 0, NULL);
+		ext4_read_bh_nowait(bh, 0, NULL, false);
 		nr++;
 	} while (block++, (bh = bh->b_this_page) != head);
 
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
@@ -1300,7 +1300,7 @@ static struct buffer_head *ext4_get_bitmap(struct super_block *sb, __u64 block)
 	if (unlikely(!bh))
 		return NULL;
 	if (!bh_uptodate_or_lock(bh)) {
-		if (ext4_read_bh(bh, 0, NULL) < 0) {
+		if (ext4_read_bh(bh, 0, NULL, false) < 0) {
 			brelse(bh);
 			return NULL;
 		}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
@@ -161,8 +161,14 @@ MODULE_ALIAS("ext3");
 
 
 static inline void __ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,
-				  bh_end_io_t *end_io)
+				  bh_end_io_t *end_io, bool simu_fail)
 {
+	if (simu_fail) {
+		clear_buffer_uptodate(bh);
+		unlock_buffer(bh);
+		return;
+	}
+
 	/*
 	 * buffer's verified bit is no longer valid after reading from
 	 * disk again due to write out error, clear it to make sure we
@@ -176,18 +182,19 @@ static inline void __ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,
 }
 
 void ext4_read_bh_nowait(struct buffer_head *bh, blk_opf_t op_flags,
-			 bh_end_io_t *end_io)
+			 bh_end_io_t *end_io, bool simu_fail)
 {
 	BUG_ON(!buffer_locked(bh));
 
 	if (ext4_buffer_uptodate(bh)) {
 		unlock_buffer(bh);
 		return;
 	}
-	__ext4_read_bh(bh, op_flags, end_io);
+	__ext4_read_bh(bh, op_flags, end_io, simu_fail);
 }
 
-int ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags, bh_end_io_t *end_io)
+int ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,
+		 bh_end_io_t *end_io, bool simu_fail)
 {
 	BUG_ON(!buffer_locked(bh));
 
@@ -196,7 +203,7 @@ int ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags, bh_end_io_t *end_io
 		return 0;
 	}
 
-	__ext4_read_bh(bh, op_flags, end_io);
+	__ext4_read_bh(bh, op_flags, end_io, simu_fail);
 
 	wait_on_buffer(bh);
 	if (buffer_uptodate(bh))
@@ -208,10 +215,10 @@ int ext4_read_bh_lock(struct buffer_head *bh, blk_opf_t op_flags, bool wait)
 {
 	lock_buffer(bh);
 	if (!wait) {
-		ext4_read_bh_nowait(bh, op_flags, NULL);
+		ext4_read_bh_nowait(bh, op_flags, NULL, false);
 		return 0;
 	}
-	return ext4_read_bh(bh, op_flags, NULL);
+	return ext4_read_bh(bh, op_flags, NULL, false);
 }
 
 /*
@@ -266,7 +273,7 @@ void ext4_sb_breadahead_unmovable(struct super_block *sb, sector_t block)
 
 	if (likely(bh)) {
 		if (trylock_buffer(bh))
-			ext4_read_bh_nowait(bh, REQ_RAHEAD, NULL);
+			ext4_read_bh_nowait(bh, REQ_RAHEAD, NULL, false);
 		brelse(bh);
 	}
 }

Original file line number	Diff line number	Diff line change
`@@ -568,7 +568,7 @@ __read_extent_tree_block(const char *function, unsigned int line,`
`568`	`568`
`569`	`569`	`if (!bh_uptodate_or_lock(bh)) {`
`570`	`570`	`trace_ext4_ext_load_extent(inode, pblk, _RET_IP_);`
`571`		`- err = ext4_read_bh(bh, 0, NULL);`
	`571`	`+ err = ext4_read_bh(bh, 0, NULL, false);`
`572`	`572`	`if (err < 0)`
`573`	`573`	`goto errout;`
`574`	`574`	`}`
Original file line number	Diff line number	Diff line change
`@@ -170,7 +170,7 @@ static Indirect ext4_get_branch(struct inode inode, int depth,`
`170`	`170`	`}`
`171`	`171`
`172`	`172`	`if (!bh_uptodate_or_lock(bh)) {`
`173`		`- if (ext4_read_bh(bh, 0, NULL) < 0) {`
	`173`	`+ if (ext4_read_bh(bh, 0, NULL, false) < 0) {`
`174`	`174`	`put_bh(bh);`
`175`	`175`	`goto failure;`
`176`	`176`	`}`
Original file line number	Diff line number	Diff line change
`@@ -94,7 +94,7 @@ static int read_mmp_block(struct super_block sb, struct buffer_head *bh,`
`94`	`94`	`}`
`95`	`95`
`96`	`96`	`lock_buffer(*bh);`
`97`		`- ret = ext4_read_bh(*bh, REQ_META \| REQ_PRIO, NULL);`
	`97`	`+ ret = ext4_read_bh(*bh, REQ_META \| REQ_PRIO, NULL, false);`
`98`	`98`	`if (ret)`
`99`	`99`	`goto warn_exit;`
`100`	`100`
Original file line number	Diff line number	Diff line change
`@@ -213,7 +213,7 @@ static int mext_page_mkuptodate(struct folio *folio, size_t from, size_t to)`
`213`	`213`	`unlock_buffer(bh);`
`214`	`214`	`continue;`
`215`	`215`	`}`
`216`		`- ext4_read_bh_nowait(bh, 0, NULL);`
	`216`	`+ ext4_read_bh_nowait(bh, 0, NULL, false);`
`217`	`217`	`nr++;`
`218`	`218`	`} while (block++, (bh = bh->b_this_page) != head);`
`219`	`219`
Original file line number	Diff line number	Diff line change
`@@ -1300,7 +1300,7 @@ static struct buffer_head ext4_get_bitmap(struct super_block sb, __u64 block)`
`1300`	`1300`	`if (unlikely(!bh))`
`1301`	`1301`	`return NULL;`
`1302`	`1302`	`if (!bh_uptodate_or_lock(bh)) {`
`1303`		`- if (ext4_read_bh(bh, 0, NULL) < 0) {`
	`1303`	`+ if (ext4_read_bh(bh, 0, NULL, false) < 0) {`
`1304`	`1304`	`brelse(bh);`
`1305`	`1305`	`return NULL;`
`1306`	`1306`	`}`
Original file line number	Diff line number	Diff line change
`@@ -161,8 +161,14 @@ MODULE_ALIAS("ext3");`
`161`	`161`
`162`	`162`
`163`	`163`	`static inline void __ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,`
`164`		`- bh_end_io_t *end_io)`
	`164`	`+ bh_end_io_t *end_io, bool simu_fail)`
`165`	`165`	`{`
	`166`	`+ if (simu_fail) {`
	`167`	`+ clear_buffer_uptodate(bh);`
	`168`	`+ unlock_buffer(bh);`
	`169`	`+ return;`
	`170`	`+ }`
	`171`	`+`
`166`	`172`	`/*`
`167`	`173`	`* buffer's verified bit is no longer valid after reading from`
`168`	`174`	`* disk again due to write out error, clear it to make sure we`
`@@ -176,18 +182,19 @@ static inline void __ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,`
`176`	`182`	`}`
`177`	`183`
`178`	`184`	`void ext4_read_bh_nowait(struct buffer_head *bh, blk_opf_t op_flags,`
`179`		`- bh_end_io_t *end_io)`
	`185`	`+ bh_end_io_t *end_io, bool simu_fail)`
`180`	`186`	`{`
`181`	`187`	`BUG_ON(!buffer_locked(bh));`
`182`	`188`
`183`	`189`	`if (ext4_buffer_uptodate(bh)) {`
`184`	`190`	`unlock_buffer(bh);`
`185`	`191`	`return;`
`186`	`192`	`}`
`187`		`- __ext4_read_bh(bh, op_flags, end_io);`
	`193`	`+ __ext4_read_bh(bh, op_flags, end_io, simu_fail);`
`188`	`194`	`}`
`189`	`195`
`190`		`-int ext4_read_bh(struct buffer_head bh, blk_opf_t op_flags, bh_end_io_t end_io)`
	`196`	`+int ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,`
	`197`	`+ bh_end_io_t *end_io, bool simu_fail)`
`191`	`198`	`{`
`192`	`199`	`BUG_ON(!buffer_locked(bh));`
`193`	`200`
`@@ -196,7 +203,7 @@ int ext4_read_bh(struct buffer_head bh, blk_opf_t op_flags, bh_end_io_t end_io`
`196`	`203`	`return 0;`
`197`	`204`	`}`
`198`	`205`
`199`		`- __ext4_read_bh(bh, op_flags, end_io);`
	`206`	`+ __ext4_read_bh(bh, op_flags, end_io, simu_fail);`
`200`	`207`
`201`	`208`	`wait_on_buffer(bh);`
`202`	`209`	`if (buffer_uptodate(bh))`
`@@ -208,10 +215,10 @@ int ext4_read_bh_lock(struct buffer_head *bh, blk_opf_t op_flags, bool wait)`
`208`	`215`	`{`
`209`	`216`	`lock_buffer(bh);`
`210`	`217`	`if (!wait) {`
`211`		`- ext4_read_bh_nowait(bh, op_flags, NULL);`
	`218`	`+ ext4_read_bh_nowait(bh, op_flags, NULL, false);`
`212`	`219`	`return 0;`
`213`	`220`	`}`
`214`		`- return ext4_read_bh(bh, op_flags, NULL);`
	`221`	`+ return ext4_read_bh(bh, op_flags, NULL, false);`
`215`	`222`	`}`
`216`	`223`
`217`	`224`	`/*`
`@@ -266,7 +273,7 @@ void ext4_sb_breadahead_unmovable(struct super_block *sb, sector_t block)`
`266`	`273`
`267`	`274`	`if (likely(bh)) {`
`268`	`275`	`if (trylock_buffer(bh))`
`269`		`- ext4_read_bh_nowait(bh, REQ_RAHEAD, NULL);`
	`276`	`+ ext4_read_bh_nowait(bh, REQ_RAHEAD, NULL, false);`
`270`	`277`	`brelse(bh);`
`271`	`278`	`}`
`272`	`279`	`}`