Re: [RESEND PATCH] ext4: Fix race in buffer_head read fault injection

[Long Li <leo.lilong@huawei.com>]

On Thu, Nov 07, 2024 at 04:53:42PM +0100, Jan Kara wrote:
> On Thu 24-10-24 10:19:09, leo.lilong@huaweicloud.com wrote:
> > From: Long Li <leo.lilong@huawei.com>
> > 
> > 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: 46f870d690fe ("ext4: simulate various I/O and checksum errors when reading metadata")
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> 
> Thanks for the fix! One suggestion below:
> 
> > @@ -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);
> 
> Instead of adding a bool argument whether we should simulate a failure, I'd
> pass the 'code' into ext4_read_bh_nowait() and handle the check in there.
> That reduces the boilerplate code a bit and looks somewhat cleaner.
> 
> 								Honza

Hi Honza,

Thanks for your reply, your solution does appear more cleaner, but it seems
we cannot directly get sbi->s_simulate_fail in ext4_read_bh_nowait(), nor
can we get it in the IO completion callback function.

Thanks,
Long Li