Re: [PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode
From: Baokun Li
Date: Tue Oct 25 2022 - 07:25:45 EST
On 2022/10/25 17:18, Jan Kara wrote:
On Tue 25-10-22 10:26:14, Baokun Li wrote:
On 2022/10/24 22:25, Jan Kara wrote:
On Fri 21-10-22 12:07:31, Baokun Li wrote:
We got a issue as fllows:
...
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@xxxxxxxxxx>
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
Hello Honza,
In ext4_iget(), the inode is marked as bad and returned only when ino is
equal to
EXT4_BOOT_LOADER_INO. In the error branch bad_inode, although the inode is
marked as bad, the returned value is the corresponding error number.
The boot loader inode is not initialized during mkfs. Therefore, when
ext4_iget() is
entered for the first time, imode of the inode is bad type. However, the
swap_inode_boot_loader() needs to obtain the inode for initialization and
swap.
Therefore, a bad_inode is returned in ext4_iget.
Generally, ext4_iget() does not get the boot loader inode. Therefore, we
only need
to pay attention to the special inodes that can be specified.
The following figure shows the check result:
1) usr_quota_inum/grp_quota_inum/prj_quota_inum
These inodes may be faulty. In the first patch, this situation is
intercepted.
At the beginning, FUZZ found that the quota inode was faulty. Later, we
found that
the operation function swap_inode_boot_loader() related to inode 5 was also
faulty.
2) journal_inum
In ext4_get_journal_inode(), the system checks whether the imode is S_IFREG.
Then,
the bmap in jbd2_journal_init_inode() checks whether the inode has
a_ops->bmap
operation. The bad inode does not set the bmap operation, so there is no
problem.
3) last_orphan
In ext4_orphan_get(), it checks if the imode is normal and if the inode is
bad inode,
so there is no problem.
4) snapshot_inum
No place to use snapshot_inum was found in the kernel, so there is no kernel
issue.
Thanks for detailed explanation! Now I agree you have actually covered all
the cases. But since EXT4_BOOT_LOADER_INO has this special behavior maybe
it would be more robust to create a special iget flag for it? Like
EXT4_IGET_BAD? And only with this flag we'd be returning bad inode from
ext4_iget(), otherwise we always return the error code?
Honza
Adding a special iget tag sounds great！
Thank you very much for your review！
I will send a patch V2 with the changes suggested by you.
--
With Best Regards,
Baokun Li
.