[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221024142527.avwgiztqvzmeo4se@quack3>
Date: Mon, 24 Oct 2022 16:25:27 +0200
From: Jan Kara <jack@...e.cz>
To: Baokun Li <libaokun1@...wei.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu,
adilger.kernel@...ger.ca, jack@...e.cz, ritesh.list@...il.com,
linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
yukuai3@...wei.com
Subject: Re: [PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by
wrong boot loader inode
On Fri 21-10-22 12:07:31, Baokun Li wrote:
> We got a issue as fllows:
> ==================================================================
> kernel BUG at fs/ext4/extents_status.c:203!
> invalid opcode: 0000 [#1] PREEMPT SMP
> CPU: 1 PID: 945 Comm: cat Not tainted 6.0.0-next-20221007-dirty #349
> RIP: 0010:ext4_es_end.isra.0+0x34/0x42
> RSP: 0018:ffffc9000143b768 EFLAGS: 00010203
> RAX: 0000000000000000 RBX: ffff8881769cd0b8 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffffffff8fc27cf7 RDI: 00000000ffffffff
> RBP: ffff8881769cd0bc R08: 0000000000000000 R09: ffffc9000143b5f8
> R10: 0000000000000001 R11: 0000000000000001 R12: ffff8881769cd0a0
> R13: ffff8881768e5668 R14: 00000000768e52f0 R15: 0000000000000000
> FS: 00007f359f7f05c0(0000)GS:ffff88842fd00000(0000)knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f359f5a2000 CR3: 000000017130c000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> __es_tree_search.isra.0+0x6d/0xf5
> ext4_es_cache_extent+0xfa/0x230
> ext4_cache_extents+0xd2/0x110
> ext4_find_extent+0x5d5/0x8c0
> ext4_ext_map_blocks+0x9c/0x1d30
> ext4_map_blocks+0x431/0xa50
> ext4_mpage_readpages+0x48e/0xe40
> ext4_readahead+0x47/0x50
> read_pages+0x82/0x530
> page_cache_ra_unbounded+0x199/0x2a0
> do_page_cache_ra+0x47/0x70
> page_cache_ra_order+0x242/0x400
> ondemand_readahead+0x1e8/0x4b0
> page_cache_sync_ra+0xf4/0x110
> filemap_get_pages+0x131/0xb20
> filemap_read+0xda/0x4b0
> generic_file_read_iter+0x13a/0x250
> ext4_file_read_iter+0x59/0x1d0
> vfs_read+0x28f/0x460
> ksys_read+0x73/0x160
> __x64_sys_read+0x1e/0x30
> do_syscall_64+0x35/0x80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> </TASK>
> ==================================================================
>
> In the above issue, ioctl invokes the swap_inode_boot_loader function to
> swap inode<5> and inode<12>. However, inode<5> contain incorrect imode and
> disordered extents, and i_nlink is set to 1. The extents check for inode in
> the ext4_iget function can be bypassed bacause 5 is EXT4_BOOT_LOADER_INO.
> While links_count is set to 1, the extents are not initialized in
> swap_inode_boot_loader. After the ioctl command is executed successfully,
> the extents are swapped to inode<12>, in this case, run the `cat` command
> to view inode<12>. And Bug_ON is triggered due to the incorrect extents.
>
> When the boot loader inode is not initialized, its imode can be one of the
> following:
> 1) the imode is a bad type, which is marked as bad_inode in ext4_iget and
> set to S_IFREG.
> 2) the imode is good type but not S_IFREG.
> 3) the imode is S_IFREG.
>
> The BUG_ON may be triggered by bypassing the check in cases 1 and 2.
> Therefore, when the boot loader inode is bad_inode or its imode is not
> S_IFREG, initialize the inode to avoid triggering the BUG.
>
> Signed-off-by: Baokun Li <libaokun1@...wei.com>
Grepping for calls to ext4_iget() in the ext4 code shows there are many
more places that will get unhappy (and crash) when ext4_iget() returns a
bad inode. In fact, I didn't find a place when returning bad inode would be
useful for anything. So why don't we just return EFSCORRUPTED instead of
returning a bad inode?
Honza
> ---
> fs/ext4/ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index ded535535b27..c41210706ea7 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -425,7 +425,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
> /* Protect extent tree against block allocations via delalloc */
> ext4_double_down_write_data_sem(inode, inode_bl);
>
> - if (inode_bl->i_nlink == 0) {
> + if (is_bad_inode(inode_bl) || !S_ISREG(inode_bl->i_mode)) {
> /* this inode has never been used as a BOOT_LOADER */
> set_nlink(inode_bl, 1);
> i_uid_write(inode_bl, 0);
> --
> 2.31.1
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists