linux-ext4 - Re: [RESEND PATCH] ext4: Fix race in buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241107155342.sonicmzg7leo63nq@quack3>
Date: Thu, 7 Nov 2024 16:53:42 +0100
From: Jan Kara <jack@...e.cz>
To: leo.lilong@...weicloud.com
Cc: tytso@....edu, adilger.kernel@...ger.ca, leo.lilong@...wei.com,
	linux-ext4@...r.kernel.org, yi.zhang@...wei.com,
	yangerkun@...wei.com
Subject: Re: [RESEND PATCH] ext4: Fix race in buffer_head read fault injection

On Thu 24-10-24 10:19:09, leo.lilong@...weicloud.com wrote:
> From: Long Li <leo.lilong@...wei.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@...wei.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
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR