Re: [PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode

[Baokun Li <libaokun1@huawei.com>]

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@huawei.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
>> 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
.